Skip to content

Commit

Permalink
Auto merge of #99762 - Nilstrieb:unreachable-prop, r=oli-obk
Browse files Browse the repository at this point in the history 
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 #77800, but it did not show a perf improvement there. Let's try it again anyways!

Fixes #68105, although that issue has been fixed for a long time (see #77680).
  • Loading branch information
@bors
bors committed Aug 22, 2022
2 parents a785176 + 18bfcd3 commit 015a824
Show file tree
Hide file tree
Showing 25 changed files with 315 additions and 181 deletions.
76 changes: 50 additions & 26 deletions compiler/rustc_mir_transform/src/unreachable_prop.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@ pub struct UnreachablePropagation;

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

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

// We do want do keep some unreachable blocks, but make them empty.
for bb in unreachable_blocks {
if !tcx.consider_optimizing(|| {
format!("UnreachablePropagation {:?} ", body.source.def_id())
}) {
break;
}

body.basic_blocks_mut()[bb].statements.clear();
}

let replaced = !replacements.is_empty();

for (bb, terminator_kind) in replacements {
if !tcx.consider_optimizing(|| {
format!("UnreachablePropagation {:?} ", body.source.def_id())
Expand All @@ -57,42 +68,55 @@ impl MirPass<'_> for UnreachablePropagation {

fn remove_successors<'tcx, F>(
terminator_kind: &TerminatorKind<'tcx>,
predicate: F,
is_unreachable: F,
) -> Option<TerminatorKind<'tcx>>
where
F: Fn(BasicBlock) -> bool,
{
let terminator = match *terminator_kind {
TerminatorKind::Goto { target } if predicate(target) => TerminatorKind::Unreachable,
TerminatorKind::SwitchInt { ref discr, switch_ty, ref targets } => {
let terminator = match terminator_kind {
// This will unconditionally run into an unreachable and is therefore unreachable as well.
TerminatorKind::Goto { target } if is_unreachable(*target) => TerminatorKind::Unreachable,
TerminatorKind::SwitchInt { targets, discr, switch_ty } => {
let otherwise = targets.otherwise();

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

if !predicate(otherwise) {
targets.push(otherwise);
} else {
values.pop();
}
let new_targets = SwitchTargets::new(reachable_iter, otherwise);

let retained_targets_len = targets.len();
// No unreachable branches were removed.
if new_targets.all_targets().len() == targets.all_targets().len() {
return None;
}

if targets.is_empty() {
TerminatorKind::Unreachable
} else if targets.len() == 1 {
TerminatorKind::Goto { target: targets[0] }
} else if original_targets_len != retained_targets_len {
TerminatorKind::SwitchInt {
discr: discr.clone(),
switch_ty,
targets: SwitchTargets::new(
values.iter().copied().zip(targets.iter().copied()),
*targets.last().unwrap(),
),
switch_ty: *switch_ty,
targets: new_targets,
}
} else {
// If the otherwise branch is reachable, we don't want to delete any unreachable branches.
return None;
}
}
Expand Down
12 changes: 8 additions & 4 deletions src/test/mir-opt/issue_73223.main.SimplifyArmIdentity.32bit.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
((_2 as Some).0: i32) = const 1_i32; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
discriminant(_2) = 1; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
_3 = const 1_isize; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
goto -> bb2; // scope 0 at $DIR/issue-73223.rs:+1:17: +1:30
goto -> bb3; // scope 0 at $DIR/issue-73223.rs:+1:17: +1:30
}

bb1: {
Expand All @@ -66,6 +66,10 @@
}

bb2: {
unreachable; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
}

bb3: {
StorageLive(_4); // scope 0 at $DIR/issue-73223.rs:+2:14: +2:15
_4 = ((_2 as Some).0: i32); // scope 0 at $DIR/issue-73223.rs:+2:14: +2:15
_1 = _4; // scope 2 at $DIR/issue-73223.rs:+2:20: +2:21
Expand Down Expand Up @@ -108,10 +112,10 @@
StorageDead(_17); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
_15 = Not(move _16); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
StorageDead(_16); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
switchInt(move _15) -> [false: bb4, otherwise: bb3]; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
switchInt(move _15) -> [false: bb5, otherwise: bb4]; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
}

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

bb4: {
bb5: {
nop; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
StorageDead(_15); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
StorageDead(_14); // scope 3 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
Expand Down
12 changes: 8 additions & 4 deletions src/test/mir-opt/issue_73223.main.SimplifyArmIdentity.64bit.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
((_2 as Some).0: i32) = const 1_i32; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
discriminant(_2) = 1; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
_3 = const 1_isize; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
goto -> bb2; // scope 0 at $DIR/issue-73223.rs:+1:17: +1:30
goto -> bb3; // scope 0 at $DIR/issue-73223.rs:+1:17: +1:30
}

bb1: {
Expand All @@ -66,6 +66,10 @@
}

bb2: {
unreachable; // scope 0 at $DIR/issue-73223.rs:+1:23: +1:30
}

bb3: {
StorageLive(_4); // scope 0 at $DIR/issue-73223.rs:+2:14: +2:15
_4 = ((_2 as Some).0: i32); // scope 0 at $DIR/issue-73223.rs:+2:14: +2:15
_1 = _4; // scope 2 at $DIR/issue-73223.rs:+2:20: +2:21
Expand Down Expand Up @@ -108,10 +112,10 @@
StorageDead(_17); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
_15 = Not(move _16); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
StorageDead(_16); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
switchInt(move _15) -> [false: bb4, otherwise: bb3]; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
switchInt(move _15) -> [false: bb5, otherwise: bb4]; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
}

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

bb4: {
bb5: {
nop; // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
StorageDead(_15); // scope 4 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
StorageDead(_14); // scope 3 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
Expand Down
12 changes: 8 additions & 4 deletions src/test/mir-opt/matches_u8.exhaustive_match.MatchBranchSimplification.32bit.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,24 @@

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

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

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

bb3: {
_0 = const 0_u8; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
goto -> bb4; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
}

bb4: {
return; // scope 0 at $DIR/matches_u8.rs:+5:2: +5:2
}
}
Expand Down
12 changes: 8 additions & 4 deletions src/test/mir-opt/matches_u8.exhaustive_match.MatchBranchSimplification.64bit.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,24 @@

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

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

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

bb3: {
_0 = const 0_u8; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
goto -> bb4; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
}

bb4: {
return; // scope 0 at $DIR/matches_u8.rs:+5:2: +5:2
}
}
Expand Down
12 changes: 8 additions & 4 deletions src/test/mir-opt/matches_u8.exhaustive_match_i8.MatchBranchSimplification.32bit.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,24 @@

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

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

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

bb3: {
_0 = const 0_i8; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
goto -> bb4; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
}

bb4: {
return; // scope 0 at $DIR/matches_u8.rs:+5:2: +5:2
}
}
Expand Down
12 changes: 8 additions & 4 deletions src/test/mir-opt/matches_u8.exhaustive_match_i8.MatchBranchSimplification.64bit.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,24 @@

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

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

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

bb3: {
_0 = const 0_i8; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
goto -> bb4; // scope 0 at $DIR/matches_u8.rs:+2:17: +2:18
}

bb4: {
return; // scope 0 at $DIR/matches_u8.rs:+5:2: +5:2
}
}
Expand Down
20 changes: 12 additions & 8 deletions src/test/mir-opt/separate_const_switch.identity.ConstProp.diff
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
_4 = _1; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:9
StorageLive(_10); // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
_10 = discriminant(_4); // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
switchInt(move _10) -> [0_isize: bb5, 1_isize: bb3, otherwise: bb4]; // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
switchInt(move _10) -> [0_isize: bb6, 1_isize: bb4, otherwise: bb5]; // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
}

bb1: {
Expand All @@ -74,6 +74,10 @@
}

bb2: {
unreachable; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
}

bb3: {
StorageLive(_6); // scope 0 at $DIR/separate_const_switch.rs:+1:9: +1:10
_6 = ((_3 as Break).0: std::result::Result<std::convert::Infallible, i32>); // scope 0 at $DIR/separate_const_switch.rs:+1:9: +1:10
StorageLive(_8); // scope 2 at $DIR/separate_const_switch.rs:+1:9: +1:10
Expand All @@ -97,7 +101,7 @@
return; // scope 0 at $DIR/separate_const_switch.rs:+2:2: +2:2
}

bb3: {
bb4: {
StorageLive(_13); // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
_13 = move ((_4 as Err).0: i32); // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
StorageLive(_14); // scope 7 at $SRC_DIR/core/src/result.rs:LL:COL
Expand All @@ -115,16 +119,16 @@
StorageDead(_10); // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
StorageDead(_4); // scope 0 at $DIR/separate_const_switch.rs:+1:9: +1:10
- _5 = discriminant(_3); // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
- switchInt(move _5) -> [0_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
- switchInt(move _5) -> [0_isize: bb1, 1_isize: bb3, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
+ _5 = const 1_isize; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
+ switchInt(const 1_isize) -> [0_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
+ switchInt(const 1_isize) -> [0_isize: bb1, 1_isize: bb3, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
}

bb4: {
bb5: {
unreachable; // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
}

bb5: {
bb6: {
StorageLive(_11); // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
_11 = move ((_4 as Ok).0: i32); // scope 5 at $SRC_DIR/core/src/result.rs:LL:COL
StorageLive(_12); // scope 6 at $SRC_DIR/core/src/result.rs:LL:COL
Expand All @@ -137,9 +141,9 @@
StorageDead(_10); // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
StorageDead(_4); // scope 0 at $DIR/separate_const_switch.rs:+1:9: +1:10
- _5 = discriminant(_3); // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
- switchInt(move _5) -> [0_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
- switchInt(move _5) -> [0_isize: bb1, 1_isize: bb3, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
+ _5 = const 0_isize; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
+ switchInt(const 0_isize) -> [0_isize: bb1, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
+ switchInt(const 0_isize) -> [0_isize: bb1, 1_isize: bb3, otherwise: bb2]; // scope 0 at $DIR/separate_const_switch.rs:+1:8: +1:10
}
}

Loading

0 comments on commit 015a824

Please sign in to comment.