don't force all slice-typed ConstValue to be ConstValue::Slice

3 files changed, 36 insertions, 54 deletions

diff --git a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs
index d4b7d6f4e29..454baf2a745 100644
--- a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs
+++ b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs
@@ -18,9 +18,9 @@ use super::{CanAccessStatics, CompileTimeEvalContext, CompileTimeInterpreter};
 use crate::errors;
 use crate::interpret::eval_nullary_intrinsic;
 use crate::interpret::{
-    intern_const_alloc_recursive, Allocation, ConstAlloc, ConstValue, CtfeValidationMode, GlobalId,
-    Immediate, InternKind, InterpCx, InterpError, InterpResult, MPlaceTy, MemoryKind, OpTy,
-    RefTracking, StackPopCleanup,
+    intern_const_alloc_recursive, ConstAlloc, ConstValue, CtfeValidationMode, GlobalId, Immediate,
+    InternKind, InterpCx, InterpError, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking,
+    StackPopCleanup,
 };
 
 // Returns a pointer to where the result lives
@@ -105,8 +105,7 @@ pub(super) fn mk_eval_cx<'mir, 'tcx>(
     )
 }
 
-/// This function converts an interpreter value into a constant that is meant for use in the
-/// type system.
+/// This function converts an interpreter value into a MIR constant.
 #[instrument(skip(ecx), level = "debug")]
 pub(super) fn op_to_const<'tcx>(
     ecx: &CompileTimeEvalContext<'_, 'tcx>,
@@ -117,28 +116,25 @@ pub(super) fn op_to_const<'tcx>(
         return ConstValue::ZeroSized;
     }
 
