Skip to content

Commit cbe7c5c

Browse files
borsbors
committed
Auto merge of #73656 - oli-obk:deaggregate-is-cleanup, r=wesleywiser
move Deaggregate pass to post_borrowck_cleanup Reopen of #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 #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
2 parents 4b9ac51 + 307d0d8 commit cbe7c5c

File tree

40 files changed

+793
-357
lines changed

40 files changed

+793
-357
lines changed

Diff for: ‎src/librustc_mir/interpret/step.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
7474
Ok(true)
7575
}
7676

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

8082
use rustc_middle::mir::StatementKind::*;

Diff for: ‎src/librustc_mir/transform/const_prop.rs

+23-3
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,11 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>
252252
throw_machine_stop_str!("tried to write to a local that is marked as not propagatable")
253253
}
254254
if frame == 0 && ecx.machine.only_propagate_inside_block_locals.contains(local) {
255+
trace!(
256+
"mutating local {:?} which is restricted to its block. \
257+
Will remove it from const-prop after block is finished.",
258+
local
259+
);
255260
ecx.machine.written_only_inside_own_block_locals.insert(local);
256261
}
257262
ecx.machine.stack[frame].locals[local].access_mut()
@@ -427,6 +432,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
427432
match f(self) {
428433
Ok(val) => Some(val),
429434
Err(error) => {
435+
trace!("InterpCx operation failed: {:?}", error);
430436
// Some errors shouldn't come up because creating them causes
431437
// an allocation, which we should avoid. When that happens,
432438
// dedicated error variants should be introduced instead.
@@ -969,10 +975,10 @@ impl<'tcx> Visitor<'tcx> for CanConstProp {
969975
ConstPropMode::OnlyPropagateInto => {}
970976
other @ ConstPropMode::FullConstProp => {
971977
trace!(
972-
"local {:?} can't be propagated because of multiple assignments",
973-
local,
978+
"local {:?} can't be propagated because of multiple assignments. Previous state: {:?}",
979+
local, other,
974980
);
975-
*other = ConstPropMode::OnlyPropagateInto;
981+
*other = ConstPropMode::OnlyInsideOwnBlock;
976982
}
977983
}
978984
}
@@ -1089,6 +1095,20 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {
10891095
}
10901096
} else {
10911097
match statement.kind {
1098+
StatementKind::SetDiscriminant { ref place, .. } => {
1099+
match self.ecx.machine.can_const_prop[place.local] {
1100+
ConstPropMode::FullConstProp | ConstPropMode::OnlyInsideOwnBlock => {
1101+
if self.use_ecx(|this| this.ecx.statement(statement)).is_some() {
1102+
trace!("propped discriminant into {:?}", place);
1103+
} else {
1104+
Self::remove_const(&mut self.ecx, place.local);
1105+
}
1106+
}
1107+
ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => {
1108+
Self::remove_const(&mut self.ecx, place.local);
1109+
}
1110+
}
1111+
}
10921112
StatementKind::StorageLive(local) | StatementKind::StorageDead(local) => {
10931113
let frame = self.ecx.frame_mut();
10941114
frame.locals[local].value =

Diff for: ‎src/librustc_mir/transform/mod.rs

+3-8
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,9 @@ fn run_post_borrowck_cleanup_passes<'tcx>(
408408
// but before optimizations begin.
409409
&add_retag::AddRetag,
410410
&simplify::SimplifyCfg::new("elaborate-drops"),
411+
// `Deaggregator` is conceptually part of MIR building, some backends rely on it happening
412+
// and it can help optimizations.
413+
&deaggregator::Deaggregator,
411414
];
412415

413416
run_passes(
@@ -439,11 +442,6 @@ fn run_optimization_passes<'tcx>(
439442
&instcombine::InstCombine,
440443
&const_prop::ConstProp,
441444
&simplify_branches::SimplifyBranches::new("after-const-prop"),
442-
// Run deaggregation here because:
443-
// 1. Some codegen backends require it
444-
// 2. It creates additional possibilities for some MIR optimizations to trigger
445-
// FIXME(#70073): Why is this done here and not in `post_borrowck_cleanup`?
446-
&deaggregator::Deaggregator,
447445
&simplify_try::SimplifyArmIdentity,
448446
&simplify_try::SimplifyBranchSame,
449447
&copy_prop::CopyPropagation,
@@ -460,9 +458,6 @@ fn run_optimization_passes<'tcx>(
460458
&generator::StateTransform,
461459
// FIXME(#70073): This pass is responsible for both optimization as well as some lints.
462460
&const_prop::ConstProp,
463-
// Even if we don't do optimizations, still run deaggregation because some backends assume
464-
// that deaggregation always occurs.
465-
&deaggregator::Deaggregator,
466461
];
467462

468463
let pre_codegen_cleanup: &[&dyn MirPass<'tcx>] = &[

Diff for: ‎src/test/codegen/consts.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,11 @@
1010
// CHECK: @STATIC = {{.*}}, align 4
1111

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

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

1919
#[derive(Copy, Clone)]
2020
// repr(i16) is required for the {low,high}_align_const test

Diff for: ‎src/test/mir-opt/const_prop/aggregate.main.ConstProp.diff

+3-1
Original file line numberDiff line numberDiff line change
@@ -14,19 +14,21 @@
1414
StorageLive(_1); // scope 0 at $DIR/aggregate.rs:5:9: 5:10
1515
StorageLive(_2); // scope 0 at $DIR/aggregate.rs:5:13: 5:24
1616
StorageLive(_3); // scope 0 at $DIR/aggregate.rs:5:13: 5:22
17-
_3 = (const 0_i32, const 1_i32, const 2_i32); // scope 0 at $DIR/aggregate.rs:5:13: 5:22
17+
(_3.0: i32) = const 0_i32; // scope 0 at $DIR/aggregate.rs:5:13: 5:22
1818
// ty::Const
1919
// + ty: i32
2020
// + val: Value(Scalar(0x00000000))
2121
// mir::Constant
2222
// + span: $DIR/aggregate.rs:5:14: 5:15
2323
// + literal: Const { ty: i32, val: Value(Scalar(0x00000000)) }
24+
(_3.1: i32) = const 1_i32; // scope 0 at $DIR/aggregate.rs:5:13: 5:22
2425
// ty::Const
2526
// + ty: i32
2627
// + val: Value(Scalar(0x00000001))
2728
// mir::Constant
2829
// + span: $DIR/aggregate.rs:5:17: 5:18
2930
// + literal: Const { ty: i32, val: Value(Scalar(0x00000001)) }
31+
(_3.2: i32) = const 2_i32; // scope 0 at $DIR/aggregate.rs:5:13: 5:22
3032
// ty::Const
3133
// + ty: i32
3234
// + val: Value(Scalar(0x00000002))

Diff for: ‎src/test/mir-opt/const_prop/discriminant.main.ConstProp.diff.32bit

+6-16
Original file line numberDiff line numberDiff line change
@@ -15,19 +15,16 @@
1515
StorageLive(_1); // scope 0 at $DIR/discriminant.rs:11:9: 11:10
1616
StorageLive(_2); // scope 0 at $DIR/discriminant.rs:11:13: 11:64
1717
StorageLive(_3); // scope 0 at $DIR/discriminant.rs:11:34: 11:44
18-
- _3 = std::option::Option::<bool>::Some(const true); // scope 0 at $DIR/discriminant.rs:11:34: 11:44
19-
+ _3 = const std::option::Option::<bool>::Some(true); // scope 0 at $DIR/discriminant.rs:11:34: 11:44
18+
((_3 as Some).0: bool) = const true; // scope 0 at $DIR/discriminant.rs:11:34: 11:44
2019
// ty::Const
21-
- // + ty: bool
22-
+ // + ty: std::option::Option<bool>
20+
// + ty: bool
2321
// + val: Value(Scalar(0x01))
2422
// mir::Constant
25-
- // + span: $DIR/discriminant.rs:11:39: 11:43
26-
- // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
23+
// + span: $DIR/discriminant.rs:11:39: 11:43
24+
// + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
25+
discriminant(_3) = 1; // scope 0 at $DIR/discriminant.rs:11:34: 11:44
2726
- _4 = discriminant(_3); // scope 0 at $DIR/discriminant.rs:11:21: 11:31
2827
- switchInt(move _4) -> [1_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/discriminant.rs:11:21: 11:31
29-
+ // + span: $DIR/discriminant.rs:11:34: 11:44
30-
+ // + literal: Const { ty: std::option::Option<bool>, val: Value(Scalar(0x01)) }
3128
+ _4 = const 1_isize; // scope 0 at $DIR/discriminant.rs:11:21: 11:31
3229
+ // ty::Const
3330
+ // + ty: isize
@@ -56,14 +53,7 @@
5653
}
5754

5855
bb2: {
59-
- switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:26: 11:30
60-
+ switchInt(const true) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:26: 11:30
61-
+ // ty::Const
62-
+ // + ty: bool
63-
+ // + val: Value(Scalar(0x01))
64-
+ // mir::Constant
65-
+ // + span: $DIR/discriminant.rs:11:26: 11:30
66-
+ // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
56+
switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:26: 11:30
6757
}
6858

6959
bb3: {

Diff for: ‎src/test/mir-opt/const_prop/discriminant.main.ConstProp.diff.64bit

+6-16
Original file line numberDiff line numberDiff line change
@@ -15,19 +15,16 @@
1515
StorageLive(_1); // scope 0 at $DIR/discriminant.rs:11:9: 11:10
1616
StorageLive(_2); // scope 0 at $DIR/discriminant.rs:11:13: 11:64
1717
StorageLive(_3); // scope 0 at $DIR/discriminant.rs:11:34: 11:44
18-
- _3 = std::option::Option::<bool>::Some(const true); // scope 0 at $DIR/discriminant.rs:11:34: 11:44
19-
+ _3 = const std::option::Option::<bool>::Some(true); // scope 0 at $DIR/discriminant.rs:11:34: 11:44
18+
((_3 as Some).0: bool) = const true; // scope 0 at $DIR/discriminant.rs:11:34: 11:44
2019
// ty::Const
21-
- // + ty: bool
22-
+ // + ty: std::option::Option<bool>
20+
// + ty: bool
2321
// + val: Value(Scalar(0x01))
2422
// mir::Constant
25-
- // + span: $DIR/discriminant.rs:11:39: 11:43
26-
- // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
23+
// + span: $DIR/discriminant.rs:11:39: 11:43
24+
// + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
25+
discriminant(_3) = 1; // scope 0 at $DIR/discriminant.rs:11:34: 11:44
2726
- _4 = discriminant(_3); // scope 0 at $DIR/discriminant.rs:11:21: 11:31
2827
- switchInt(move _4) -> [1_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/discriminant.rs:11:21: 11:31
29-
+ // + span: $DIR/discriminant.rs:11:34: 11:44
30-
+ // + literal: Const { ty: std::option::Option<bool>, val: Value(Scalar(0x01)) }
3128
+ _4 = const 1_isize; // scope 0 at $DIR/discriminant.rs:11:21: 11:31
3229
+ // ty::Const
3330
+ // + ty: isize
@@ -56,14 +53,7 @@
5653
}
5754

5855
bb2: {
59-
- switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:26: 11:30
60-
+ switchInt(const true) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:26: 11:30
61-
+ // ty::Const
62-
+ // + ty: bool
63-
+ // + val: Value(Scalar(0x01))
64-
+ // mir::Constant
65-
+ // + span: $DIR/discriminant.rs:11:26: 11:30
66-
+ // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
56+
switchInt(((_3 as Some).0: bool)) -> [false: bb1, otherwise: bb3]; // scope 0 at $DIR/discriminant.rs:11:26: 11:30
6757
}
6858

6959
bb3: {

Diff for: ‎src/test/mir-opt/const_prop/issue_66971.main.ConstProp.diff

+5-4
Original file line numberDiff line numberDiff line change
@@ -11,21 +11,22 @@
1111
StorageLive(_1); // scope 0 at $DIR/issue-66971.rs:16:5: 16:23
1212
StorageLive(_2); // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
1313
StorageLive(_3); // scope 0 at $DIR/issue-66971.rs:16:13: 16:15
14-
- _3 = (); // scope 0 at $DIR/issue-66971.rs:16:13: 16:15
15-
+ _3 = const (); // scope 0 at $DIR/issue-66971.rs:16:13: 16:15
14+
- (_2.0: ()) = move _3; // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
15+
+ (_2.0: ()) = const (); // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
1616
+ // ty::Const
1717
+ // + ty: ()
1818
+ // + val: Value(Scalar(<ZST>))
1919
+ // mir::Constant
20-
+ // + span: $DIR/issue-66971.rs:16:13: 16:15
20+
+ // + span: $DIR/issue-66971.rs:16:12: 16:22
2121
+ // + literal: Const { ty: (), val: Value(Scalar(<ZST>)) }
22-
_2 = (move _3, const 0_u8, const 0_u8); // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
22+
(_2.1: u8) = const 0_u8; // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
2323
// ty::Const
2424
// + ty: u8
2525
// + val: Value(Scalar(0x00))
2626
// mir::Constant
2727
// + span: $DIR/issue-66971.rs:16:17: 16:18
2828
// + literal: Const { ty: u8, val: Value(Scalar(0x00)) }
29+
(_2.2: u8) = const 0_u8; // scope 0 at $DIR/issue-66971.rs:16:12: 16:22
2930
// ty::Const
3031
// + ty: u8
3132
// + val: Value(Scalar(0x00))

Diff for: ‎src/test/mir-opt/const_prop/issue_67019.main.ConstProp.diff

+18-6
Original file line numberDiff line numberDiff line change
@@ -11,22 +11,34 @@
1111
StorageLive(_1); // scope 0 at $DIR/issue-67019.rs:11:5: 11:20
1212
StorageLive(_2); // scope 0 at $DIR/issue-67019.rs:11:10: 11:19
1313
StorageLive(_3); // scope 0 at $DIR/issue-67019.rs:11:11: 11:17
14-
_3 = (const 1_u8, const 2_u8); // scope 0 at $DIR/issue-67019.rs:11:11: 11:17
14+
(_3.0: u8) = const 1_u8; // scope 0 at $DIR/issue-67019.rs:11:11: 11:17
1515
// ty::Const
1616
// + ty: u8
1717
// + val: Value(Scalar(0x01))
1818
// mir::Constant
19-
- // + span: $DIR/issue-67019.rs:11:12: 11:13
20-
+ // + span: $DIR/issue-67019.rs:11:11: 11:17
19+
// + span: $DIR/issue-67019.rs:11:12: 11:13
2120
// + literal: Const { ty: u8, val: Value(Scalar(0x01)) }
21+
(_3.1: u8) = const 2_u8; // scope 0 at $DIR/issue-67019.rs:11:11: 11:17
2222
// ty::Const
2323
// + ty: u8
2424
// + val: Value(Scalar(0x02))
2525
// mir::Constant
26-
- // + span: $DIR/issue-67019.rs:11:15: 11:16
27-
+ // + span: $DIR/issue-67019.rs:11:11: 11:17
26+
// + span: $DIR/issue-67019.rs:11:15: 11:16
2827
// + literal: Const { ty: u8, val: Value(Scalar(0x02)) }
29-
_2 = (move _3,); // scope 0 at $DIR/issue-67019.rs:11:10: 11:19
28+
- (_2.0: (u8, u8)) = move _3; // scope 0 at $DIR/issue-67019.rs:11:10: 11:19
29+
+ (_2.0: (u8, u8)) = (const 1_u8, const 2_u8); // scope 0 at $DIR/issue-67019.rs:11:10: 11:19
30+
+ // ty::Const
31+
+ // + ty: u8
32+
+ // + val: Value(Scalar(0x01))
33+
+ // mir::Constant
34+
+ // + span: $DIR/issue-67019.rs:11:10: 11:19
35+
+ // + literal: Const { ty: u8, val: Value(Scalar(0x01)) }
36+
+ // ty::Const
37+
+ // + ty: u8
38+
+ // + val: Value(Scalar(0x02))
39+
+ // mir::Constant
40+
+ // + span: $DIR/issue-67019.rs:11:10: 11:19
41+
+ // + literal: Const { ty: u8, val: Value(Scalar(0x02)) }
3042
StorageDead(_3); // scope 0 at $DIR/issue-67019.rs:11:18: 11:19
3143
_1 = const test(move _2) -> bb1; // scope 0 at $DIR/issue-67019.rs:11:5: 11:20
3244
// ty::Const

0 commit comments

Comments
 (0)