GVN: Do not introduce new dereferences or unify dereferences if they borrow from non-SSA locals

Author: dianqk

compiler/rustc_mir_transform/src/gvn.rs

@@ -135,7 +135,7 @@ impl<'tcx> crate::MirPass<'tcx> for GVN {
             VnState::new(tcx, body, typing_env, &ssa, dominators, &body.local_decls, &arena);
 
         for local in body.args_iter().filter(|&local| ssa.is_ssa(local)) {
-            let opaque = state.new_opaque(body.local_decls[local].ty);
+            let opaque = state.new_argument(body.local_decls[local].ty);
             state.assign(local, opaque);
         }
 
@@ -198,8 +198,9 @@ enum AddressBase {
 enum Value<'a, 'tcx> {
     // Root values.
     /// Used to represent values we know nothing about.
-    /// The `usize` is a counter incremented by `new_opaque`.
     Opaque(VnOpaque),
+    /// The value is a argument.
+    Argument(VnOpaque),
     /// Evaluated or unevaluated constant value.
     Constant {
         value: Const<'tcx>,
@@ -284,7 +285,7 @@ impl<'a, 'tcx> ValueSet<'a, 'tcx> {
         let value = value(VnOpaque);
 
         debug_assert!(match value {
-            Value::Opaque(_) | Value::Address { .. } => true,
+            Value::Opaque(_) | Value::Argument(_) | Value::Address { .. } => true,
             Value::Constant { disambiguator, .. } => disambiguator.is_some(),
             _ => false,
         });
@@ -440,6 +441,13 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
         index
     }
 
+    #[instrument(level = "trace", skip(self), ret)]
+    fn new_argument(&mut self, ty: Ty<'tcx>) -> VnIndex {
+        let index = self.insert_unique(ty, Value::Argument);
+        self.evaluated[index] = Some(None);
+        index
+    }
+
     /// Create a new `Value::Address` distinct from all the others.
     #[instrument(level = "trace", skip(self), ret)]
     fn new_pointer(&mut self, place: Place<'tcx>, kind: AddressKind) -> Option<VnIndex> {
@@ -458,6 +466,10 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
             projection.next();
             AddressBase::Deref(base)
         } else {
+            // Only propagate the pointer of the SSA local.
+            if !self.ssa.is_ssa(place.local) {
+                return None;
+            }
             AddressBase::Local(place.local)
         };
         // Do not try evaluating inside `Index`, this has been done by `simplify_place_projection`.
@@ -526,10 +538,6 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
         self.insert(ty, Value::Aggregate(VariantIdx::ZERO, self.arena.alloc_slice(values)))
     }
 
-    fn insert_deref(&mut self, ty: Ty<'tcx>, value: VnIndex) -> VnIndex {
-        self.insert(ty, Value::Projection(value, ProjectionElem::Deref))
-    }
-
     #[instrument(level = "trace", skip(self), ret)]
     fn eval_to_const_inner(&mut self, value: VnIndex) -> Option<OpTy<'tcx>> {
         use Value::*;
@@ -543,7 +551,7 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
         let op = match self.get(value) {
             _ if ty.is_zst() => ImmTy::uninit(ty).into(),
 
-            Opaque(_) => return None,
+            Opaque(_) | Argument(_) => return None,
             // Keep runtime check constants as symbolic.
             RuntimeChecks(..) => return None,
 
@@ -795,7 +803,13 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
 
                     // An immutable borrow `_x` always points to the same value for the
                     // lifetime of the borrow, so we can merge all instances of `*_x`.
-                    return Some((projection_ty, self.insert_deref(projection_ty.ty, value)));
+                    return Some((
+                        projection_ty,
+                        self.insert(
+                            projection_ty.ty,
+                            Value::Projection(value, ProjectionElem::Deref),
+                        ),
+                    ));
                 } else {
                     return None;
                 }
@@ -1119,6 +1133,42 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
             }
         }
 
+        // We can introduce a new dereference if the source value cannot be changed in the body.
+        let mut copy_root = copy_from_local_value;
+        loop {
+            match self.get(copy_root) {
+                Value::Projection(base, _) => {
+                    copy_root = base;
+                }
+                Value::Address {
+                    base,
+                    projection,
+                    kind: AddressKind::Ref(BorrowKind::Shared),
+                    ..
+                } if projection.iter().all(ProjectionElem::is_stable_offset) => match base {
+                    AddressBase::Local(local) => {
+                        break;
+                    }
+                    AddressBase::Deref(index) => {
+                        copy_root = index;
+                    }
+                },
+                Value::Argument(_) if !self.ty(copy_root).is_mutable_ptr() => {
+                    break;
+                }
+                Value::Opaque(_) => {
+                    let ty = self.ty(copy_root);
+                    if ty.is_fn() || !ty.is_any_ptr() {
+                        break;
+                    }
+                    return None;
+                }
+                _ => {
+                    return None;
+                }
+            }
+        }
+
         // Both must be variants of the same type.
         if self.ty(copy_from_local_value) == ty { Some(copy_from_local_value) } else { None }
     }
