Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move Deaggregate pass to post_borrowck_cleanup #71946

Closed
wants to merge 6 commits into from
Closed

move Deaggregate pass to post_borrowck_cleanup #71946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fc6b16c
move Deaggregate pass to post_borrowck_cleanup
RalfJung May 17, 2020
454871f
Gracefully degrade const prop to only-within-block const prop
oli-obk May 31, 2020
a10ac7d
Don't intern memory in const prop.
oli-obk Jun 3, 2020
ef005bb
Bump codegen test
oli-obk Jun 3, 2020
310ac1a
Add regression test showing the lack of branch optimization
oli-obk Jun 5, 2020
803e048
Const prop explicit discriminant setting
oli-obk Jun 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/librustc_mir/interpret/intern.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,6 @@ pub enum InternKind {
Static(hir::Mutability),
Constant,
Promoted,
ConstProp,
}

/// Intern `ret` and everything it references.
Expand All @@ -315,9 +314,7 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
let base_intern_mode = match intern_kind {
InternKind::Static(mutbl) => InternMode::Static(mutbl),
// FIXME: what about array lengths, array initializers?
InternKind::Constant | InternKind::ConstProp | InternKind::Promoted => {
InternMode::ConstBase
}
InternKind::Constant | InternKind::Promoted => InternMode::ConstBase,
};

