From eafab660964bcd1e2c8b19cc2021cc5c7fee6c38 Mon Sep 17 00:00:00 2001
From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com>
Date: Tue, 26 Jul 2022 15:11:12 +0200
Subject: [PATCH] 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.
---
 .../src/unreachable_prop.rs                   | 75 ++++++++++++-------
 1 file changed, 50 insertions(+), 25 deletions(-)

diff --git a/compiler/rustc_mir_transform/src/unreachable_prop.rs b/compiler/rustc_mir_transform/src/unreachable_prop.rs
index f916ca36217..a7e522beeab 100644
--- a/compiler/rustc_mir_transform/src/unreachable_prop.rs
+++ b/compiler/rustc_mir_transform/src/unreachable_prop.rs
@@ -38,7 +38,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())
@@ -57,42 +69,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 !predicate(otherwise) {
-                targets.push(otherwise);
-            } else {
-                values.pop();
-            }
-
-            let retained_targets_len = targets.len();
-
-            if targets.is_empty() {
+            // 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 targets.len() == 1 {
-                TerminatorKind::Goto { target: targets[0] }
-            } else if original_targets_len != retained_targets_len {
+            } 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));
+
+                let new_targets = SwitchTargets::new(reachable_iter, otherwise);
+
+                // No unreachable branches were removed.
+                if new_targets.all_targets().len() == targets.all_targets().len() {
+                    return None;
+                }
+
                 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;
             }
         }