From 7aa44eee997e51154eb4dd04ee13a08fe7dc53af Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 12 Sep 2023 15:57:40 +0200 Subject: [PATCH] don't force all slice-typed ConstValue to be ConstValue::Slice --- .../src/const_eval/eval_queries.rs | 65 +++++++++---------- .../rustc_middle/src/mir/interpret/mod.rs | 2 +- .../rustc_middle/src/mir/interpret/value.rs | 23 ++----- 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 { 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"); - } -}