Skip to content

Commit 015a824

Browse files
borsbors
committed
Auto merge of rust-lang#99762 - Nilstrieb:unreachable-prop, r=oli-obk
UnreachableProp: Preserve unreachable branches for multiple targets Before, UnreachablePropagation removed all unreachable branches. This was a pessimization, as it removed information about reachability that was used later in the optimization pipeline. For example, this code ```rust pub enum Two { A, B } pub fn identity(x: Two) -> Two { match x { Two::A => Two::A, Two::B => Two::B, } } ``` basically has `switchInt() -> [0: 0, 1: 1, otherwise: unreachable]` for the match. This allows it to be transformed into a simple `x`. If we remove the unreachable branch, the transformation becomes illegal. This was the problem keeping `UnreachablePropagation` from being enabled, so we can enable it now. Something similar already happened in rust-lang#77800, but it did not show a perf improvement there. Let's try it again anyways! Fixes rust-lang#68105, although that issue has been fixed for a long time (see rust-lang#77680).
2 parents a785176 + 18bfcd3 commit 015a824

File tree

25 files changed

+315
-181
lines changed

25 files changed

+315
-181
lines changed

Diff for: ‎compiler/rustc_mir_transform/src/unreachable_prop.rs

+50-26
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,8 @@ pub struct UnreachablePropagation;
1212

1313
impl MirPass<'_> for UnreachablePropagation {
1414
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
15-
// Enable only under -Zmir-opt-level=4 as in some cases (check the deeply-nested-opt
16-
// perf benchmark) LLVM may spend quite a lot of time optimizing the generated code.
17-
sess.mir_opt_level() >= 4
15+
// Enable only under -Zmir-opt-level=2 as this can make programs less debuggable.
16+
sess.mir_opt_level() >= 2
1817
}
1918

2019
fn run_pass<'tcx>(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
@@ -38,7 +37,19 @@ impl MirPass<'_> for UnreachablePropagation {
3837
}
3938
}
4039

