-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify Semantics of Assignment in MIR #97567
Changes from all commits
9b6b689
a50fc36
51a19e1
b7e7a44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,16 +8,6 @@ use rustc_middle::ty::layout::LayoutOf; | |
|
||
use super::{InterpCx, Machine}; | ||
|
||
/// Classify whether an operator is "left-homogeneous", i.e., the LHS has the | ||
/// same type as the result. | ||
#[inline] | ||
fn binop_left_homogeneous(op: mir::BinOp) -> bool { | ||
use rustc_middle::mir::BinOp::*; | ||
match op { | ||
Add | Sub | Mul | Div | Rem | BitXor | BitAnd | BitOr | Offset | Shl | Shr => true, | ||
Eq | Ne | Lt | Le | Gt | Ge => false, | ||
} | ||
} | ||
/// Classify whether an operator is "right-homogeneous", i.e., the RHS has the | ||
/// same type as the LHS. | ||
#[inline] | ||
|
@@ -157,26 +147,29 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |
rvalue: &mir::Rvalue<'tcx>, | ||
place: mir::Place<'tcx>, | ||
) -> InterpResult<'tcx> { | ||
let dest = self.eval_place(place)?; | ||
|
||
use rustc_middle::mir::Rvalue::*; | ||
match *rvalue { | ||
ThreadLocalRef(did) => { | ||
let dest = self.eval_place(place)?; | ||
let ptr = M::thread_local_static_base_pointer(self, did)?; | ||
self.write_pointer(ptr, &dest)?; | ||
} | ||
|
||
Use(ref operand) => { | ||
// FIXME: Here, in `Aggregate`, and in `Repeat`, the various `dest = eval_place`s | ||
// should also be retagging the pointer to ensure that it does not overlap with the | ||
// RHS. | ||
let dest = self.eval_place(place)?; | ||
// Avoid recomputing the layout | ||
let op = self.eval_operand(operand, Some(dest.layout))?; | ||
self.copy_op(&op, &dest)?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that |
||
} | ||
|
||
BinaryOp(bin_op, box (ref left, ref right)) => { | ||
let layout = binop_left_homogeneous(bin_op).then_some(dest.layout); | ||
let left = self.read_immediate(&self.eval_operand(left, layout)?)?; | ||
let left = self.read_immediate(&self.eval_operand(left, None)?)?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs careful benchmarking; the perf gain from that optimization was quite noticeable in the past. |
||
let layout = binop_right_homogeneous(bin_op).then_some(left.layout); | ||
let right = self.read_immediate(&self.eval_operand(right, layout)?)?; | ||
let dest = self.eval_place(place)?; | ||
self.binop_ignore_overflow(bin_op, &left, &right, &dest)?; | ||
} | ||
|
||
|
@@ -185,19 +178,22 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |
let left = self.read_immediate(&self.eval_operand(left, None)?)?; | ||
let layout = binop_right_homogeneous(bin_op).then_some(left.layout); | ||
let right = self.read_immediate(&self.eval_operand(right, layout)?)?; | ||
let dest = self.eval_place(place)?; | ||
self.binop_with_overflow(bin_op, &left, &right, &dest)?; | ||
} | ||
|
||
UnaryOp(un_op, ref operand) => { | ||
// The operand always has the same type as the result. | ||
let val = self.read_immediate(&self.eval_operand(operand, Some(dest.layout))?)?; | ||
let val = self.read_immediate(&self.eval_operand(operand, None)?)?; | ||
let val = self.unary_op(un_op, &val)?; | ||
let dest = self.eval_place(place)?; | ||
assert_eq!(val.layout, dest.layout, "layout mismatch for result of {:?}", un_op); | ||
self.write_immediate(*val, &dest)?; | ||
} | ||
|
||
Aggregate(box ref kind, ref operands) => { | ||
assert!(matches!(kind, mir::AggregateKind::Array(..))); | ||
let dest = self.eval_place(place)?; | ||
|
||
for (field_index, operand) in operands.iter().enumerate() { | ||
let op = self.eval_operand(operand, None)?; | ||
|
@@ -207,6 +203,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |
} | ||
|
||
Repeat(ref operand, _) => { | ||
let dest = self.eval_place(place)?; | ||
let src = self.eval_operand(operand, None)?; | ||
assert!(!src.layout.is_unsized()); | ||
let dest = self.force_allocation(&dest)?; | ||
|
@@ -242,17 +239,19 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |
} | ||
} | ||
|
||
Len(place) => { | ||
let src = self.eval_place(place)?; | ||
Len(src_place) => { | ||
let src = self.eval_place(src_place)?; | ||
let mplace = self.force_allocation(&src)?; | ||
let len = mplace.len(self)?; | ||
let dest = self.eval_place(place)?; | ||
self.write_scalar(Scalar::from_machine_usize(len, self), &dest)?; | ||
} | ||
|
||
AddressOf(_, place) | Ref(_, _, place) => { | ||
let src = self.eval_place(place)?; | ||
let place = self.force_allocation(&src)?; | ||
self.write_immediate(place.to_ref(self), &dest)?; | ||
AddressOf(_, src_place) | Ref(_, _, src_place) => { | ||
let src_place = self.eval_place(src_place)?; | ||
let src_place = self.force_allocation(&src_place)?; | ||
let dest = self.eval_place(place)?; | ||
self.write_immediate(src_place.to_ref(self), &dest)?; | ||
} | ||
|
||
NullaryOp(null_op, ty) => { | ||
|
@@ -270,31 +269,33 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |
mir::NullOp::SizeOf => layout.size.bytes(), | ||
mir::NullOp::AlignOf => layout.align.abi.bytes(), | ||
}; | ||
let dest = self.eval_place(place)?; | ||
self.write_scalar(Scalar::from_machine_usize(val, self), &dest)?; | ||
} | ||
|
||
ShallowInitBox(ref operand, _) => { | ||
let src = self.eval_operand(operand, None)?; | ||
let v = self.read_immediate(&src)?; | ||
let dest = self.eval_place(place)?; | ||
self.write_immediate(*v, &dest)?; | ||
} | ||
|
||
Cast(cast_kind, ref operand, cast_ty) => { | ||
let src = self.eval_operand(operand, None)?; | ||
let cast_ty = | ||
self.subst_from_current_frame_and_normalize_erasing_regions(cast_ty)?; | ||
let dest = self.eval_place(place)?; | ||
self.cast(&src, cast_kind, cast_ty, &dest)?; | ||
} | ||
|
||
Discriminant(place) => { | ||
let op = self.eval_place_to_op(place, None)?; | ||
Discriminant(src_place) => { | ||
let op = self.eval_place_to_op(src_place, None)?; | ||
let discr_val = self.read_discriminant(&op)?.0; | ||
let dest = self.eval_place(place)?; | ||
self.write_scalar(discr_val, &dest)?; | ||
} | ||
} | ||
|
||
trace!("{:?}", self.dump_place(*dest)); | ||
|
||
Ok(()) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1618,6 +1618,12 @@ pub enum StatementKind<'tcx> { | |
/// parts of this may do type specific things that are more complicated than simply copying | ||
/// bytes. | ||
/// | ||
/// For `Rvalue::Use`, `Rvalue::Aggregate`, and `Rvalue::Repeat`, the destination `Place` may | ||
/// not overlap with any memory computed as a part of the `Rvalue`. Relatedly, the evaluation | ||
/// order is `compute lhs place -> compute rhs value -> store value in place`. The plan is for | ||
/// the rule about the evaluation order to justify the rule about the non-overlappingness via | ||
/// the aliasing model. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait, having the eval order depend on the rvalue is deliberate? That thought didn't even occur to me when reading the I think this is a very bad idea. It makes justifying rvalue transformations a total disaster. |
||
/// | ||
/// **Needs clarification**: The implication of the above idea would be that assignment implies | ||
/// that the resulting value is initialized. I believe we could commit to this separately from | ||
/// committing to whatever part of the memory model we would need to decide on to make the above | ||
|
@@ -1630,11 +1636,6 @@ pub enum StatementKind<'tcx> { | |
/// could meaningfully require this anyway. How about free lifetimes? Is ignoring this | ||
/// interesting for optimizations? Do we want to allow such optimizations? | ||
/// | ||
/// **Needs clarification**: We currently require that the LHS place not overlap with any place | ||
/// read as part of computation of the RHS for some rvalues (generally those not producing | ||
/// primitives). This requirement is under discussion in [#68364]. As a part of this discussion, | ||
/// it is also unclear in what order the components are evaluated. | ||
/// | ||
/// [#68364]: https://github.com/rust-lang/rust/issues/68364 | ||
/// | ||
/// See [`Rvalue`] documentation for details on each of those. | ||
|
@@ -1766,6 +1767,8 @@ pub enum RetagKind { | |
Raw, | ||
/// A "normal" retag. | ||
Default, | ||
/// Retagging for the lhs of an assignment: `this_place = val;` | ||
Assign, | ||
} | ||
|
||
/// The `FakeReadCause` describes the type of pattern why a FakeRead statement exists. | ||
|
@@ -1837,6 +1840,7 @@ impl Debug for Statement<'_> { | |
RetagKind::TwoPhase => "[2phase] ", | ||
RetagKind::Raw => "[raw] ", | ||
RetagKind::Default => "", | ||
RetagKind::Assign => "[assign]", | ||
}, | ||
place, | ||
), | ||
|
@@ -2166,6 +2170,58 @@ impl<'tcx> Place<'tcx> { | |
|
||
Place { local: self.local, projection: tcx.intern_place_elems(new_projections) } | ||
} | ||
|
||
/// If this returns true, then: If `self` and `other` are evaluated immediately in succession, | ||
/// then the places that result from the evaluation are non-overlapping. | ||
/// | ||
/// Gives no information if it returns false. | ||
/// | ||
/// This places passed to this function must obey the derefered invariant, ie only the first | ||
/// projection may be a deref projection. | ||
pub fn is_disjoint(self, other: Place<'tcx>) -> bool { | ||
use ProjectionElem::*; | ||
let indirect_a = self.projection.get(0) == Some(&Deref); | ||
let indirect_b = other.projection.get(0) == Some(&Deref); | ||
if self.local != other.local { | ||
// If the places are based on different locals, and neither of them is indirect, then | ||
// they must each refer to memory inside their locals, which must be disjoint. | ||
return !(indirect_a || indirect_b); | ||
} | ||
|
||
// We already know that the locals are the same. If both are indirect, then the result of | ||
// dereferencing both must be the same. If neither are indirect, then the result of | ||
// derefencing neither must be the same. If just one is indirect, we can't know. This also | ||
// removes the deref projection, if it's there. | ||
let (proj_a, proj_b) = match (indirect_a, indirect_b) { | ||
(true, true) => (&self.projection[1..], &other.projection[1..]), | ||
(false, false) => (&self.projection[..], &other.projection[..]), | ||
_ => return false, | ||
}; | ||
|
||
if let Some(pair) = std::iter::zip(proj_a, proj_b).find(|(a, b)| a != b) { | ||
// Because we are interested in the places when they are evaluated immediately in | ||
// succession, equal projections means equal places, even if there are indexing | ||
// projections. Furthermore, there are no more derefs involved, since we removed those | ||
// above. | ||
match pair { | ||
// The fields are different and different fields are disjoint | ||
(Field(a, _), Field(b, _)) if a != b => true, | ||
// The indexes are different, so the places are disjoint | ||
( | ||
ConstantIndex { offset: offset_a, from_end: from_end_a, .. }, | ||
ConstantIndex { offset: offset_b, from_end: from_end_b, .. }, | ||
) if offset_a != offset_b && from_end_a == from_end_b => true, | ||
// The subranges are disjoint | ||
( | ||
Subslice { from: from_a, to: to_a, from_end: false }, | ||
Subslice { from: from_b, to: to_b, from_end: false }, | ||
) if to_a <= from_b || to_b <= from_a => true, | ||
_ => false, | ||
} | ||
} else { | ||
false | ||
} | ||
} | ||
} | ||
|
||
impl From<Local> for Place<'_> { | ||
|
@@ -2475,9 +2531,9 @@ impl<'tcx> Operand<'tcx> { | |
/// | ||
/// Not all of these are allowed at every [`MirPhase`] - when this is the case, it's stated below. | ||
/// | ||
/// Computing any rvalue begins by evaluating the places and operands in some order (**Needs | ||
/// clarification**: Which order?). These are then used to produce a "value" - the same kind of | ||
/// value that an [`Operand`] produces. | ||
/// The evaluation order of all components of any part of these rvalues is the order in which they | ||
/// appear. These components are then combined in a variant specific way, until they finally produce | ||
/// a "value" - the same kind of value that an [`Operand`] produces. | ||
pub enum Rvalue<'tcx> { | ||
/// Yields the operand unchanged | ||
Use(Operand<'tcx>), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like how this makes the evaluation of the LHS and RHS all tangled up. For some rvalues you do the LHS first, for some the LHS; it all looks like a big mess.
IMO we should just stick to left-to-right evaluation order. I don't see the benefit from changing this, and it surely has a significant complexity cost.