Auto merge of rust-lang#132527 - DianQK:gvn-stmt-iter, r=oli-obk

Invalidate all dereferences when encountering non-local assignments

Fixes rust-lang#132353.

This PR removes the computation value by traversing SSA locals through `for_each_assignment_mut`.

Because the `for_each_assignment_mut` traversal skips statements which have side effects, such as dereference assignments, the computation may be unsound. Instead of `for_each_assignment_mut`, we compute values by traversing in reverse postorder.

Because we compute and use the symbolic representation of values on the fly, I invalidate all old values when encountering a dereference assignment. The current approach does not prevent the optimization of a clone to a copy.

In the future, we may add an alias model, or dominance information for dereference assignments, or SSA form to help GVN.

r? cjgillot

cc `@jieyouxu` rust-lang#132356
cc `@RalfJung` rust-lang#133474

Author: bors

compiler/rustc_data_structures/src/fx.rs

@@ -9,6 +9,8 @@ pub type FxIndexSet<V> = indexmap::IndexSet<V, BuildHasherDefault<FxHasher>>;
 pub type IndexEntry<'a, K, V> = indexmap::map::Entry<'a, K, V>;
 pub type IndexOccupiedEntry<'a, K, V> = indexmap::map::OccupiedEntry<'a, K, V>;
 
+pub use indexmap::set::MutableValues;
+
 #[macro_export]
 macro_rules! define_id_collections {
     ($map_name:ident, $set_name:ident, $entry_name:ident, $key:ty) => {

compiler/rustc_mir_transform/src/gvn.rs

@@ -32,12 +32,6 @@ pub(super) struct SsaLocals {
     borrowed_locals: BitSet<Local>,
 }
 
-pub(super) enum AssignedValue<'a, 'tcx> {
-    Arg,
-    Rvalue(&'a mut Rvalue<'tcx>),
-    Terminator,
-}
-
 impl SsaLocals {
     pub(super) fn new<'tcx>(
         tcx: TyCtxt<'tcx>,
@@ -152,37 +146,6 @@ impl SsaLocals {
         })
     }
 
-    pub(super) fn for_each_assignment_mut<'tcx>(
-        &self,
-        basic_blocks: &mut IndexSlice<BasicBlock, BasicBlockData<'tcx>>,
-        mut f: impl FnMut(Local, AssignedValue<'_, 'tcx>, Location),
-    ) {
-        for &local in &self.assignment_order {
-            match self.assignments[local] {
-                Set1::One(DefLocation::Argument) => f(local, AssignedValue::Arg, Location {
-                    block: START_BLOCK,
-                    statement_index: 0,
-                }),
-                Set1::One(DefLocation::Assignment(loc)) => {
-                    let bb = &mut basic_blocks[loc.block];
-                    // `loc` must point to a direct assignment to `local`.
-                    let stmt = &mut bb.statements[loc.statement_index];
-                    let StatementKind::Assign(box (target, ref mut rvalue)) = stmt.kind else {
-                        bug!()
-                    };
-                    assert_eq!(target.as_local(), Some(local));
-                    f(local, AssignedValue::Rvalue(rvalue), loc)
-                }
-                Set1::One(DefLocation::CallReturn { call, .. }) => {
-                    let bb = &mut basic_blocks[call];
-                    let loc = Location { block: call, statement_index: bb.statements.len() };
-                    f(local, AssignedValue::Terminator, loc)
-                }
-                _ => {}
-            }
-        }
-    }
-
     /// Compute the equivalence classes for locals, based on copy statements.
     ///
     /// The returned vector maps each local to the one it copies. In the following case:

compiler/rustc_mir_transform/src/ssa.rs

@@ -1,6 +1,4 @@
 //@ revisions: DEBUGINFO NODEBUGINFO
-//@ compile-flags: -Zunsound-mir-opts
-// FIXME: see <https://github.com/rust-lang/rust/issues/132353>
 //@ compile-flags: -O -Cno-prepopulate-passes
 //@ [DEBUGINFO] compile-flags: -Cdebuginfo=full
 

tests/codegen/clone_as_copy.rs