// Type based interning.
Expand Down Expand Up @@ -359,7 +356,10 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
Err(error) => {
ecx.tcx.sess.delay_span_bug(
ecx.tcx.span,
"error during interning should later cause validation failure",
&format!(
"error during interning should later cause validation failure: {}",
error
),
);
// Some errors shouldn't come up because creating them causes
// an allocation, which we should avoid. When that happens,
Expand Down Expand Up @@ -400,7 +400,7 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
// immutability is so important.
alloc.mutability = Mutability::Not;
}
InternKind::Constant | InternKind::ConstProp => {
InternKind::Constant => {
// If it's a constant, we should not have any "leftovers" as everything
// is tracked by const-checking.
// FIXME: downgrade this to a warning? It rejects some legitimate consts,
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/interpret/step.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Ok(true)
}

fn statement(&mut self, stmt: &mir::Statement<'tcx>) -> InterpResult<'tcx> {
crate fn statement(&mut self, stmt: &mir::Statement<'tcx>) -> InterpResult<'tcx> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... of it's (semi-)public, maybe it should have a doc comment?

Also for consistency, terminator should probably get the same treatment.

info!("{:?}", stmt);
self.set_span(stmt.source_info.span);

Expand Down
21 changes: 11 additions & 10 deletions src/librustc_mir/transform/const_prop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ use rustc_trait_selection::traits;

use crate::const_eval::error_to_const_error;
use crate::interpret::{
self, compile_time_machine, intern_const_alloc_recursive, AllocId, Allocation, Frame, ImmTy,
Immediate, InternKind, InterpCx, LocalState, LocalValue, Memory, MemoryKind, OpTy,
Operand as InterpOperand, PlaceTy, Pointer, ScalarMaybeUninit, StackPopCleanup,
self, compile_time_machine, AllocId, Allocation, Frame, ImmTy, Immediate, InterpCx, LocalState,
LocalValue, Memory, MemoryKind, OpTy, Operand as InterpOperand, PlaceTy, Pointer,
ScalarMaybeUninit, StackPopCleanup,
};
use crate::transform::{MirPass, MirSource};

Expand Down Expand Up @@ -381,6 +381,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
match f(self) {
Ok(val) => Some(val),
Err(error) => {
trace!("InterpCx operation failed: {:?}", error);
// Some errors shouldn't come up because creating them causes
// an allocation, which we should avoid. When that happens,
// dedicated error variants should be introduced instead.
Expand Down Expand Up @@ -707,11 +708,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
ScalarMaybeUninit::Scalar(l),
ScalarMaybeUninit::Scalar(r),
)) => l.is_bits() && r.is_bits(),
interpret::Operand::Indirect(_) if mir_opt_level >= 2 => {
let mplace = op.assert_mem_place(&self.ecx);
intern_const_alloc_recursive(&mut self.ecx, InternKind::ConstProp, mplace, false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this needed / why is this no longer needed? Seems like a surprising change to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was buggy code that basically wasn't doing anything before this PR and was causing crashes with this PR. It's a leftover that multiple refactorings made obsolete. I can remove it on master if you prefer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's fine. One less interning entry point to worry about. :)

true
}
interpret::Operand::Indirect(_) if mir_opt_level >= 2 => true,
_ => false,
}
}
Expand Down Expand Up @@ -793,10 +790,11 @@ impl<'tcx> Visitor<'tcx> for CanConstProp {
// end of the block anyway, and inside the block we overwrite previous
// states as applicable.
ConstPropMode::OnlyInsideOwnBlock => {}
mode @ ConstPropMode::FullConstProp => *mode = ConstPropMode::OnlyInsideOwnBlock,
other => {
trace!(
"local {:?} can't be propagated because of multiple assignments",
local,
"local {:?} can't be propagated because of multiple assignments. Previous state: {:?}",
local, other,
);
*other = ConstPropMode::NoPropagation;
}
Expand Down Expand Up @@ -909,6 +907,9 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {
}
} else {
match statement.kind {
StatementKind::SetDiscriminant { .. } => {
self.use_ecx(|this| this.ecx.statement(statement));
}
StatementKind::StorageLive(local) | StatementKind::StorageDead(local) => {
let frame = self.ecx.frame_mut();
frame.locals[local].value =
Expand Down
11 changes: 3 additions & 8 deletions src/librustc_mir/transform/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,9 @@ fn run_optimization_passes<'tcx>(
// but before optimizations begin.
&add_retag::AddRetag,
&simplify::SimplifyCfg::new("elaborate-drops"),
// `Deaggregator` is conceptually part of MIR building, some backends rely on it happening
// and it can help optimizations.
&deaggregator::Deaggregator,
// No lifetime analysis based on borrowing can be done from here on out.
];

Expand All @@ -334,11 +337,6 @@ fn run_optimization_passes<'tcx>(
&instcombine::InstCombine,
&const_prop::ConstProp,
&simplify_branches::SimplifyBranches::new("after-const-prop"),
// Run deaggregation here because:
// 1. Some codegen backends require it
// 2. It creates additional possibilities for some MIR optimizations to trigger
// FIXME(#70073): Why is this done here and not in `post_borrowck_cleanup`?
&deaggregator::Deaggregator,
&simplify_try::SimplifyArmIdentity,
&simplify_try::SimplifyBranchSame,
&copy_prop::CopyPropagation,
Expand All @@ -355,9 +353,6 @@ fn run_optimization_passes<'tcx>(
&generator::StateTransform,
// FIXME(#70073): This pass is responsible for both optimization as well as some lints.
&const_prop::ConstProp,
// Even if we don't do optimizations, still run deaggregation because some backends assume
// that deaggregation always occurs.
&deaggregator::Deaggregator,
];

let pre_codegen_cleanup: &[&dyn MirPass<'tcx>] = &[
Expand Down
4 changes: 2 additions & 2 deletions src/test/codegen/consts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@
// CHECK: @STATIC = {{.*}}, align 4

// This checks the constants from inline_enum_const
// CHECK: @alloc7 = {{.*}}, align 2
// CHECK: @alloc8 = {{.*}}, align 2

// This checks the constants from {low,high}_align_const, they share the same
// constant, but the alignment differs, so the higher one should be used
// CHECK: [[LOW_HIGH:@[0-9]+]] = {{.*}} getelementptr inbounds (<{ [8 x i8] }>, <{ [8 x i8] }>* @alloc19, i32 0, i32 0, i32 0), {{.*}}
// CHECK: [[LOW_HIGH:@[0-9]+]] = {{.*}} getelementptr inbounds (<{ [8 x i8] }>, <{ [8 x i8] }>* @alloc20, i32 0, i32 0, i32 0), {{.*}}

#[derive(Copy, Clone)]
// repr(i16) is required for the {low,high}_align_const test
Expand Down
4 changes: 3 additions & 1 deletion src/test/mir-opt/const_prop/aggregate/rustc.main.ConstProp.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,21 @@
StorageLive(_1); // scope 0 at $DIR/aggregate.rs:5:9: 5:10
StorageLive(_2); // scope 0 at $DIR/aggregate.rs:5:13: 5:24
StorageLive(_3); // scope 0 at $DIR/aggregate.rs:5:13: 5:22
_3 = (const 0i32, const 1i32, const 2i32); // scope 0 at $DIR/aggregate.rs:5:13: 5:22
(_3.0: i32) = const 0i32; // scope 0 at $DIR/aggregate.rs:5:13: 5:22
// ty::Const
// + ty: i32
// + val: Value(Scalar(0x00000000))
// mir::Constant
// + span: $DIR/aggregate.rs:5:14: 5:15
// + literal: Const { ty: i32, val: Value(Scalar(0x00000000)) }
(_3.1: i32) = const 1i32; // scope 0 at $DIR/aggregate.rs:5:13: 5:22
// ty::Const
// + ty: i32
// + val: Value(Scalar(0x00000001))
// mir::Constant
// + span: $DIR/aggregate.rs:5:17: 5:18
// + literal: Const { ty: i32, val: Value(Scalar(0x00000001)) }
(_3.2: i32) = const 2i32; // scope 0 at $DIR/aggregate.rs:5:13: 5:22
// ty::Const
// + ty: i32
// + val: Value(Scalar(0x00000002))
Expand Down
22 changes: 6 additions & 16 deletions src/test/mir-opt/const_prop/discriminant/32bit/rustc.main.ConstProp.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,16 @@
StorageLive(_1); // scope 0 at $DIR/discriminant.rs:6:9: 6:10
StorageLive(_2); // scope 0 at $DIR/discriminant.rs:6:13: 6:64
StorageLive(_3); // scope 0 at $DIR/discriminant.rs:6:34: 6:44
- _3 = std::option::Option::<bool>::Some(const true); // scope 0 at $DIR/discriminant.rs:6:34: 6:44
+ _3 = const std::option::Option::<bool>::Some(true); // scope 0 at $DIR/discriminant.rs:6:34: 6:44
((_3 as Some).0: bool) = const true; // scope 0 at $DIR/discriminant.rs:6:34: 6:44
// ty::Const
- // + ty: bool
+ // + ty: std::option::Option<bool>
// + ty: bool
// + val: Value(Scalar(0x01))
// mir::Constant
- // + span: $DIR/discriminant.rs:6:39: 6:43
- // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
// + span: $DIR/discriminant.rs:6:39: 6:43
// + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
discriminant(_3) = 1; // scope 0 at $DIR/discriminant.rs:6:34: 6:44
- _4 = discriminant(_3); // scope 0 at $DIR/discriminant.rs:6:21: 6:31
- switchInt(move _4) -> [1isize: bb2, otherwise: bb1]; // scope 0 at $DIR/discriminant.rs:6:21: 6:31
+ // + span: $DIR/discriminant.rs:6:34: 6:44
+ // + literal: Const { ty: std::option::Option<bool>, val: Value(Scalar(0x01)) }
+ _4 = const 1isize; // scope 0 at $DIR/discriminant.rs:6:21: 6:31
+ // ty::Const
+ // + ty: isize
Expand Down Expand Up @@ -56,14 +53,7 @@
}

bb2: {
- switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:6:26: 6:30
+ switchInt(const true) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:6:26: 6:30
+ // ty::Const
+ // + ty: bool
+ // + val: Value(Scalar(0x01))
+ // mir::Constant
+ // + span: $DIR/discriminant.rs:6:26: 6:30
+ // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:6:26: 6:30
}

bb3: {
Expand Down
22 changes: 6 additions & 16 deletions src/test/mir-opt/const_prop/discriminant/64bit/rustc.main.ConstProp.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,16 @@
StorageLive(_1); // scope 0 at $DIR/discriminant.rs:6:9: 6:10
StorageLive(_2); // scope 0 at $DIR/discriminant.rs:6:13: 6:64
StorageLive(_3); // scope 0 at $DIR/discriminant.rs:6:34: 6:44
- _3 = std::option::Option::<bool>::Some(const true); // scope 0 at $DIR/discriminant.rs:6:34: 6:44
+ _3 = const std::option::Option::<bool>::Some(true); // scope 0 at $DIR/discriminant.rs:6:34: 6:44
((_3 as Some).0: bool) = const true; // scope 0 at $DIR/discriminant.rs:6:34: 6:44
// ty::Const
- // + ty: bool
+ // + ty: std::option::Option<bool>
// + ty: bool
// + val: Value(Scalar(0x01))
// mir::Constant
- // + span: $DIR/discriminant.rs:6:39: 6:43
- // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
// + span: $DIR/discriminant.rs:6:39: 6:43
// + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
discriminant(_3) = 1; // scope 0 at $DIR/discriminant.rs:6:34: 6:44
- _4 = discriminant(_3); // scope 0 at $DIR/discriminant.rs:6:21: 6:31
- switchInt(move _4) -> [1isize: bb2, otherwise: bb1]; // scope 0 at $DIR/discriminant.rs:6:21: 6:31
+ // + span: $DIR/discriminant.rs:6:34: 6:44
+ // + literal: Const { ty: std::option::Option<bool>, val: Value(Scalar(0x01)) }
+ _4 = const 1isize; // scope 0 at $DIR/discriminant.rs:6:21: 6:31
+ // ty::Const
+ // + ty: isize
Expand Down Expand Up @@ -56,14 +53,7 @@
}

bb2: {
- switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:6:26: 6:30
+ switchInt(const true) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:6:26: 6:30
+ // ty::Const
+ // + ty: bool
+ // + val: Value(Scalar(0x01))
+ // mir::Constant
+ // + span: $DIR/discriminant.rs:6:26: 6:30
+ // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:6:26: 6:30
}

bb3: {
Expand Down
9 changes: 5 additions & 4 deletions src/test/mir-opt/const_prop/issue-66971/rustc.main.ConstProp.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,22 @@
StorageLive(_1); // scope 0 at $DIR/issue-66971.rs:16:5: 16:23
StorageLive(_2); // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
StorageLive(_3); // scope 0 at $DIR/issue-66971.rs:16:13: 16:15
- _3 = (); // scope 0 at $DIR/issue-66971.rs:16:13: 16:15
+ _3 = const (); // scope 0 at $DIR/issue-66971.rs:16:13: 16:15
- (_2.0: ()) = move _3; // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
+ (_2.0: ()) = const (); // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
+ // ty::Const
+ // + ty: ()
+ // + val: Value(Scalar(<ZST>))
+ // mir::Constant
+ // + span: $DIR/issue-66971.rs:16:13: 16:15
+ // + span: $DIR/issue-66971.rs:16:12: 16:22
+ // + literal: Const { ty: (), val: Value(Scalar(<ZST>)) }
_2 = (move _3, const 0u8, const 0u8); // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
(_2.1: u8) = const 0u8; // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
// ty::Const
// + ty: u8
// + val: Value(Scalar(0x00))
// mir::Constant
// + span: $DIR/issue-66971.rs:16:17: 16:18
// + literal: Const { ty: u8, val: Value(Scalar(0x00)) }
(_2.2: u8) = const 0u8; // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
// ty::Const
// + ty: u8
// + val: Value(Scalar(0x00))
Expand Down
Loading