Auto merge of rust-lang#139639 - compiler-errors:do-not-optimize-switchint-2, r=<try>

[Experiment] Make RemoveRedundantSwitch into a separate pass

r? `@ghost`

Author: bors

compiler/rustc_mir_transform/src/lib.rs

@@ -189,7 +189,8 @@ declare_passes! {
             BeforeConstProp,
             AfterGVN,
             Final
-        };
+        },
+        RemoveRedundantSwitch;
     mod simplify_branches : SimplifyConstCondition {
         AfterConstProp,
         Final
@@ -675,6 +676,8 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
             &check_null::CheckNull,
             // Before inlining: trim down MIR with passes to reduce inlining work.
 
+            // Remove some redundant `SwitchInt` terminators.
+            &simplify::RemoveRedundantSwitch,
             // Has to be done before inlining, otherwise actual call will be almost always inlined.
             // Also simple, so can just do first.
             &lower_slice_len::LowerSliceLenCalls,

compiler/rustc_mir_transform/src/simplify.rs

@@ -26,6 +26,13 @@
 //! Here the block (`{ return; }`) has the return type `char`, rather than `()`, but the MIR we
 //! naively generate still contains the `_a = ()` write in the unreachable block "after" the
 //! return.
+//!
+//! **WARNING**: `SimplifyCfg` is one of the few optimizations that runs on built and analysis
+//! MIR, and so its effects may affect the type-checking, borrow-checking, and other analysis.
+//! We must be extremely careful to only apply optimizations that preserve UB and all
+//! non-determinism, since changes here can affect which programs compile in an insta-stable way.
+//! The normal logic that a program with UB can be changed to do anything does not apply to
+//! pre-"runtime" MIR!
 
 use rustc_index::{Idx, IndexSlice, IndexVec};
 use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor};
@@ -144,7 +151,6 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
                 merged_blocks.clear();
                 while inner_changed {
                     inner_changed = false;
-                    inner_changed |= self.simplify_branch(&mut terminator);
                     inner_changed |= self.merge_successor(&mut merged_blocks, &mut terminator);
                     changed |= inner_changed;
                 }
@@ -251,32 +257,6 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
         true
     }
 
-    // turn a branch with all successors identical to a goto
-    fn simplify_branch(&mut self, terminator: &mut Terminator<'tcx>) -> bool {
-        match terminator.kind {
-            TerminatorKind::SwitchInt { .. } => {}
-            _ => return false,
-        };
-
-        let first_succ = {
-            if let Some(first_succ) = terminator.successors().next() {
-                if terminator.successors().all(|s| s == first_succ) {
-                    let count = terminator.successors().count();
-                    self.pred_count[first_succ] -= (count - 1) as u32;
-                    first_succ
-                } else {
-                    return false;
-                }
-            } else {
-                return false;
-            }
-        };
-
-        debug!("simplifying branch {:?}", terminator);
-        terminator.kind = TerminatorKind::Goto { target: first_succ };
-        true
-    }
-
     fn strip_nops(&mut self) {
         for blk in self.basic_blocks.iter_mut() {
             blk.statements.retain(|stmt| !matches!(stmt.kind, StatementKind::Nop))
@@ -615,3 +595,42 @@ impl<'tcx> MutVisitor<'tcx> for LocalUpdater<'tcx> {
         *l = self.map[*l].unwrap();
     }
 }
+
+pub struct RemoveRedundantSwitch;
+
+impl<'tcx> crate::MirPass<'tcx> for RemoveRedundantSwitch {
+    fn run_pass(&self, _tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
+        loop {
+            let mut should_simplify = false;
+
+            for block in body.basic_blocks_mut() {
+                let TerminatorKind::SwitchInt { discr: _, targets } = &block.terminator().kind
+                else {
+                    continue;
+                };
+                let Some((first_succ, rest)) = targets.all_targets().split_first() else {
+                    continue;
+                };
+                if !rest.iter().all(|succ| succ == first_succ) {
+                    continue;
+                }
+                block.terminator_mut().kind = TerminatorKind::Goto { target: *first_succ };
+                should_simplify = true;
+            }
+
+            if should_simplify {
+                simplify_cfg(body);
+            } else {
+                break;
+            }
+        }
+    }
+
+    fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
+        sess.mir_opt_level() >= 1
+    }
+
+    fn is_required(&self) -> bool {
+        false
+    }
+}