40+
// We do want do keep some unreachable blocks, but make them empty.
41+
for bb in unreachable_blocks {
42+
if !tcx.consider_optimizing(|| {
43+
format!("UnreachablePropagation {:?} ", body.source.def_id())
44+
}) {
45+
break;
46+
}
47+
48+
body.basic_blocks_mut()[bb].statements.clear();
49+
}
50+
4151
let replaced = !replacements.is_empty();
52+
4253
for (bb, terminator_kind) in replacements {
4354
if !tcx.consider_optimizing(|| {
4455
format!("UnreachablePropagation {:?} ", body.source.def_id())
@@ -57,42 +68,55 @@ impl MirPass<'_> for UnreachablePropagation {
5768

5869
fn remove_successors<'tcx, F>(
5970
terminator_kind: &TerminatorKind<'tcx>,
60-
predicate: F,
71+
is_unreachable: F,
6172
) -> Option<TerminatorKind<'tcx>>
6273
where
6374
F: Fn(BasicBlock) -> bool,
6475
{
65-
let terminator = match *terminator_kind {
66-
TerminatorKind::Goto { target } if predicate(target) => TerminatorKind::Unreachable,
67-
TerminatorKind::SwitchInt { ref discr, switch_ty, ref targets } => {
76+
let terminator = match terminator_kind {
77+
// This will unconditionally run into an unreachable and is therefore unreachable as well.
78+
TerminatorKind::Goto { target } if is_unreachable(*target) => TerminatorKind::Unreachable,
79+
TerminatorKind::SwitchInt { targets, discr, switch_ty } => {
6880
let otherwise = targets.otherwise();
6981

70-
let original_targets_len = targets.iter().len() + 1;
71-
let (mut values, mut targets): (Vec<_>, Vec<_>) =
72-
targets.iter().filter(|(_, bb)| !predicate(*bb)).unzip();
82+
// If all targets are unreachable, we can be unreachable as well.
83+
if targets.all_targets().iter().all(|bb| is_unreachable(*bb)) {
84+
TerminatorKind::Unreachable
85+
} else if is_unreachable(otherwise) {
86+
// If there are multiple targets, don't delete unreachable branches (like an unreachable otherwise)
87+
// unless otherwise is unrachable, in which case deleting a normal branch causes it to be merged with
88+
// the otherwise, keeping its unreachable.
89+
// This looses information about reachability causing worse codegen.
90+
// For example (see src/test/codegen/match-optimizes-away.rs)
91+
//
92+
// pub enum Two { A, B }
93+
// pub fn identity(x: Two) -> Two {
94+
// match x {
95+
// Two::A => Two::A,
96+
// Two::B => Two::B,
97+
// }
98+
// }
99+
//
100+
// This generates a `switchInt() -> [0: 0, 1: 1, otherwise: unreachable]`, which allows us or LLVM to
101+
// turn it into just `x` later. Without the unreachable, such a transformation would be illegal.
102+
// If the otherwise branch is unreachable, we can delete all other unreacahble targets, as they will
103+
// still point to the unreachable and therefore not lose reachability information.
104+
let reachable_iter = targets.iter().filter(|(_, bb)| !is_unreachable(*bb));
73105

74-
if !predicate(otherwise) {
75-
targets.push(otherwise);
76-
} else {
77-
values.pop();
78-
}
106+
let new_targets = SwitchTargets::new(reachable_iter, otherwise);
79107

80-
let retained_targets_len = targets.len();
108+
// No unreachable branches were removed.
109+
if new_targets.all_targets().len() == targets.all_targets().len() {
110+
return None;
111+
}
81112

82-
if targets.is_empty() {
83-
TerminatorKind::Unreachable
84-
} else if targets.len() == 1 {
85-
TerminatorKind::Goto { target: targets[0] }
86-
} else if original_targets_len != retained_targets_len {
87113
TerminatorKind::SwitchInt {
88114
discr: discr.clone(),
89-
switch_ty,
90-
targets: SwitchTargets::new(
91-
values.iter().copied().zip(targets.iter().copied()),
92-
*targets.last().unwrap(),
93-
),
115+
switch_ty: *switch_ty,
116+
targets: new_targets,
94117
}
95118
} else {
119+
// If the otherwise branch is reachable, we don't want to delete any unreachable branches.
96120
return None;
97121
}
98122
}

Diff for: ‎src/test/mir-opt/issue_73223.main.SimplifyArmIdentity.32bit.diff

+8-4
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@
5555
((_2 as Some).0: i32) = const 1_i32; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
5656
discriminant(_2) = 1; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
5757
_3 = const 1_isize; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
58-
goto -> bb2; // scope 0 at $DIR/issue-73223.rs:+1:17: +1:30
58+
goto -> bb3; // scope 0 at $DIR/issue-73223.rs:+1:17: +1:30
5959
}
6060

6161
bb1: {
@@ -66,6 +66,10 @@
6666
}
6767

6868
bb2: {
69+
unreachable; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
70+
}
71+
72+
bb3: {
6973
StorageLive(_4); // scope 0 at $DIR/issue-73223.rs:+2:14: +2:15
7074
_4 = ((_2 as Some).0: i32); // scope 0 at $DIR/issue-73223.rs:+2:14: +2:15
7175
_1 = _4; // scope 2 at $DIR/issue-73223.rs:+2:20: +2:21
@@ -108,10 +112,10 @@
108112
StorageDead(_17); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
109113
_15 = Not(move _16); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
110114
StorageDead(_16); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
111-
switchInt(move _15) -> [false: bb4, otherwise: bb3]; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
115+
switchInt(move _15) -> [false: bb5, otherwise: bb4]; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
112116
}
113117

114-
bb3: {
118+
bb4: {
115119
StorageLive(_20); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
116120
Deinit(_20); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
117121
discriminant(_20) = 0; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
@@ -141,7 +145,7 @@
141145
// + literal: Const { ty: core::panicking::AssertKind, val: Value(Scalar(0x00)) }
142146
}
143147

144-
bb4: {
148+
bb5: {
145149
nop; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
146150
StorageDead(_15); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
147151
StorageDead(_14); // scope 3 at $SRC_DIR/core/src/macros/mod.rs:LL:COL

Diff for: ‎src/test/mir-opt/issue_73223.main.SimplifyArmIdentity.64bit.diff

+8-4
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@
5555
((_2 as Some).0: i32) = const 1_i32; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
5656
discriminant(_2) = 1; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
5757
_3 = const 1_isize; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
58-
goto -> bb2; // scope 0 at $DIR/issue-73223.rs:+1:17: +1:30
58+
goto -> bb3; // scope 0 at $DIR/issue-73223.rs:+1:17: +1:30
5959
}
6060

6161
bb1: {
@@ -66,6 +66,10 @@
6666
}
6767

6868
bb2: {
69+
unreachable; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
70+
}
71+
72+
bb3: {
6973
StorageLive(_4); // scope 0 at $DIR/issue-73223.rs:+2:14: +2:15
7074
_4 = ((_2 as Some).0: i32); // scope 0 at $DIR/issue-73223.rs:+2:14: +2:15
7175
_1 = _4; // scope 2 at $DIR/issue-73223.rs:+2:20: +2:21
@@ -108,10 +112,10 @@
108112
StorageDead(_17); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
109113
_15 = Not(move _16); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
110114
StorageDead(_16); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
111-
switchInt(move _15) -> [false: bb4, otherwise: bb3]; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
115+
switchInt(move _15) -> [false: bb5, otherwise: bb4]; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
112116
}
113117

114-
bb3: {
118+
bb4: {
115119
StorageLive(_20); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
116120
Deinit(_20); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
117121
discriminant(_20) = 0; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
@@ -141,7 +145,7 @@
141145
// + literal: Const { ty: core::panicking::AssertKind, val: Value(Scalar(0x00)) }
142146
}
143147

144-
bb4: {
148+
bb5: {
145149
nop; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
146150
StorageDead(_15); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
147151
StorageDead(_14); // scope 3 at $SRC_DIR/core/src/macros/mod.rs:LL:COL

Diff for: ‎src/test/mir-opt/matches_u8.exhaustive_match.MatchBranchSimplification.32bit.diff

+8-4
Original file line numberDiff line numberDiff line change
@@ -8,20 +8,24 @@
88

99
bb0: {
1010
_2 = discriminant(_1); // scope 0 at $DIR/matches_u8.rs:+1:11: +1:12
11-
switchInt(move _2) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/matches_u8.rs:+1:5: +1:12
11+
switchInt(move _2) -> [0_isize: bb3, 1_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/matches_u8.rs:+1:5: +1:12
1212
}
1313

1414
bb1: {
1515
_0 = const 1_u8; // scope 0 at $DIR/matches_u8.rs:+3:17: +3:18
16-
goto -> bb3; // scope 0 at $DIR/matches_u8.rs:+3:17: +3:18
16+
goto -> bb4; // scope 0 at $DIR/matches_u8.rs:+3:17: +3:18
1717
}
1818

1919
bb2: {
20-
_0 = const 0_u8; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
21-
goto -> bb3; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
20+
unreachable; // scope 0 at $DIR/matches_u8.rs:+1:11: +1:12
2221
}
2322

2423
bb3: {
24+
_0 = const 0_u8; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
25+
goto -> bb4; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
26+
}
27+
28+
bb4: {
2529
return; // scope 0 at $DIR/matches_u8.rs:+5:2: +5:2
2630
}
2731
}

Diff for: ‎src/test/mir-opt/matches_u8.exhaustive_match.MatchBranchSimplification.64bit.diff

+8-4
Original file line numberDiff line numberDiff line change
@@ -8,20 +8,24 @@
88

99
bb0: {
1010
_2 = discriminant(_1); // scope 0 at $DIR/matches_u8.rs:+1:11: +1:12
11-
switchInt(move _2) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/matches_u8.rs:+1:5: +1:12
11+
switchInt(move _2) -> [0_isize: bb3, 1_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/matches_u8.rs:+1:5: +1:12
1212
}
1313

1414
bb1: {
1515
_0 = const 1_u8; // scope 0 at $DIR/matches_u8.rs:+3:17: +3:18
16-
goto -> bb3; // scope 0 at $DIR/matches_u8.rs:+3:17: +3:18
16+
goto -> bb4; // scope 0 at $DIR/matches_u8.rs:+3:17: +3:18
1717
}
1818

1919
bb2: {
20-
_0 = const 0_u8; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
21-
goto -> bb3; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
20+
unreachable; // scope 0 at $DIR/matches_u8.rs:+1:11: +1:12
2221
}
2322

2423
bb3: {
24+
_0 = const 0_u8; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
25+
goto -> bb4; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
26+
}
27+
28+
bb4: {
2529
return; // scope 0 at $DIR/matches_u8.rs:+5:2: +5:2
2630
}
2731
}

Diff for: ‎src/test/mir-opt/matches_u8.exhaustive_match_i8.MatchBranchSimplification.32bit.diff

+8-4
Original file line numberDiff line numberDiff line change
@@ -8,20 +8,24 @@
88

99
bb0: {
1010
_2 = discriminant(_1); // scope 0 at $DIR/matches_u8.rs:+1:11: +1:12
11-
switchInt(move _2) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/matches_u8.rs:+1:5: +1:12
11+
switchInt(move _2) -> [0_isize: bb3, 1_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/matches_u8.rs:+1:5: +1:12
1212
}
1313

1414
bb1: {
1515
_0 = const 1_i8; // scope 0 at $DIR/matches_u8.rs:+3:17: +3:18
16-
goto -> bb3; // scope 0 at $DIR/matches_u8.rs:+3:17: +3:18
16+
goto -> bb4; // scope 0 at $DIR/matches_u8.rs:+3:17: +3:18
1717
}
1818

1919
bb2: {
20-
_0 = const 0_i8; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
21-
goto -> bb3; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
20+
unreachable; // scope 0 at $DIR/matches_u8.rs:+1:11: +1:12
2221
}
2322

2423
bb3: {
24+
_0 = const 0_i8; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
25+
goto -> bb4; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
26+
}
27+
28+
bb4: {
2529
return; // scope 0 at $DIR/matches_u8.rs:+5:2: +5:2
2630
}
2731
}

Diff for: ‎src/test/mir-opt/matches_u8.exhaustive_match_i8.MatchBranchSimplification.64bit.diff

+8-4
Original file line numberDiff line numberDiff line change
@@ -8,20 +8,24 @@
88

99
bb0: {
1010
_2 = discriminant(_1); // scope 0 at $DIR/matches_u8.rs:+1:11: +1:12
11-
switchInt(move _2) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/matches_u8.rs:+1:5: +1:12
11+
switchInt(move _2) -> [0_isize: bb3, 1_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/matches_u8.rs:+1:5: +1:12
1212
}
1313

1414
bb1: {
1515
_0 = const 1_i8; // scope 0 at $DIR/matches_u8.rs:+3:17: +3:18
16-
goto -> bb3; // scope 0 at $DIR/matches_u8.rs:+3:17: +3:18
16+
goto -> bb4; // scope 0 at $DIR/matches_u8.rs:+3:17: +3:18
1717
}
1818

1919
bb2: {
20-
_0 = const 0_i8; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
21-
goto -> bb3; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
20+
unreachable; // scope 0 at $DIR/matches_u8.rs:+1:11: +1:12
2221
}
2322

2423
bb3: {
24+
_0 = const 0_i8; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
25+
goto -> bb4; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
26+
}
27+
28+
bb4: {
2529
return; // scope 0 at $DIR/matches_u8.rs:+5:2: +5:2
2630
}
2731
}

Diff for: ‎src/test/mir-opt/separate_const_switch.identity.ConstProp.diff

+12-8
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@
5757
_4 = _1; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:9
5858
StorageLive(_10); // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
5959
_10 = discriminant(_4); // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
60-
switchInt(move _10) -> [0_isize: bb5, 1_isize: bb3, otherwise: bb4]; // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
60+
switchInt(move _10) -> [0_isize: bb6, 1_isize: bb4, otherwise: bb5]; // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
6161
}
6262

6363
bb1: {
@@ -74,6 +74,10 @@
7474
}
7575

7676
bb2: {
77+
unreachable; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
78+
}
79+
80+
bb3: {
7781
StorageLive(_6); // scope 0 at $DIR/separate_const_switch.rs:+1:9: +1:10
7882
_6 = ((_3 as Break).0: std::result::Result<std::convert::Infallible, i32>); // scope 0 at $DIR/separate_const_switch.rs:+1:9: +1:10
7983
StorageLive(_8); // scope 2 at $DIR/separate_const_switch.rs:+1:9: +1:10
@@ -97,7 +101,7 @@
97101
return; // scope 0 at $DIR/separate_const_switch.rs:+2:2: +2:2
98102
}
99103

100-
bb3: {
104+
bb4: {
101105
StorageLive(_13); // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
102106
_13 = move ((_4 as Err).0: i32); // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
103107
StorageLive(_14); // scope 7 at $SRC_DIR/core/src/result.rs:LL:COL
@@ -115,16 +119,16 @@
115119
StorageDead(_10); // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
116120
StorageDead(_4); // scope 0 at $DIR/separate_const_switch.rs:+1:9: +1:10
117121
- _5 = discriminant(_3); // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
118-
- switchInt(move _5) -> [0_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
122+
- switchInt(move _5) -> [0_isize: bb1, 1_isize: bb3, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
119123
+ _5 = const 1_isize; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
120-
+ switchInt(const 1_isize) -> [0_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
124+
+ switchInt(const 1_isize) -> [0_isize: bb1, 1_isize: bb3, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
121125
}
122126

123-
bb4: {
127+
bb5: {
124128
unreachable; // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
125129
}
126130

127-
bb5: {
131+
bb6: {
128132
StorageLive(_11); // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
129133
_11 = move ((_4 as Ok).0: i32); // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
130134
StorageLive(_12); // scope 6 at $SRC_DIR/core/src/result.rs:LL:COL
@@ -137,9 +141,9 @@
137141
StorageDead(_10); // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
138142
StorageDead(_4); // scope 0 at $DIR/separate_const_switch.rs:+1:9: +1:10
139143
- _5 = discriminant(_3); // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
140-
- switchInt(move _5) -> [0_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
144+
- switchInt(move _5) -> [0_isize: bb1, 1_isize: bb3, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
141145
+ _5 = const 0_isize; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
142-
+ switchInt(const 0_isize) -> [0_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
146+
+ switchInt(const 0_isize) -> [0_isize: bb1, 1_isize: bb3, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
143147
}
144148
}
145149

0 commit comments

Comments
 (0)