Auto merge of #146289 - cjgillot:gvn-aggregate, r=dianqk

GVN: Allow reusing aggregates if LHS is not a simple local.

This resolves a FIXME in the code. I don't see a reason not to allow this.

Author: bors

compiler/rustc_mir_transform/src/gvn.rs

@@ -896,18 +896,13 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
 
     fn simplify_aggregate_to_copy(
         &mut self,
-        lhs: &Place<'tcx>,
-        rvalue: &mut Rvalue<'tcx>,
-        location: Location,
-        fields: &[VnIndex],
+        ty: Ty<'tcx>,
         variant_index: VariantIdx,
+        fields: &[VnIndex],
     ) -> Option<VnIndex> {
-        let Some(&first_field) = fields.first() else {
-            return None;
-        };
-        let Value::Projection(copy_from_value, _) = *self.get(first_field) else {
-            return None;
-        };
+        let Some(&first_field) = fields.first() else { return None };
+        let Value::Projection(copy_from_value, _) = *self.get(first_field) else { return None };
+
         // All fields must correspond one-to-one and come from the same aggregate value.
         if fields.iter().enumerate().any(|(index, &v)| {
             if let Value::Projection(pointer, ProjectionElem::Field(from_index, _)) = *self.get(v)
@@ -934,21 +929,8 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
             }
         }
 
-        // Allow introducing places with non-constant offsets, as those are still better than
-        // reconstructing an aggregate.
-        if self.ty(copy_from_local_value) == rvalue.ty(self.local_decls, self.tcx)
-            && let Some(place) = self.try_as_place(copy_from_local_value, location, true)
-        {
-            // 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);
-        }
-
-        None
+        // Both must be variants of the same type.
+        if self.ty(copy_from_local_value) == ty { Some(copy_from_local_value) } else { None }
     }
 
     fn simplify_aggregate(
@@ -1035,9 +1017,16 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
             return Some(self.insert(ty, Value::Repeat(first, len)));
         }
 
-        if let Some(value) =
-            self.simplify_aggregate_to_copy(lhs, rvalue, location, &fields, variant_index)
-        {
+        if let Some(value) = self.simplify_aggregate_to_copy(ty, variant_index, &fields) {
+            // Allow introducing places with non-constant offsets, as those are still better than
+            // reconstructing an aggregate. But avoid creating `*a = copy (*b)`, as they might be
+            // aliases resulting in overlapping assignments.
+            let allow_complex_projection =
+                lhs.projection[..].iter().all(PlaceElem::is_stable_offset);
+            if let Some(place) = self.try_as_place(value, location, allow_complex_projection) {
+                self.reused_locals.insert(place.local);
+                *rvalue = Rvalue::Use(Operand::Copy(place));
+            }
             return Some(value);
         }
 

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

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

tests/mir-opt/gvn_overlapping.rs

@@ -2,11 +2,10 @@
 
 #![feature(custom_mir, core_intrinsics)]
 
-// Check that we do not create overlapping assignments.
-
 use std::intrinsics::mir::*;
 
 // EMIT_MIR gvn_overlapping.overlapping.GVN.diff
+/// Check that we do not create overlapping assignments.
 #[custom_mir(dialect = "runtime")]
 fn overlapping(_17: Adt) {
     // CHECK-LABEL: fn overlapping(
@@ -26,6 +25,45 @@ fn overlapping(_17: Adt) {
     }
 }
 
+// EMIT_MIR gvn_overlapping.stable_projection.GVN.diff
+/// Check that we allow dereferences in the RHS if the LHS is a stable projection.
+#[custom_mir(dialect = "runtime")]
+fn stable_projection(_1: (Adt,)) {
+    // CHECK-LABEL: fn stable_projection(
+    // CHECK: let mut _2: *mut Adt;
+    // CHECK: let mut _4: &Adt;
+    // CHECK: (_1.0: Adt) = copy (*_4);
+    mir! {
+        let _2: *mut Adt;
+        let _3: u32;
+        let _4: &Adt;
+        {
+            _2 = core::ptr::addr_of_mut!(_1.0);
+            _4 = &(*_2);
+            _3 = Field(Variant((*_4), 1), 0);
+            _1.0 = Adt::Some(_3);
+            Return()
+        }
+    }
+}
+
+// EMIT_MIR gvn_overlapping.fields.GVN.diff
+/// Check that we do not create assignments between different fields of the same local.
+#[custom_mir(dialect = "runtime")]
+fn fields(_1: (Adt, Adt)) {
+    // CHECK-LABEL: fn fields(
+    // CHECK: _2 = copy (((_1.0: Adt) as variant#1).0: u32);
+    // CHECK-NEXT: (_1.1: Adt) = Adt::Some(copy _2);
+    mir! {
+        let _2: u32;
+        {
+            _2 = Field(Variant(_1.0, 1), 0);
+            _1.1 = Adt::Some(_2);
+            Return()
+        }
+    }
+}
+
 fn main() {
     overlapping(Adt::Some(0));
 }

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

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