@@ -16,12 +16,17 @@ use std::ptr::NonNull;
 #[no_mangle]
 pub fn option_nop_match_32(x: Option<u32>) -> Option<u32> {
     // CHECK: start:
-    // TWENTY-NEXT: %trunc = trunc nuw i32 %0 to i1
-    // TWENTY-NEXT: %.2 = select i1 %trunc, i32 %1, i32 undef
-    // CHECK-NEXT: [[REG1:%.*]] = insertvalue { i32, i32 } poison, i32 %0, 0
-    // NINETEEN-NEXT: [[REG2:%.*]] = insertvalue { i32, i32 } [[REG1]], i32 %1, 1
-    // TWENTY-NEXT: [[REG2:%.*]] = insertvalue { i32, i32 } [[REG1]], i32 %.2, 1
-    // CHECK-NEXT: ret { i32, i32 } [[REG2]]
+    // CHECK-NEXT: [[TRUNC:%.*]] = trunc nuw i32 %0 to i1
+
+    // NINETEEN-NEXT: [[SELECT:%.*]] = select i1 [[TRUNC]], i32 %0, i32 0
+    // NINETEEN-NEXT: [[REG2:%.*]] = insertvalue { i32, i32 } poison, i32 [[SELECT]], 0
+    // NINETEEN-NEXT: [[REG3:%.*]] = insertvalue { i32, i32 } [[REG2]], i32 %1, 1
+
+    // TWENTY-NEXT: [[SELECT:%.*]] = select i1 [[TRUNC]], i32 %1, i32 undef
+    // TWENTY-NEXT: [[REG2:%.*]] = insertvalue { i32, i32 } poison, i32 %0, 0
+    // TWENTY-NEXT: [[REG3:%.*]] = insertvalue { i32, i32 } [[REG2]], i32 [[SELECT]], 1
+
+    // CHECK-NEXT: ret { i32, i32 } [[REG3]]
     match x {
         Some(x) => Some(x),
         None => None,

tests/codegen/try_question_mark_nop.rs

@@ -8,8 +8,9 @@
 
       bb0: {
           StorageLive(_1);
-          _1 = const <bool as NeedsDrop>::NEEDS;
+-         _1 = const <bool as NeedsDrop>::NEEDS;
 -         switchInt(move _1) -> [0: bb2, otherwise: bb1];
++         _1 = const false;
 +         switchInt(const false) -> [0: bb2, otherwise: bb1];
       }
 

tests/coverage/issue-84561.cov-map

@@ -8,8 +8,9 @@
 
       bb0: {
           StorageLive(_1);
-          _1 = const <bool as NeedsDrop>::NEEDS;
+-         _1 = const <bool as NeedsDrop>::NEEDS;
 -         switchInt(move _1) -> [0: bb2, otherwise: bb1];
++         _1 = const false;
 +         switchInt(const false) -> [0: bb2, otherwise: bb1];
       }
 

tests/mir-opt/const_prop/control_flow_simplification.hello.GVN.panic-abort.diff

@@ -14,19 +14,23 @@
 
       bb0: {
           StorageLive(_1);
-          StorageLive(_2);
+-         StorageLive(_2);
 -         StorageLive(_3);
++         nop;
 +         nop;
           _3 = const {ALLOC0: &u8};
-          _2 = copy (*_3);
+-         _2 = copy (*_3);
++         _2 = const 2_u8;
           StorageLive(_4);
           StorageLive(_5);
           _5 = const {ALLOC0: &u8};
 -         _4 = copy (*_5);
-+         _4 = copy (*_3);
-          _1 = Add(move _2, move _4);
+-         _1 = Add(move _2, move _4);
++         _4 = const 2_u8;
++         _1 = const 4_u8;
           StorageDead(_4);
-          StorageDead(_2);
+-         StorageDead(_2);
++         nop;
           StorageDead(_5);
 -         StorageDead(_3);
 +         nop;

tests/mir-opt/const_prop/control_flow_simplification.hello.GVN.panic-unwind.diff

@@ -6,7 +6,6 @@ static FOO: u8 = 2;
 fn main() {
     // CHECK-LABEL: fn main(
     // CHECK: debug x => [[x:_.*]];
-    // Disabled due to <https://github.com/rust-lang/rust/issues/130853>
-    // COM: CHECK: [[x]] = const 4_u8;
+    // CHECK: [[x]] = const 4_u8;
     let x = FOO + FOO;
 }