@@ -1208,12 +1258,7 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
         }
 
         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) {
+            if let Some(place) = self.try_as_place(value, location, true) {
                 self.reused_locals.insert(place.local);
                 *rvalue = Rvalue::Use(Operand::Copy(place));
             }
@@ -1866,7 +1911,7 @@ impl<'tcx> MutVisitor<'tcx> for VnState<'_, '_, 'tcx> {
     ) {
         self.simplify_place_projection(lhs, location);
 
-        let value = self.simplify_rvalue(lhs, rvalue, location);
+        let mut value = self.simplify_rvalue(lhs, rvalue, location);
         if let Some(value) = value {
             if let Some(const_) = self.try_as_constant(value) {
                 *rvalue = Rvalue::Use(Operand::Constant(Box::new(const_)));
@@ -1879,9 +1924,34 @@ impl<'tcx> MutVisitor<'tcx> for VnState<'_, '_, 'tcx> {
             }
         }
 
+        let rvalue_ty = rvalue.ty(self.local_decls, self.tcx);
+        // DO NOT reason the pointer value if it may point to a non-SSA local.
+        // For instance, we cannot unify two pointers that dereference same local, because they may
+        // have different lifetimes.
+        if rvalue_ty.is_ref()
+            && let Some(index) = value
+        {
+            match self.get(index) {
+                Value::Opaque(_) | Value::Projection(..) => {
+                    value = None;
+                }
+                Value::Constant { .. }
+                | Value::Address { .. }
+                | Value::Argument(_)
+                | Value::RawPtr { .. }
+                | Value::BinaryOp(..)
+                | Value::Cast { .. } => {}
+                Value::Aggregate(..)
+                | Value::Union(..)
+                | Value::Repeat(..)
+                | Value::Discriminant(..)
+                | Value::RuntimeChecks(..)
+                | Value::UnaryOp(..) => unreachable!(),
+            }
+        }
+
         if let Some(local) = lhs.as_local()
             && self.ssa.is_ssa(local)
-            && let rvalue_ty = rvalue.ty(self.local_decls, self.tcx)
             // FIXME(#112651) `rvalue` may have a subtype to `local`. We can only mark
             // `local` as reusable if we have an exact type match.
             && self.local_decls[local].ty == rvalue_ty

tests/mir-opt/gvn.dereference_reborrow.GVN.panic-abort.diff

@@ -0,0 +1,37 @@
+- // MIR for `dereference_reborrow` before GVN
++ // MIR for `dereference_reborrow` after GVN
+  
+  fn dereference_reborrow(_1: &mut u8) -> () {
+      debug mut_a => _1;
+      let mut _0: ();
+      let _2: &u8;
+      scope 1 {
+          debug a => _2;
+          let _3: u8;
+          scope 2 {
+              debug b => _3;
+              let _4: u8;
+              scope 3 {
+                  debug c => _4;
+              }
+          }
+      }
+  
+      bb0: {
+          StorageLive(_2);
+          _2 = &(*_1);
+-         StorageLive(_3);
++         nop;
+          _3 = copy (*_2);
+          StorageLive(_4);
+-         _4 = copy (*_2);
++         _4 = copy _3;
+          _0 = const ();
+          StorageDead(_4);
+-         StorageDead(_3);
++         nop;
+          StorageDead(_2);
+          return;
+      }
+  }
+  

tests/mir-opt/gvn.dereference_reborrow.GVN.panic-unwind.diff

@@ -0,0 +1,37 @@
+- // MIR for `dereference_reborrow` before GVN
++ // MIR for `dereference_reborrow` after GVN
+  
+  fn dereference_reborrow(_1: &mut u8) -> () {
+      debug mut_a => _1;
+      let mut _0: ();
+      let _2: &u8;
+      scope 1 {
+          debug a => _2;
+          let _3: u8;
+          scope 2 {
+              debug b => _3;
+              let _4: u8;
+              scope 3 {
+                  debug c => _4;
+              }
+          }
+      }
+  
+      bb0: {
+          StorageLive(_2);
+          _2 = &(*_1);
+-         StorageLive(_3);
++         nop;
+          _3 = copy (*_2);
+          StorageLive(_4);
+-         _4 = copy (*_2);
++         _4 = copy _3;
+          _0 = const ();
+          StorageDead(_4);
+-         StorageDead(_3);
++         nop;
+          StorageDead(_2);
+          return;
+      }
+  }
+  

tests/mir-opt/gvn.dereferences.GVN.panic-abort.diff

@@ -107,18 +107,23 @@
           StorageLive(_18);
           _18 = &(*_1);
           StorageLive(_19);
-          StorageLive(_20);
+-         StorageLive(_20);
++         nop;
           _20 = copy (*_18);
-          _19 = opaque::<u32>(move _20) -> [return: bb7, unwind unreachable];
+-         _19 = opaque::<u32>(move _20) -> [return: bb7, unwind unreachable];
++         _19 = opaque::<u32>(copy _20) -> [return: bb7, unwind unreachable];
       }
 
       bb7: {
-          StorageDead(_20);
+-         StorageDead(_20);
++         nop;
           StorageDead(_19);
           StorageLive(_21);
           StorageLive(_22);
-          _22 = copy (*_18);
-          _21 = opaque::<u32>(move _22) -> [return: bb8, unwind unreachable];
+-         _22 = copy (*_18);
+-         _21 = opaque::<u32>(move _22) -> [return: bb8, unwind unreachable];
++         _22 = copy _20;
++         _21 = opaque::<u32>(copy _20) -> [return: bb8, unwind unreachable];
       }
 
       bb8: {
@@ -152,18 +157,23 @@
           StorageDead(_28);
           StorageDead(_27);
           StorageLive(_29);
-          StorageLive(_30);
+-         StorageLive(_30);
++         nop;
           _30 = copy ((*_3).0: u32);
-          _29 = opaque::<u32>(move _30) -> [return: bb12, unwind unreachable];
+-         _29 = opaque::<u32>(move _30) -> [return: bb12, unwind unreachable];
++         _29 = opaque::<u32>(copy _30) -> [return: bb12, unwind unreachable];
       }
 
       bb12: {
-          StorageDead(_30);
+-         StorageDead(_30);
++         nop;
           StorageDead(_29);
           StorageLive(_31);
           StorageLive(_32);
-          _32 = copy ((*_3).0: u32);
-          _31 = opaque::<u32>(move _32) -> [return: bb13, unwind unreachable];
+-         _32 = copy ((*_3).0: u32);
+-         _31 = opaque::<u32>(move _32) -> [return: bb13, unwind unreachable];
++         _32 = copy _30;
++         _31 = opaque::<u32>(copy _30) -> [return: bb13, unwind unreachable];
       }
 
       bb13: {

tests/mir-opt/gvn.dereferences.GVN.panic-unwind.diff

@@ -107,18 +107,23 @@
           StorageLive(_18);
           _18 = &(*_1);
           StorageLive(_19);
-          StorageLive(_20);
+-         StorageLive(_20);
++         nop;
           _20 = copy (*_18);
-          _19 = opaque::<u32>(move _20) -> [return: bb7, unwind continue];
+-         _19 = opaque::<u32>(move _20) -> [return: bb7, unwind continue];
++         _19 = opaque::<u32>(copy _20) -> [return: bb7, unwind continue];
       }
 
       bb7: {
-          StorageDead(_20);
+-         StorageDead(_20);
++         nop;
           StorageDead(_19);
           StorageLive(_21);
           StorageLive(_22);
-          _22 = copy (*_18);
-          _21 = opaque::<u32>(move _22) -> [return: bb8, unwind continue];
+-         _22 = copy (*_18);
+-         _21 = opaque::<u32>(move _22) -> [return: bb8, unwind continue];
++         _22 = copy _20;
++         _21 = opaque::<u32>(copy _20) -> [return: bb8, unwind continue];
       }
 
       bb8: {
@@ -152,18 +157,23 @@
           StorageDead(_28);
           StorageDead(_27);
           StorageLive(_29);
-          StorageLive(_30);
+-         StorageLive(_30);
++         nop;
           _30 = copy ((*_3).0: u32);
-          _29 = opaque::<u32>(move _30) -> [return: bb12, unwind continue];
+-         _29 = opaque::<u32>(move _30) -> [return: bb12, unwind continue];
++         _29 = opaque::<u32>(copy _30) -> [return: bb12, unwind continue];
       }
 
       bb12: {
-          StorageDead(_30);
+-         StorageDead(_30);
++         nop;
           StorageDead(_29);
           StorageLive(_31);
           StorageLive(_32);
-          _32 = copy ((*_3).0: u32);
-          _31 = opaque::<u32>(move _32) -> [return: bb13, unwind continue];
+-         _32 = copy ((*_3).0: u32);
+-         _31 = opaque::<u32>(move _32) -> [return: bb13, unwind continue];
++         _32 = copy _30;
++         _31 = opaque::<u32>(copy _30) -> [return: bb13, unwind continue];
       }
 
       bb13: {