compiler/rustc_mir_transform/src/simplify_comparison_integral.rs

@@ -55,7 +55,9 @@ impl<'tcx> crate::MirPass<'tcx> for SimplifyComparisonIntegral {
             const FALSE: u128 = 0;
 
             let mut new_targets = opt.targets;
-            let first_value = new_targets.iter().next().unwrap().0;
+            let Some((first_value, _)) = new_targets.iter().next() else {
+                continue;
+            };
             let first_is_false_target = first_value == FALSE;
             match opt.op {
                 BinOp::Eq => {

src/tools/miri/tests/fail/read_from_trivial_switch.rs

@@ -0,0 +1,14 @@
+// Ensure that we don't optimize out `SwitchInt` reads even if that terminator
+// branches to the same basic block on every target, since the operand may have
+// side-effects that affect analysis of the MIR.
+//
+// See <https://github.com/rust-lang/miri/issues/4237>.
+
+use std::mem::MaybeUninit;
+
+fn main() {
+    let uninit: MaybeUninit<i32> = MaybeUninit::uninit();
+    let bad_ref: &i32 = unsafe { uninit.assume_init_ref() };
+    let &(0 | _) = bad_ref;
+    //~^ ERROR: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
+}

src/tools/miri/tests/fail/read_from_trivial_switch.stderr

@@ -0,0 +1,15 @@
+error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
+  --> tests/fail/read_from_trivial_switch.rs:LL:CC
+   |
+LL |     let &(0 | _) = bad_ref;
+   |         ^^^^^^^^ using uninitialized data, but this operation requires initialized memory
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `main` at tests/fail/read_from_trivial_switch.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

tests/mir-opt/building/match/exponential_or.match_tuple.SimplifyCfg-initial.after.mir

@@ -24,43 +24,47 @@ fn match_tuple(_1: (u32, bool, Option<i32>, u32)) -> u32 {
 
     bb1: {
         _0 = const 0_u32;
-        goto -> bb10;
+        goto -> bb11;
     }
 
     bb2: {
-        _2 = discriminant((_1.2: std::option::Option<i32>));
-        switchInt(move _2) -> [0: bb4, 1: bb3, otherwise: bb1];
+        switchInt(copy (_1.1: bool)) -> [0: bb3, otherwise: bb3];
     }
 
     bb3: {
-        switchInt(copy (((_1.2: std::option::Option<i32>) as Some).0: i32)) -> [1: bb4, 8: bb4, otherwise: bb1];
+        _2 = discriminant((_1.2: std::option::Option<i32>));
+        switchInt(move _2) -> [0: bb5, 1: bb4, otherwise: bb1];
     }
 
     bb4: {
-        _5 = Le(const 6_u32, copy (_1.3: u32));
-        switchInt(move _5) -> [0: bb5, otherwise: bb7];
+        switchInt(copy (((_1.2: std::option::Option<i32>) as Some).0: i32)) -> [1: bb5, 8: bb5, otherwise: bb1];
     }
 
     bb5: {
-        _3 = Le(const 13_u32, copy (_1.3: u32));
-        switchInt(move _3) -> [0: bb1, otherwise: bb6];
+        _5 = Le(const 6_u32, copy (_1.3: u32));
+        switchInt(move _5) -> [0: bb6, otherwise: bb8];
     }
 
     bb6: {
-        _4 = Le(copy (_1.3: u32), const 16_u32);
-        switchInt(move _4) -> [0: bb1, otherwise: bb8];
+        _3 = Le(const 13_u32, copy (_1.3: u32));
+        switchInt(move _3) -> [0: bb1, otherwise: bb7];
     }
 
     bb7: {
-        _6 = Le(copy (_1.3: u32), const 9_u32);
-        switchInt(move _6) -> [0: bb5, otherwise: bb8];
+        _4 = Le(copy (_1.3: u32), const 16_u32);
+        switchInt(move _4) -> [0: bb1, otherwise: bb9];
     }
 
     bb8: {
-        falseEdge -> [real: bb9, imaginary: bb1];
+        _6 = Le(copy (_1.3: u32), const 9_u32);
+        switchInt(move _6) -> [0: bb6, otherwise: bb9];
     }
 
     bb9: {
+        falseEdge -> [real: bb10, imaginary: bb1];
+    }
+
+    bb10: {
         StorageLive(_7);
         _7 = copy (_1.0: u32);
         StorageLive(_8);
@@ -74,10 +78,10 @@ fn match_tuple(_1: (u32, bool, Option<i32>, u32)) -> u32 {
         StorageDead(_9);
         StorageDead(_8);
         StorageDead(_7);
-        goto -> bb10;
+        goto -> bb11;
     }
 
-    bb10: {
+    bb11: {
         return;
     }
 }

tests/mir-opt/early_otherwise_branch_unwind.poll.EarlyOtherwiseBranch.diff

@@ -12,6 +12,7 @@
       let mut _7: bool;
       let mut _8: bool;
       let mut _9: isize;
+      let mut _10: isize;
       scope 1 {
           debug _trailers => _5;
       }
@@ -99,12 +100,13 @@
       }
 
       bb15: {
-          goto -> bb14;
+          _9 = discriminant(((((_1 as Ready).0: std::result::Result<std::option::Option<std::vec::Vec<u8>>, u8>) as Ok).0: std::option::Option<std::vec::Vec<u8>>));
+          switchInt(move _9) -> [1: bb14, otherwise: bb14];
       }
 
       bb16: {
-          _9 = discriminant(((_1 as Ready).0: std::result::Result<std::option::Option<std::vec::Vec<u8>>, u8>));
-          switchInt(move _9) -> [0: bb13, otherwise: bb12];
+          _10 = discriminant(((_1 as Ready).0: std::result::Result<std::option::Option<std::vec::Vec<u8>>, u8>));
+          switchInt(move _10) -> [0: bb13, otherwise: bb12];
       }
 
       bb17: {
@@ -116,7 +118,7 @@
       }
 
       bb19 (cleanup): {
-          goto -> bb9;
+          switchInt(copy _2) -> [1: bb9, otherwise: bb9];
       }
 
       bb20 (cleanup): {

tests/mir-opt/early_otherwise_branch_unwind.unwind.EarlyOtherwiseBranch.diff

@@ -11,6 +11,7 @@
       let mut _6: bool;
       let mut _7: bool;
       let mut _8: isize;
+      let mut _9: isize;
       scope 1 {
           debug _v => _5;
       }
@@ -92,12 +93,13 @@
       }
 
       bb15: {
-          goto -> bb14;
+          _8 = discriminant(((((_1 as Some).0: std::option::Option<std::option::Option<T>>) as Some).0: std::option::Option<T>));
+          switchInt(move _8) -> [1: bb14, otherwise: bb14];
       }
 
       bb16: {
-          _8 = discriminant(((_1 as Some).0: std::option::Option<std::option::Option<T>>));
-          switchInt(move _8) -> [1: bb13, otherwise: bb12];
+          _9 = discriminant(((_1 as Some).0: std::option::Option<std::option::Option<T>>));
+          switchInt(move _9) -> [1: bb13, otherwise: bb12];
       }
 
       bb17: {
@@ -109,7 +111,7 @@
       }
 
       bb19 (cleanup): {
-          goto -> bb9;
+          switchInt(copy _2) -> [1: bb9, otherwise: bb9];
       }
 
       bb20 (cleanup): {

tests/mir-opt/gvn_copy_aggregate.remove_storage_dead.GVN.diff

@@ -39,6 +39,10 @@
 +         _2 = copy _5;
 +         nop;
           _7 = discriminant(_3);
+          switchInt(move _7) -> bb2;
+      }
+  
+      bb2: {
 -         StorageDead(_3);
 +         nop;
           StorageLive(_6);

tests/mir-opt/multiple_return_terminators.test.MultipleReturnTerminators.diff

@@ -6,6 +6,10 @@
       let mut _0: ();
 
       bb0: {
+          switchInt(copy _1) -> bb1;
+      }
+  
+      bb1: {
           return;
       }
   }