GVN: consider constants of primitive types as deterministic

compiler/rustc_middle/src/mir/consts.rs

@@ -181,26 +181,6 @@ impl ConstValue {
         Some(data.inner().inspect_with_uninit_and_ptr_outside_interpreter(start..end))
     }
 
-    /// Check if a constant may contain provenance information. This is used by MIR opts.
-    /// Can return `true` even if there is no provenance.
-    pub fn may_have_provenance(&self, tcx: TyCtxt<'_>, size: Size) -> bool {
-        match *self {
-            ConstValue::ZeroSized | ConstValue::Scalar(Scalar::Int(_)) => return false,
-            ConstValue::Scalar(Scalar::Ptr(..)) => return true,
-            // It's hard to find out the part of the allocation we point to;
-            // just conservatively check everything.
-            ConstValue::Slice { alloc_id, meta: _ } => {
-                !tcx.global_alloc(alloc_id).unwrap_memory().inner().provenance().ptrs().is_empty()
-            }
-            ConstValue::Indirect { alloc_id, offset } => !tcx
-                .global_alloc(alloc_id)
-                .unwrap_memory()
-                .inner()
-                .provenance()
-                .range_empty(AllocRange::from(offset..offset + size), &tcx),
-        }
-    }
-
     /// Check if a constant only contains uninitialized bytes.
     pub fn all_bytes_uninit(&self, tcx: TyCtxt<'_>) -> bool {
         let ConstValue::Indirect { alloc_id, .. } = self else {
@@ -489,39 +469,6 @@ impl<'tcx> Const<'tcx> {
         let val = ConstValue::Scalar(s);
         Self::Val(val, ty)
     }
-
-    /// Return true if any evaluation of this constant always returns the same value,
-    /// taking into account even pointer identity tests.
-    pub fn is_deterministic(&self) -> bool {
-        // Some constants may generate fresh allocations for pointers they contain,
-        // so using the same constant twice can yield two different results.
-        // Notably, valtrees purposefully generate new allocations.
-        match self {
-            Const::Ty(_, c) => match c.kind() {
-                ty::ConstKind::Param(..) => true,
-                // A valtree may be a reference. Valtree references correspond to a
-                // different allocation each time they are evaluated. Valtrees for primitive
-                // types are fine though.
-                ty::ConstKind::Value(cv) => cv.ty.is_primitive(),
-                ty::ConstKind::Unevaluated(..) | ty::ConstKind::Expr(..) => false,
-                // This can happen if evaluation of a constant failed. The result does not matter
-                // much since compilation is doomed.
-                ty::ConstKind::Error(..) => false,
-                // Should not appear in runtime MIR.
-                ty::ConstKind::Infer(..)
-                | ty::ConstKind::Bound(..)
-                | ty::ConstKind::Placeholder(..) => bug!(),
-            },
-            Const::Unevaluated(..) => false,
-            Const::Val(
-                ConstValue::Slice { .. }
-                | ConstValue::ZeroSized
-                | ConstValue::Scalar(_)
-                | ConstValue::Indirect { .. },
-                _,
-            ) => true,
-        }
-    }
 }
 
 /// An unevaluated (potentially generic) constant used in MIR.

compiler/rustc_mir_transform/src/gvn.rs

@@ -61,10 +61,10 @@
 //! The evaluated form is inserted in `evaluated` as an `OpTy` or `None` if evaluation failed.
 //!
 //! The difficulty is non-deterministic evaluation of MIR constants. Some `Const` can have
-//! different runtime values each time they are evaluated. This is the case with
-//! `Const::Slice` which have a new pointer each time they are evaluated, and constants that
-//! contain a fn pointer (`AllocId` pointing to a `GlobalAlloc::Function`) pointing to a different
-//! symbol in each codegen unit.
+//! different runtime values each time they are evaluated. This used to be the case with
+//! `ConstValue::Slice` which have a new pointer each time they are evaluated, and is still the
+//! case with valtrees that generate a new allocation each time they are used. This is checked by
+//! `is_deterministic`.
 //!
 //! Meanwhile, we want to be able to read indirect constants. For instance:
 //! ```
@@ -81,8 +81,11 @@
 //! may be non-deterministic. When that happens, we assign a disambiguator to ensure that we do not
 //! merge the constants. See `duplicate_slice` test in `gvn.rs`.
 //!
-//! Second, when writing constants in MIR, we do not write `Const::Slice` or `Const`
-//! that contain `AllocId`s.
+//! Conversely, some constants cannot cross crate boundaries, which could happen because of
+//! inlining. For instance, constants that contain a fn pointer (`AllocId` pointing to a
+//! `GlobalAlloc::Function`) point to a different symbol in each codegen unit. To avoid this,
+//! when writing constants in MIR, we do not write `Const`s that contain `AllocId`s. This is
+//! checked by `may_have_provenance`.
 
 use std::borrow::Cow;
 use std::hash::{Hash, Hasher};
@@ -103,7 +106,7 @@ use rustc_hir::def::DefKind;
 use rustc_index::bit_set::DenseBitSet;
 use rustc_index::{IndexVec, newtype_index};
 use rustc_middle::bug;
-use rustc_middle::mir::interpret::GlobalAlloc;
+use rustc_middle::mir::interpret::{AllocRange, GlobalAlloc};
 use rustc_middle::mir::visit::*;
 use rustc_middle::mir::*;
 use rustc_middle::ty::layout::HasTypingEnv;
@@ -486,7 +489,7 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
 
     #[instrument(level = "trace", skip(self), ret)]
     fn insert_constant(&mut self, value: Const<'tcx>) -> VnIndex {
-        let (index, new) = if value.is_deterministic() {
+        let (index, new) = if is_deterministic(value) {
             // The constant is deterministic, no need to disambiguate.
             let constant = Value::Constant { value, disambiguator: None };
             self.values.insert(value.ty(), constant)
@@ -529,14 +532,14 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
     fn insert_bool(&mut self, flag: bool) -> VnIndex {
         // Booleans are deterministic.
         let value = Const::from_bool(self.tcx, flag);
-        debug_assert!(value.is_deterministic());
+        debug_assert!(is_deterministic(value));
         self.insert(self.tcx.types.bool, Value::Constant { value, disambiguator: None })
     }
 
     fn insert_scalar(&mut self, ty: Ty<'tcx>, scalar: Scalar) -> VnIndex {
         // Scalars are deterministic.
         let value = Const::from_scalar(self.tcx, scalar, ty);
-        debug_assert!(value.is_deterministic());
+        debug_assert!(is_deterministic(value));
         self.insert(ty, Value::Constant { value, disambiguator: None })
     }
 
@@ -1006,16 +1009,16 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
         operand: &mut Operand<'tcx>,
         location: Location,
     ) -> Option<VnIndex> {
-        match *operand {
-            Operand::Constant(ref constant) => Some(self.insert_constant(constant.const_)),
+        let value = match *operand {
+            Operand::Constant(ref constant) => self.insert_constant(constant.const_),
             Operand::Copy(ref mut place) | Operand::Move(ref mut place) => {
-                let value = self.simplify_place_value(place, location)?;
-                if let Some(const_) = self.try_as_constant(value) {
-                    *operand = Operand::Constant(Box::new(const_));
-                }
-                Some(value)
+                self.simplify_place_value(place, location)?
             }
+        };
+        if let Some(const_) = self.try_as_constant(value) {
+            *operand = Operand::Constant(Box::new(const_));
         }
+        Some(value)
     }
 
     #[instrument(level = "trace", skip(self), ret)]
@@ -1692,6 +1695,45 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
     }
 }
 
+/// Return true if any evaluation of this constant in the same MIR body
+/// always returns the same value, taking into account even pointer identity tests.
+///
+/// In other words, this answers: is "cloning" the `Const` ok?
+fn is_deterministic(c: Const<'_>) -> bool {
+    // Primitive types cannot contain provenance and always have the same value.
+    if c.ty().is_primitive() {
+        return true;
+    }
+
+    match c {
+        // Some constants may generate fresh allocations for pointers they contain,
+        // so using the same constant twice can yield two different results.
+        // Notably, valtrees purposefully generate new allocations.
+        Const::Ty(..) => false,
+        // We do not know the contents, so don't attempt to do anything clever.
+        Const::Unevaluated(..) => false,
+        // When an evaluated constant contains provenance, it is encoded as an `AllocId`.
+        // Cloning the constant will reuse the same `AllocId`. If this is in the same MIR
+        // body, this same `AllocId` will result in the same pointer in codegen.
+        Const::Val(..) => true,
+    }
+}
+
+/// Check if a constant may contain provenance information.
+/// Can return `true` even if there is no provenance.
+fn may_have_provenance(tcx: TyCtxt<'_>, value: ConstValue, size: Size) -> bool {
+    match value {
+        ConstValue::ZeroSized | ConstValue::Scalar(Scalar::Int(_)) => return false,
+        ConstValue::Scalar(Scalar::Ptr(..)) | ConstValue::Slice { .. } => return true,
+        ConstValue::Indirect { alloc_id, offset } => !tcx
+            .global_alloc(alloc_id)
+            .unwrap_memory()
+            .inner()
+            .provenance()
+            .range_empty(AllocRange::from(offset..offset + size), &tcx),
+    }
+}
+
 fn op_to_prop_const<'tcx>(
     ecx: &mut InterpCx<'tcx, DummyMachine>,
     op: &OpTy<'tcx>,
@@ -1767,7 +1809,16 @@ fn op_to_prop_const<'tcx>(
     // Check that we do not leak a pointer.
     // Those pointers may lose part of their identity in codegen.
     // FIXME: remove this hack once https://github.com/rust-lang/rust/issues/79738 is fixed.
-    if ecx.tcx.global_alloc(alloc_id).unwrap_memory().inner().provenance().ptrs().is_empty() {
+    if ecx
+        .tcx
+        .global_alloc(alloc_id)
+        .unwrap_memory()
+        .inner()
+        .provenance()
+        .provenances()
+        .next()
+        .is_none()
+    {
         return Some(value);
     }
 
@@ -1790,14 +1841,28 @@ impl<'tcx> VnState<'_, '_, 'tcx> {
 
     /// If `index` is a `Value::Constant`, return the `Constant` to be put in the MIR.
     fn try_as_constant(&mut self, index: VnIndex) -> Option<ConstOperand<'tcx>> {
-        // This was already constant in MIR, do not change it. If the constant is not
-        // deterministic, adding an additional mention of it in MIR will not give the same value as
-        // the former mention.
-        if let Value::Constant { value, disambiguator: None } = self.get(index) {
-            debug_assert!(value.is_deterministic());
+        let value = self.get(index);
+
+        // This was already an *evaluated* constant in MIR, do not change it.
+        if let Value::Constant { value, disambiguator: None } = value
+            && let Const::Val(..) = value
+        {
+            return Some(ConstOperand { span: DUMMY_SP, user_ty: None, const_: value });
+        }
+
+        if let Some(value) = self.try_as_evaluated_constant(index) {
             return Some(ConstOperand { span: DUMMY_SP, user_ty: None, const_: value });
         }
 
+        // We failed to provide an evaluated form, fallback to using the unevaluated constant.
+        if let Value::Constant { value, disambiguator: None } = value {
+            return Some(ConstOperand { span: DUMMY_SP, user_ty: None, const_: value });
+        }
+
+        None
+    }
+
+    fn try_as_evaluated_constant(&mut self, index: VnIndex) -> Option<Const<'tcx>> {
         let op = self.eval_to_const(index)?;
         if op.layout.is_unsized() {
             // Do not attempt to propagate unsized locals.
@@ -1809,10 +1874,9 @@ impl<'tcx> VnState<'_, '_, 'tcx> {
         // Check that we do not leak a pointer.
         // Those pointers may lose part of their identity in codegen.
         // FIXME: remove this hack once https://github.com/rust-lang/rust/issues/79738 is fixed.
-        assert!(!value.may_have_provenance(self.tcx, op.layout.size));
+        assert!(!may_have_provenance(self.tcx, value, op.layout.size));
 
-        let const_ = Const::Val(value, op.layout.ty);
-        Some(ConstOperand { span: DUMMY_SP, user_ty: None, const_ })
+        Some(Const::Val(value, op.layout.ty))
     }
 
     /// Construct a place which holds the same value as `index` and for which all locals strictly

tests/incremental/hashes/enum_constructors.rs

@@ -65,7 +65,7 @@ pub fn change_field_order_struct_like() -> Enum {
 #[cfg(not(any(cfail1,cfail4)))]
 #[rustc_clean(cfg="cfail2", except="opt_hir_owner_nodes,typeck")]
 #[rustc_clean(cfg="cfail3")]
-#[rustc_clean(cfg="cfail5", except="opt_hir_owner_nodes,typeck,optimized_mir")]
+#[rustc_clean(cfg="cfail5", except="opt_hir_owner_nodes,typeck")]
 #[rustc_clean(cfg="cfail6")]
 // FIXME(michaelwoerister):Interesting. I would have thought that that changes the MIR. And it
 // would if it were not all constants

tests/incremental/hashes/struct_constructors.rs

@@ -62,7 +62,7 @@ pub fn change_field_order_regular_struct() -> RegularStruct {
 #[cfg(not(any(cfail1,cfail4)))]
 #[rustc_clean(cfg="cfail2", except="opt_hir_owner_nodes,typeck")]
 #[rustc_clean(cfg="cfail3")]
-#[rustc_clean(cfg="cfail5", except="opt_hir_owner_nodes,typeck,optimized_mir")]
+#[rustc_clean(cfg="cfail5", except="opt_hir_owner_nodes,typeck")]
 #[rustc_clean(cfg="cfail6")]
 pub fn change_field_order_regular_struct() -> RegularStruct {
     RegularStruct {

tests/mir-opt/const_prop/boxes.main.GVN.panic-abort.diff

@@ -22,7 +22,8 @@
 -         StorageLive(_2);
 +         nop;
           StorageLive(_3);
-          _4 = alloc::alloc::exchange_malloc(const <i32 as std::mem::SizedTypeProperties>::SIZE, const <i32 as std::mem::SizedTypeProperties>::ALIGN) -> [return: bb1, unwind unreachable];
+-         _4 = alloc::alloc::exchange_malloc(const <i32 as std::mem::SizedTypeProperties>::SIZE, const <i32 as std::mem::SizedTypeProperties>::ALIGN) -> [return: bb1, unwind unreachable];
++         _4 = alloc::alloc::exchange_malloc(const 4_usize, const 4_usize) -> [return: bb1, unwind unreachable];
       }
 
       bb1: {

tests/mir-opt/const_prop/boxes.main.GVN.panic-unwind.diff

@@ -22,7 +22,8 @@
 -         StorageLive(_2);
 +         nop;
           StorageLive(_3);
-          _4 = alloc::alloc::exchange_malloc(const <i32 as std::mem::SizedTypeProperties>::SIZE, const <i32 as std::mem::SizedTypeProperties>::ALIGN) -> [return: bb1, unwind continue];
+-         _4 = alloc::alloc::exchange_malloc(const <i32 as std::mem::SizedTypeProperties>::SIZE, const <i32 as std::mem::SizedTypeProperties>::ALIGN) -> [return: bb1, unwind continue];
++         _4 = alloc::alloc::exchange_malloc(const 4_usize, const 4_usize) -> [return: bb1, unwind continue];
       }
 
       bb1: {

tests/mir-opt/funky_arms.float_to_exponential_common.GVN.32bit.panic-abort.diff

@@ -47,7 +47,7 @@
           StorageLive(_20);
           StorageLive(_21);
           _21 = copy (((*_1).0: std::fmt::FormattingOptions).0: u32);
-          _20 = BitAnd(move _21, const core::fmt::flags::SIGN_PLUS_FLAG);
+          _20 = BitAnd(move _21, const 2097152_u32);
           StorageDead(_21);
           _4 = Ne(move _20, const 0_u32);
           StorageDead(_20);
@@ -72,7 +72,7 @@
           StorageLive(_22);
           StorageLive(_23);
           _23 = copy (((*_1).0: std::fmt::FormattingOptions).0: u32);
-          _22 = BitAnd(move _23, const core::fmt::flags::PRECISION_FLAG);
+          _22 = BitAnd(move _23, const 268435456_u32);
           StorageDead(_23);
           switchInt(move _22) -> [0: bb10, otherwise: bb11];
       }

tests/mir-opt/funky_arms.float_to_exponential_common.GVN.32bit.panic-unwind.diff

@@ -47,7 +47,7 @@
           StorageLive(_20);
           StorageLive(_21);
           _21 = copy (((*_1).0: std::fmt::FormattingOptions).0: u32);
-          _20 = BitAnd(move _21, const core::fmt::flags::SIGN_PLUS_FLAG);
+          _20 = BitAnd(move _21, const 2097152_u32);
           StorageDead(_21);
           _4 = Ne(move _20, const 0_u32);
           StorageDead(_20);
@@ -72,7 +72,7 @@
           StorageLive(_22);
           StorageLive(_23);
           _23 = copy (((*_1).0: std::fmt::FormattingOptions).0: u32);
-          _22 = BitAnd(move _23, const core::fmt::flags::PRECISION_FLAG);
+          _22 = BitAnd(move _23, const 268435456_u32);
           StorageDead(_23);
           switchInt(move _22) -> [0: bb10, otherwise: bb11];
       }

tests/mir-opt/funky_arms.float_to_exponential_common.GVN.64bit.panic-abort.diff

@@ -47,7 +47,7 @@
           StorageLive(_20);
           StorageLive(_21);
           _21 = copy (((*_1).0: std::fmt::FormattingOptions).0: u32);
-          _20 = BitAnd(move _21, const core::fmt::flags::SIGN_PLUS_FLAG);
+          _20 = BitAnd(move _21, const 2097152_u32);
           StorageDead(_21);
           _4 = Ne(move _20, const 0_u32);
           StorageDead(_20);
@@ -72,7 +72,7 @@
           StorageLive(_22);
           StorageLive(_23);
           _23 = copy (((*_1).0: std::fmt::FormattingOptions).0: u32);
-          _22 = BitAnd(move _23, const core::fmt::flags::PRECISION_FLAG);
+          _22 = BitAnd(move _23, const 268435456_u32);
           StorageDead(_23);
           switchInt(move _22) -> [0: bb10, otherwise: bb11];
       }

tests/mir-opt/funky_arms.float_to_exponential_common.GVN.64bit.panic-unwind.diff

@@ -47,7 +47,7 @@
           StorageLive(_20);
           StorageLive(_21);
           _21 = copy (((*_1).0: std::fmt::FormattingOptions).0: u32);
-          _20 = BitAnd(move _21, const core::fmt::flags::SIGN_PLUS_FLAG);
+          _20 = BitAnd(move _21, const 2097152_u32);
           StorageDead(_21);
           _4 = Ne(move _20, const 0_u32);
           StorageDead(_20);
@@ -72,7 +72,7 @@
           StorageLive(_22);
           StorageLive(_23);
           _23 = copy (((*_1).0: std::fmt::FormattingOptions).0: u32);
-          _22 = BitAnd(move _23, const core::fmt::flags::PRECISION_FLAG);
+          _22 = BitAnd(move _23, const 268435456_u32);
           StorageDead(_23);
           switchInt(move _22) -> [0: bb10, otherwise: bb11];
       }

tests/mir-opt/gvn_const_eval_polymorphic.no_optimize.GVN.diff

@@ -5,7 +5,8 @@
       let mut _0: bool;
 
       bb0: {
-          _0 = Eq(const no_optimize::<T>::{constant#0}, const no_optimize::<T>::{constant#1});
+-         _0 = Eq(const no_optimize::<T>::{constant#0}, const no_optimize::<T>::{constant#1});
++         _0 = Eq(const no_optimize::<T>::{constant#0}, const true);
           return;
       }
   }

tests/mir-opt/gvn_const_eval_polymorphic.rs

@@ -51,7 +51,7 @@ fn optimize_false<T>() -> bool {
 // EMIT_MIR gvn_const_eval_polymorphic.no_optimize.GVN.diff
 fn no_optimize<T>() -> bool {
     // CHECK-LABEL: fn no_optimize(
-    // CHECK: _0 = Eq(const no_optimize::<T>::{constant#0}, const no_optimize::<T>::{constant#1});
+    // CHECK: _0 = Eq(const no_optimize::<T>::{constant#0}, const true);
     // CHECK-NEXT: return;
     (const { type_name_contains_i32(&generic::<T>) }) == const { true }
 }