Skip to content

Commit

Permalink
Auto merge of rust-lang#73656 - oli-obk:deaggregate-is-cleanup, r=wes…
Browse files Browse the repository at this point in the history 
…leywiser

move Deaggregate pass to post_borrowck_cleanup

Reopen of rust-lang#71946

Only the second commit is from this PR, the other commit is a bugfix that's in the process of getting merged. I'll rebase once that's done

In rust-lang#70073 MIR pass handling got reorganized, but with the goal of not changing behavior (except for disabling some optimizations on opt-level = 0). But there we realized that the Deaggregator pass, while conceptually more of a "cleanup" pass (and one that should be run before optimizations), was run in the middle of the optimization chain. Likely this is an accident of history, so I suggest we try and clean that up by making it a proper cleanup pass.

This does change mir-opt output, because deaggregation now runs before const-prop instead of after.

r? @wesleywiser @rust-lang/wg-mir-opt

cc @RalfJung
  • Loading branch information
@bors
bors committed Aug 11, 2020
2 parents 4b9ac51 + 307d0d8 commit cbe7c5c
Show file tree
Hide file tree
Showing 40 changed files with 793 additions and 357 deletions.
4 changes: 3 additions & 1 deletion src/librustc_mir/interpret/step.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Ok(true)
}

fn statement(&mut self, stmt: &mir::Statement<'tcx>) -> InterpResult<'tcx> {
/// Runs the interpretation logic for the given `mir::Statement` at the current frame and
/// statement counter. This also moves the statement counter forward.
crate fn statement(&mut self, stmt: &mir::Statement<'tcx>) -> InterpResult<'tcx> {
info!("{:?}", stmt);

use rustc_middle::mir::StatementKind::*;
Expand Down
26 changes: 23 additions & 3 deletions src/librustc_mir/transform/const_prop.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,11 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>
throw_machine_stop_str!("tried to write to a local that is marked as not propagatable")
}
if frame == 0 && ecx.machine.only_propagate_inside_block_locals.contains(local) {
trace!(
"mutating local {:?} which is restricted to its block. \
Will remove it from const-prop after block is finished.",
local
);
ecx.machine.written_only_inside_own_block_locals.insert(local);
}
ecx.machine.stack[frame].locals[local].access_mut()
Expand Down Expand Up @@ -427,6 +432,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 @@ -969,10 +975,10 @@ impl<'tcx> Visitor<'tcx> for CanConstProp {
ConstPropMode::OnlyPropagateInto => {}
other @ ConstPropMode::FullConstProp => {
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::OnlyPropagateInto;
*other = ConstPropMode::OnlyInsideOwnBlock;
}
}
}
Expand Down Expand Up @@ -1089,6 +1095,20 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {
}
} else {
match statement.kind {
StatementKind::SetDiscriminant { ref place, .. } => {
match self.ecx.machine.can_const_prop[place.local] {
ConstPropMode::FullConstProp | ConstPropMode::OnlyInsideOwnBlock => {
if self.use_ecx(|this| this.ecx.statement(statement)).is_some() {
trace!("propped discriminant into {:?}", place);
} else {
Self::remove_const(&mut self.ecx, place.local);
}
}
ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => {
Self::remove_const(&mut self.ecx, place.local);
}
}
}
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
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,9 @@ fn run_post_borrowck_cleanup_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,
];

run_passes(
Expand Down Expand Up @@ -439,11 +442,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 @@ -460,9 +458,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
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.main.ConstProp.diff
View file
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 0_i32, const 1_i32, const 2_i32); // scope 0 at $DIR/aggregate.rs:5:13: 5:22
(_3.0: i32) = const 0_i32; // 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 1_i32; // 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 2_i32; // 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.main.ConstProp.diff.32bit
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,16 @@
StorageLive(_1); // scope 0 at $DIR/discriminant.rs:11:9: 11:10
StorageLive(_2); // scope 0 at $DIR/discriminant.rs:11:13: 11:64
StorageLive(_3); // scope 0 at $DIR/discriminant.rs:11:34: 11:44
- _3 = std::option::Option::<bool>::Some(const true); // scope 0 at $DIR/discriminant.rs:11:34: 11:44
+ _3 = const std::option::Option::<bool>::Some(true); // scope 0 at $DIR/discriminant.rs:11:34: 11:44
((_3 as Some).0: bool) = const true; // scope 0 at $DIR/discriminant.rs:11:34: 11:44
// ty::Const
- // + ty: bool
+ // + ty: std::option::Option<bool>
// + ty: bool
// + val: Value(Scalar(0x01))
// mir::Constant
- // + span: $DIR/discriminant.rs:11:39: 11:43
- // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
// + span: $DIR/discriminant.rs:11:39: 11:43
// + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
discriminant(_3) = 1; // scope 0 at $DIR/discriminant.rs:11:34: 11:44
- _4 = discriminant(_3); // scope 0 at $DIR/discriminant.rs:11:21: 11:31
- switchInt(move _4) -> [1_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/discriminant.rs:11:21: 11:31
+ // + span: $DIR/discriminant.rs:11:34: 11:44
+ // + literal: Const { ty: std::option::Option<bool>, val: Value(Scalar(0x01)) }
+ _4 = const 1_isize; // scope 0 at $DIR/discriminant.rs:11:21: 11: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:11:26: 11:30
+ switchInt(const true) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:26: 11:30
+ // ty::Const
+ // + ty: bool
+ // + val: Value(Scalar(0x01))
+ // mir::Constant
+ // + span: $DIR/discriminant.rs:11:26: 11: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:11:26: 11:30
}

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

bb3: {
Expand Down
9 changes: 5 additions & 4 deletions src/test/mir-opt/const_prop/issue_66971.main.ConstProp.diff
View file
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 0_u8, const 0_u8); // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
(_2.1: u8) = const 0_u8; // 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 0_u8; // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
// ty::Const
// + ty: u8
// + val: Value(Scalar(0x00))
Expand Down
24 changes: 18 additions & 6 deletions src/test/mir-opt/const_prop/issue_67019.main.ConstProp.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,34 @@
StorageLive(_1); // scope 0 at $DIR/issue-67019.rs:11:5: 11:20
StorageLive(_2); // scope 0 at $DIR/issue-67019.rs:11:10: 11:19
StorageLive(_3); // scope 0 at $DIR/issue-67019.rs:11:11: 11:17
_3 = (const 1_u8, const 2_u8); // scope 0 at $DIR/issue-67019.rs:11:11: 11:17
(_3.0: u8) = const 1_u8; // scope 0 at $DIR/issue-67019.rs:11:11: 11:17
// ty::Const
// + ty: u8
// + val: Value(Scalar(0x01))
// mir::Constant
- // + span: $DIR/issue-67019.rs:11:12: 11:13
+ // + span: $DIR/issue-67019.rs:11:11: 11:17
// + span: $DIR/issue-67019.rs:11:12: 11:13
// + literal: Const { ty: u8, val: Value(Scalar(0x01)) }
(_3.1: u8) = const 2_u8; // scope 0 at $DIR/issue-67019.rs:11:11: 11:17
// ty::Const
// + ty: u8
// + val: Value(Scalar(0x02))
// mir::Constant
- // + span: $DIR/issue-67019.rs:11:15: 11:16
+ // + span: $DIR/issue-67019.rs:11:11: 11:17
// + span: $DIR/issue-67019.rs:11:15: 11:16
// + literal: Const { ty: u8, val: Value(Scalar(0x02)) }
_2 = (move _3,); // scope 0 at $DIR/issue-67019.rs:11:10: 11:19
- (_2.0: (u8, u8)) = move _3; // scope 0 at $DIR/issue-67019.rs:11:10: 11:19
+ (_2.0: (u8, u8)) = (const 1_u8, const 2_u8); // scope 0 at $DIR/issue-67019.rs:11:10: 11:19
+ // ty::Const
+ // + ty: u8
+ // + val: Value(Scalar(0x01))
+ // mir::Constant
+ // + span: $DIR/issue-67019.rs:11:10: 11:19
+ // + literal: Const { ty: u8, val: Value(Scalar(0x01)) }
+ // ty::Const
+ // + ty: u8
+ // + val: Value(Scalar(0x02))
+ // mir::Constant
+ // + span: $DIR/issue-67019.rs:11:10: 11:19
+ // + literal: Const { ty: u8, val: Value(Scalar(0x02)) }
StorageDead(_3); // scope 0 at $DIR/issue-67019.rs:11:18: 11:19
_1 = const test(move _2) -> bb1; // scope 0 at $DIR/issue-67019.rs:11:5: 11:20
// ty::Const
Expand Down
Loading

0 comments on commit cbe7c5c

Please sign in to comment.