From 956b51f79a482d6eb90e4320f6e170c39ecd6d68 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 17 Aug 2018 17:47:37 +0200 Subject: [PATCH] optimize validation iterating over the elements of an array This is still roughly 45ns slower than the old state, because it now works with an MPlaceTy and uses the appropriate abstractions, instead of working with a ptr-align pair directly. --- src/librustc_mir/interpret/memory.rs | 27 ++++++-------- src/librustc_mir/interpret/operand.rs | 15 ++++++-- src/librustc_mir/interpret/place.rs | 51 ++++++++++++++++++++++---- src/librustc_mir/interpret/validity.rs | 9 +++-- 4 files changed, 72 insertions(+), 30 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 72bd32efe7b..b7e951a78f5 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -494,6 +494,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { /// Byte accessors impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { + /// This checks alignment! fn get_bytes_unchecked( &self, ptr: Pointer, @@ -514,6 +515,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { Ok(&alloc.bytes[offset..offset + size.bytes() as usize]) } + /// This checks alignment! fn get_bytes_unchecked_mut( &mut self, ptr: Pointer, @@ -551,7 +553,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { ) -> EvalResult<'tcx, &mut [u8]> { assert_ne!(size.bytes(), 0); self.clear_relocations(ptr, size)?; - self.mark_definedness(ptr.into(), size, true)?; + self.mark_definedness(ptr, size, true)?; self.get_bytes_unchecked_mut(ptr, size, align) } } @@ -749,9 +751,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { Ok(()) } + /// Read a *non-ZST* scalar pub fn read_scalar(&self, ptr: Pointer, ptr_align: Align, size: Size) -> EvalResult<'tcx, ScalarMaybeUndef> { self.check_relocation_edges(ptr, size)?; // Make sure we don't read part of a pointer as a pointer let endianness = self.endianness(); + // get_bytes_unchecked tests alignment let bytes = self.get_bytes_unchecked(ptr, size, ptr_align.min(self.int_align(size)))?; // Undef check happens *after* we established that the alignment is correct. // We must not return Ok() for unaligned pointers! @@ -784,16 +788,15 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { self.read_scalar(ptr, ptr_align, self.pointer_size()) } + /// Write a *non-ZST* scalar pub fn write_scalar( &mut self, - ptr: Scalar, + ptr: Pointer, ptr_align: Align, val: ScalarMaybeUndef, type_size: Size, - type_align: Align, ) -> EvalResult<'tcx> { let endianness = self.endianness(); - self.check_align(ptr, ptr_align)?; let val = match val { ScalarMaybeUndef::Scalar(scalar) => scalar, @@ -806,12 +809,6 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { val.offset.bytes() as u128 } - Scalar::Bits { size: 0, .. } => { - // nothing to do for ZSTs - assert_eq!(type_size.bytes(), 0); - return Ok(()); - } - Scalar::Bits { bits, size } => { assert_eq!(size as u64, type_size.bytes()); assert_eq!(truncate(bits, Size::from_bytes(size.into())), bits, @@ -820,10 +817,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { }, }; - let ptr = ptr.to_ptr()?; - { - let dst = self.get_bytes_mut(ptr, type_size, ptr_align.min(type_align))?; + // get_bytes_mut checks alignment + let dst = self.get_bytes_mut(ptr, type_size, ptr_align)?; write_target_uint(endianness, dst, bytes).unwrap(); } @@ -843,7 +839,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { pub fn write_ptr_sized(&mut self, ptr: Pointer, ptr_align: Align, val: ScalarMaybeUndef) -> EvalResult<'tcx> { let ptr_size = self.pointer_size(); - self.write_scalar(ptr.into(), ptr_align, val, ptr_size, ptr_align) + self.write_scalar(ptr.into(), ptr_align, val, ptr_size) } fn int_align(&self, size: Size) -> Align { @@ -959,14 +955,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { pub fn mark_definedness( &mut self, - ptr: Scalar, + ptr: Pointer, size: Size, new_state: bool, ) -> EvalResult<'tcx> { if size.bytes() == 0 { return Ok(()); } - let ptr = ptr.to_ptr()?; let alloc = self.get_mut(ptr.alloc_id)?; alloc.undef_mask.set_range( ptr.offset, diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 169a5689694..1907b2beeaf 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -41,6 +41,7 @@ impl<'tcx> Value { Value::ScalarPair(val.into(), Scalar::Ptr(vtable).into()) } + #[inline] pub fn to_scalar_or_undef(self) -> ScalarMaybeUndef { match self { Value::Scalar(val) => val, @@ -48,11 +49,14 @@ impl<'tcx> Value { } } + #[inline] pub fn to_scalar(self) -> EvalResult<'tcx, Scalar> { self.to_scalar_or_undef().not_undef() } /// Convert the value into a pointer (or a pointer-sized integer). + /// Throws away the second half of a ScalarPair! + #[inline] pub fn to_scalar_ptr(self) -> EvalResult<'tcx, Scalar> { match self { Value::Scalar(ptr) | @@ -89,6 +93,7 @@ pub struct ValTy<'tcx> { impl<'tcx> ::std::ops::Deref for ValTy<'tcx> { type Target = Value; + #[inline(always)] fn deref(&self) -> &Value { &self.value } @@ -141,12 +146,14 @@ pub struct OpTy<'tcx> { impl<'tcx> ::std::ops::Deref for OpTy<'tcx> { type Target = Operand; + #[inline(always)] fn deref(&self) -> &Operand { &self.op } } impl<'tcx> From> for OpTy<'tcx> { + #[inline(always)] fn from(mplace: MPlaceTy<'tcx>) -> Self { OpTy { op: Operand::Indirect(*mplace), @@ -156,6 +163,7 @@ impl<'tcx> From> for OpTy<'tcx> { } impl<'tcx> From> for OpTy<'tcx> { + #[inline(always)] fn from(val: ValTy<'tcx>) -> Self { OpTy { op: Operand::Immediate(val.value), @@ -192,14 +200,15 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { return Ok(None); } let (ptr, ptr_align) = mplace.to_scalar_ptr_align(); - self.memory.check_align(ptr, ptr_align)?; if mplace.layout.size.bytes() == 0 { + // Not all ZSTs have a layout we would handle below, so just short-circuit them + // all here. + self.memory.check_align(ptr, ptr_align)?; return Ok(Some(Value::Scalar(Scalar::zst().into()))); } let ptr = ptr.to_ptr()?; - match mplace.layout.abi { layout::Abi::Scalar(..) => { let scalar = self.memory.read_scalar(ptr, ptr_align, mplace.layout.size)?; @@ -264,7 +273,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { // This decides which types we will use the Immediate optimization for, and hence should // match what `try_read_value` and `eval_place_to_op` support. if layout.is_zst() { - return Ok(Operand::Immediate(Value::Scalar(ScalarMaybeUndef::Undef))); + return Ok(Operand::Immediate(Value::Scalar(Scalar::zst().into()))); } Ok(match layout.abi { diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 92f9268ef87..58c976086bb 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -54,6 +54,7 @@ pub struct PlaceTy<'tcx> { impl<'tcx> ::std::ops::Deref for PlaceTy<'tcx> { type Target = Place; + #[inline(always)] fn deref(&self) -> &Place { &self.place } @@ -68,12 +69,14 @@ pub struct MPlaceTy<'tcx> { impl<'tcx> ::std::ops::Deref for MPlaceTy<'tcx> { type Target = MemPlace; + #[inline(always)] fn deref(&self) -> &MemPlace { &self.mplace } } impl<'tcx> From> for PlaceTy<'tcx> { + #[inline(always)] fn from(mplace: MPlaceTy<'tcx>) -> Self { PlaceTy { place: Place::Ptr(mplace.mplace), @@ -160,6 +163,7 @@ impl<'tcx> PartialEq for MPlaceTy<'tcx> { impl<'tcx> Eq for MPlaceTy<'tcx> {} impl<'tcx> OpTy<'tcx> { + #[inline(always)] pub fn try_as_mplace(self) -> Result, Value> { match *self { Operand::Indirect(mplace) => Ok(MPlaceTy { mplace, layout: self.layout }), @@ -167,7 +171,7 @@ impl<'tcx> OpTy<'tcx> { } } - #[inline] + #[inline(always)] pub fn to_mem_place(self) -> MPlaceTy<'tcx> { self.try_as_mplace().unwrap() } @@ -311,6 +315,28 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { Ok(MPlaceTy { mplace: MemPlace { ptr, align, extra }, layout: field_layout }) } + // Iterates over all fields of an array. Much more efficient than doing the + // same by repeatedly calling `mplace_array`. + pub fn mplace_array_fields( + &self, + base: MPlaceTy<'tcx>, + ) -> EvalResult<'tcx, impl Iterator>> + 'a> { + let len = base.len(); + let stride = match base.layout.fields { + layout::FieldPlacement::Array { stride, .. } => stride, + _ => bug!("mplace_array_fields: expected an array layout"), + }; + let layout = base.layout.field(self, 0)?; + let dl = &self.tcx.data_layout; + Ok((0..len).map(move |i| { + let ptr = base.ptr.ptr_offset(i * stride, dl)?; + Ok(MPlaceTy { + mplace: MemPlace { ptr, align: base.align, extra: PlaceExtra::None }, + layout + }) + })) + } + pub fn mplace_subslice( &self, base: MPlaceTy<'tcx>, @@ -545,14 +571,22 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { value: Value, dest: MPlaceTy<'tcx>, ) -> EvalResult<'tcx> { - assert_eq!(dest.extra, PlaceExtra::None); + let (ptr, ptr_align) = dest.to_scalar_ptr_align(); // Note that it is really important that the type here is the right one, and matches the type things are read at. // In case `src_val` is a `ScalarPair`, we don't do any magic here to handle padding properly, which is only // correct if we never look at this data with the wrong type. + + // Nothing to do for ZSTs, other than checking alignment + if dest.layout.size.bytes() == 0 { + self.memory.check_align(ptr, ptr_align)?; + return Ok(()); + } + + let ptr = ptr.to_ptr()?; match value { Value::Scalar(scalar) => { self.memory.write_scalar( - dest.ptr, dest.align, scalar, dest.layout.size, dest.layout.align + ptr, ptr_align.min(dest.layout.align), scalar, dest.layout.size ) } Value::ScalarPair(a_val, b_val) => { @@ -562,12 +596,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { }; let (a_size, b_size) = (a.size(&self), b.size(&self)); let (a_align, b_align) = (a.align(&self), b.align(&self)); - let a_ptr = dest.ptr; let b_offset = a_size.abi_align(b_align); - let b_ptr = a_ptr.ptr_offset(b_offset, &self)?.into(); + let b_ptr = ptr.offset(b_offset, &self)?.into(); - self.memory.write_scalar(a_ptr, dest.align, a_val, a_size, a_align)?; - self.memory.write_scalar(b_ptr, dest.align, b_val, b_size, b_align) + self.memory.write_scalar(ptr, ptr_align.min(a_align), a_val, a_size)?; + self.memory.write_scalar(b_ptr, ptr_align.min(b_align), b_val, b_size) } } } @@ -608,6 +641,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { ) -> EvalResult<'tcx, MPlaceTy<'tcx>> { let mplace = match place.place { Place::Local { frame, local } => { + // FIXME: Consider not doing anything for a ZST, and just returning + // a fake pointer? + // We need the layout of the local. We can NOT use the layout we got, // that might e.g. be a downcast variant! let local_layout = self.layout_of_local(frame, local)?; @@ -707,6 +743,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { } /// Every place can be read from, so we can turm them into an operand + #[inline(always)] pub fn place_to_op(&self, place: PlaceTy<'tcx>) -> EvalResult<'tcx, OpTy<'tcx>> { let op = match place.place { Place::Ptr(mplace) => { diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 53d30068310..12ea3ed09ff 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -124,7 +124,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { } } - /// This function checks the memory where `ptr` points to. + /// This function checks the memory where `dest` points to. The place must be sized + /// (i.e., dest.extra == PlaceExtra::None). /// It will error if the bits at the destination do not match the ones described by the layout. pub fn validate_mplace( &self, @@ -205,11 +206,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { // See https://github.com/rust-lang/rust/issues/32836#issuecomment-406875389 Ok(()) }, - layout::FieldPlacement::Array { count, .. } => { - for i in 0..count { + layout::FieldPlacement::Array { .. } => { + for (i, field) in self.mplace_array_fields(dest)?.enumerate() { + let field = field?; let mut path = path.clone(); self.dump_field_name(&mut path, dest.layout.ty, i as usize, variant).unwrap(); - let field = self.mplace_field(dest, i)?; self.validate_mplace(field, path, seen, todo)?; } Ok(())