Rollup merge of #141218 - dianqk:gvn-overlapping, r=oli-obk

gvn: avoid creating overlapping assignments

Quick fix #141038, as I couldn't find a way to avoid in-place modification. I'm considering handling all `ravlue` modifications within the `visit_statement` function.

r? mir-opt

Author: fmease

compiler/rustc_mir_transform/src/gvn.rs

@@ -836,6 +836,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
     #[instrument(level = "trace", skip(self), ret)]
     fn simplify_rvalue(
         &mut self,
+        lhs: &Place<'tcx>,
         rvalue: &mut Rvalue<'tcx>,
         location: Location,
     ) -> Option<VnIndex> {
@@ -855,7 +856,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
                 Value::Repeat(op, amount)
             }
             Rvalue::NullaryOp(op, ty) => Value::NullaryOp(op, ty),
-            Rvalue::Aggregate(..) => return self.simplify_aggregate(rvalue, location),
+            Rvalue::Aggregate(..) => return self.simplify_aggregate(lhs, rvalue, location),
             Rvalue::Ref(_, borrow_kind, ref mut place) => {
                 self.simplify_place_projection(place, location);
                 return Some(self.new_pointer(*place, AddressKind::Ref(borrow_kind)));
@@ -943,6 +944,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
 
     fn simplify_aggregate_to_copy(
         &mut self,
+        lhs: &Place<'tcx>,
         rvalue: &mut Rvalue<'tcx>,
         location: Location,
         fields: &[VnIndex],
@@ -982,19 +984,24 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
 
         // Allow introducing places with non-constant offsets, as those are still better than
         // reconstructing an aggregate.
-        if let Some(place) = self.try_as_place(copy_from_local_value, location, true) {
-            if rvalue.ty(self.local_decls, self.tcx) == place.ty(self.local_decls, self.tcx).ty {
+        if let Some(place) = self.try_as_place(copy_from_local_value, location, true)
+            && rvalue.ty(self.local_decls, self.tcx) == place.ty(self.local_decls, self.tcx).ty
+        {
+            // Avoid creating `*a = copy (*b)`, as they might be aliases resulting in overlapping assignments.
+            // FIXME: This also avoids any kind of projection, not just derefs. We can add allowed projections.
+            if lhs.as_local().is_some() {
                 self.reused_locals.insert(place.local);
                 *rvalue = Rvalue::Use(Operand::Copy(place));
-                return Some(copy_from_local_value);
             }
+            return Some(copy_from_local_value);
         }
 
         None
     }
 
     fn simplify_aggregate(
         &mut self,
+        lhs: &Place<'tcx>,
         rvalue: &mut Rvalue<'tcx>,
         location: Location,
     ) -> Option<VnIndex> {
@@ -1090,7 +1097,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
 
         if let AggregateTy::Def(_, _) = ty
             && let Some(value) =
-                self.simplify_aggregate_to_copy(rvalue, location, &fields, variant_index)
+                self.simplify_aggregate_to_copy(lhs, rvalue, location, &fields, variant_index)
         {
             return Some(value);
         }
@@ -1765,7 +1772,7 @@ impl<'tcx> MutVisitor<'tcx> for VnState<'_, 'tcx> {
         if let StatementKind::Assign(box (ref mut lhs, ref mut rvalue)) = stmt.kind {
             self.simplify_place_projection(lhs, location);
 
-            let value = self.simplify_rvalue(rvalue, location);
+            let value = self.simplify_rvalue(lhs, rvalue, location);
             let value = if let Some(local) = lhs.as_local()
                 && self.ssa.is_ssa(local)
                 // FIXME(#112651) `rvalue` may have a subtype to `local`. We can only mark

tests/mir-opt/gvn_overlapping.overlapping.GVN.diff

@@ -0,0 +1,18 @@
+- // MIR for `overlapping` before GVN
++ // MIR for `overlapping` after GVN
+  
+  fn overlapping(_1: Adt) -> () {
+      let mut _0: ();
+      let mut _2: *mut Adt;
+      let mut _3: u32;
+      let mut _4: &Adt;
+  
+      bb0: {
+          _2 = &raw mut _1;
+          _4 = &(*_2);
+          _3 = copy (((*_4) as variant#1).0: u32);
+          (*_2) = Adt::Some(copy _3);
+          return;
+      }
+  }
+  

tests/mir-opt/gvn_overlapping.rs

@@ -0,0 +1,36 @@
+//@ test-mir-pass: GVN
+
+#![feature(custom_mir, core_intrinsics)]
+
+// Check that we do not create overlapping assignments.
+
+use std::intrinsics::mir::*;
+
+// EMIT_MIR gvn_overlapping.overlapping.GVN.diff
+#[custom_mir(dialect = "runtime")]
+fn overlapping(_17: Adt) {
+    // CHECK-LABEL: fn overlapping(
+    // CHECK: let mut [[PTR:.*]]: *mut Adt;
+    // CHECK: (*[[PTR]]) = Adt::Some(copy {{.*}});
+    mir! {
+        let _33: *mut Adt;
+        let _48: u32;
+        let _73: &Adt;
+        {
+            _33 = core::ptr::addr_of_mut!(_17);
+            _73 = &(*_33);
+            _48 = Field(Variant((*_73), 1), 0);
+            (*_33) = Adt::Some(_48);
+            Return()
+        }
+    }
+}
+
+fn main() {
+    overlapping(Adt::Some(0));
+}
+
+enum Adt {
+    None,
+    Some(u32),
+}