-    // We do not have value optimizations for everything.
-    // Only scalars and slices, since they are very common.
-    let try_as_immediate = match op.layout.abi {
+    // All scalar types should be stored as `ConstValue::Scalar`. This is needed to make
+    // `ConstValue::try_to_scalar` efficient; we want that to work for *all* constants of scalar
+    // type (it's used throughout the compiler and having it work just on literals is not enough)
+    // and we want it to be fast (i.e., don't go to an `Allocation` and reconstruct the `Scalar`
+    // from its byte-serialized form).
+    let force_as_immediate = match op.layout.abi {
         Abi::Scalar(abi::Scalar::Initialized { .. }) => true,
-        Abi::ScalarPair(..) => match op.layout.ty.kind() {
-            ty::Ref(_, inner, _) => match *inner.kind() {
-                ty::Slice(elem) => elem == ecx.tcx.types.u8,
-                ty::Str => true,
-                _ => false,
-            },
-            _ => false,
-        },
+        // We don't *force* `ConstValue::Slice` for `ScalarPair`. This has the advantage that if the
+        // input `op` is a place, then turning it into a `ConstValue` and back into a `OpTy` will
+        // not have to generate any duplicate allocations (we preserve the original `AllocId` in
+        // `ConstValue::Indirect`). It means accessing the contents of a slice can be slow (since
+        // they can be stored as `ConstValue::Indirect`), but that's not relevant since we barely
+        // ever have to do this. (`try_get_slice_bytes_for_diagnostics` exists to provide this
+        // functionality.)
         _ => false,
     };
-    let immediate = if try_as_immediate {
+    let immediate = if force_as_immediate {
         Right(ecx.read_immediate(op).expect("normalization works on validated constants"))
     } else {
-        // It is guaranteed that any non-slice scalar pair is actually `Indirect` here.
-        // When we come back from raw const eval, we are always by-ref. The only way our op here is
-        // by-val is if we are in destructure_mir_constant, i.e., if this is (a field of) something that we
-        // "tried to make immediate" before. We wouldn't do that for non-slice scalar pairs or
-        // structs containing such.
         op.as_mplace_or_imm()
     };
 
@@ -151,25 +147,22 @@ pub(super) fn op_to_const<'tcx>(
             let alloc_id = alloc_id.expect("cannot have `fake` place fot non-ZST type");
             ConstValue::Indirect { alloc_id, offset }
         }
-        // see comment on `let try_as_immediate` above
+        // see comment on `let force_as_immediate` above
         Right(imm) => match *imm {
             Immediate::Scalar(x) => ConstValue::Scalar(x),
             Immediate::ScalarPair(a, b) => {
                 debug!("ScalarPair(a: {:?}, b: {:?})", a, b);
+                // FIXME: assert that this has an appropriate type.
+                // Currently we actually get here for non-[u8] slices during valtree construction!
+                let msg = "`op_to_const` on an immediate scalar pair must only be used on slice references to actually allocated memory";
                 // We know `offset` is relative to the allocation, so we can use `into_parts`.
-                let (data, start) = match a.to_pointer(ecx).unwrap().into_parts() {
-                    (Some(alloc_id), offset) => {
-                        (ecx.tcx.global_alloc(alloc_id).unwrap_memory(), offset.bytes())
-                    }
-                    (None, _offset) => (
-                        ecx.tcx.mk_const_alloc(Allocation::from_bytes_byte_aligned_immutable(
-                            b"" as &[u8],
-                        )),
-                        0,
-                    ),
-                };
-                let len = b.to_target_usize(ecx).unwrap();
-                let start = start.try_into().unwrap();
+                // We use `ConstValue::Slice` so that we don't have to generate an allocation for
+                // `ConstValue::Indirect` here.
+                let (alloc_id, offset) = a.to_pointer(ecx).expect(msg).into_parts();
+                let alloc_id = alloc_id.expect(msg);
+                let data = ecx.tcx.global_alloc(alloc_id).unwrap_memory();
+                let start = offset.bytes_usize();
+                let len = b.to_target_usize(ecx).expect(msg);
                 let len: usize = len.try_into().unwrap();
                 ConstValue::Slice { data, start, end: start + len }
             }
diff --git a/compiler/rustc_middle/src/mir/interpret/mod.rs b/compiler/rustc_middle/src/mir/interpret/mod.rs
index f03a28d210d..44d1dcbbe17 100644
--- a/compiler/rustc_middle/src/mir/interpret/mod.rs
+++ b/compiler/rustc_middle/src/mir/interpret/mod.rs
@@ -149,7 +149,7 @@ pub use self::error::{
     UnsupportedOpInfo, ValidationErrorInfo, ValidationErrorKind,
 };
 
-pub use self::value::{get_slice_bytes, ConstAlloc, ConstValue, Scalar};
+pub use self::value::{ConstAlloc, ConstValue, Scalar};
 
 pub use self::allocation::{
     alloc_range, AllocBytes, AllocError, AllocRange, AllocResult, Allocation, ConstAllocation,
diff --git a/compiler/rustc_middle/src/mir/interpret/value.rs b/compiler/rustc_middle/src/mir/interpret/value.rs
index a77d16e42e9..6207a35e4f3 100644
--- a/compiler/rustc_middle/src/mir/interpret/value.rs
+++ b/compiler/rustc_middle/src/mir/interpret/value.rs
@@ -12,7 +12,7 @@ use rustc_target::abi::{HasDataLayout, Size};
 use crate::ty::{ParamEnv, ScalarInt, Ty, TyCtxt};
 
 use super::{
-    AllocId, AllocRange, ConstAllocation, InterpResult, Pointer, PointerArithmetic, Provenance,
+    AllocId, ConstAllocation, InterpResult, Pointer, PointerArithmetic, Provenance,
     ScalarSizeMismatch,
 };
 
@@ -40,10 +40,14 @@ pub enum ConstValue<'tcx> {
 
     /// Used for `&[u8]` and `&str`.
     ///
-    /// This is worth the optimization since Rust has literals of that type.
+    /// This is worth an optimized representation since Rust has literals of these types.
+    /// Not having to indirect those through an `AllocId` (or two, if we used `Indirect`) has shown
+    /// measurable performance improvements on stress tests.
     Slice { data: ConstAllocation<'tcx>, start: usize, end: usize },
 
     /// A value not representable by the other variants; needs to be stored in-memory.
+    ///
+    /// Must *not* be used for scalars or ZST, but having `&str` or other slices in this variant is fine.
     Indirect {
         /// The backing memory of the value. May contain more memory than needed for just the value
         /// if this points into some other larger ConstValue.
@@ -511,18 +515,3 @@ impl<'tcx, Prov: Provenance> Scalar<Prov> {
         Ok(Double::from_bits(self.to_u64()?.into()))
     }
 }
-
-/// Gets the bytes of a constant slice value.
-pub fn get_slice_bytes<'tcx>(cx: &impl HasDataLayout, val: ConstValue<'tcx>) -> &'tcx [u8] {
-    if let ConstValue::Slice { data, start, end } = val {
-        let len = end - start;
-        data.inner()
-            .get_bytes_strip_provenance(
-                cx,
-                AllocRange { start: Size::from_bytes(start), size: Size::from_bytes(len) },
-            )
-            .unwrap_or_else(|err| bug!("const slice is invalid: {:?}", err))
-    } else {
-        bug!("expected const slice, but found another const value");
-    }
-}