diff --git a/compiler/rustc_const_eval/src/interpret/step.rs b/compiler/rustc_const_eval/src/interpret/step.rs index e6dfdf54a32d1..ebeb4a0da30aa 100644 --- a/compiler/rustc_const_eval/src/interpret/step.rs +++ b/compiler/rustc_const_eval/src/interpret/step.rs @@ -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)?; } 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)?)?; 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,12 +269,14 @@ 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)?; } @@ -283,18 +284,18 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { 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(()) } diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 71cea005cf858..67cd45f786ccf 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -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. + /// /// **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 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>), diff --git a/compiler/rustc_middle/src/mir/tcx.rs b/compiler/rustc_middle/src/mir/tcx.rs index f1fb484a801ba..24d9445efc346 100644 --- a/compiler/rustc_middle/src/mir/tcx.rs +++ b/compiler/rustc_middle/src/mir/tcx.rs @@ -240,6 +240,73 @@ impl<'tcx> Rvalue<'tcx> { false } + + /// Returns true if evaluation of the rvalue is left to right. + /// + /// See [`StatementKind::Asssign`](crate::mir::StatementKind::Assign) for details. + #[inline] + pub fn is_left_to_right(&self) -> bool { + match self { + Rvalue::Use(_) | Rvalue::Repeat(..) | Rvalue::Aggregate(..) => true, + Rvalue::Ref(..) + | Rvalue::ThreadLocalRef(..) + | Rvalue::AddressOf(..) + | Rvalue::Len(..) + | Rvalue::Cast(..) + | Rvalue::BinaryOp(..) + | Rvalue::CheckedBinaryOp(..) + | Rvalue::NullaryOp(..) + | Rvalue::UnaryOp(..) + | Rvalue::Discriminant(..) + | Rvalue::ShallowInitBox(..) => false, + } + } + + /// Ensures that an assignment statement obeys the rules for the left and right hand side being + /// non-overlapping. + /// + /// This function accepts as input the sides of an assignment statement. It will then decide if + /// a temporary needs to be inserted, and if so insert that temporary by modifying the + /// assignment statement and returning a new statement to be inserted immediately after the + /// current one. + pub fn expand_assign( + stmt: &mut (Place<'tcx>, Rvalue<'tcx>), + source: SourceInfo, + locals: &mut LocalDecls<'tcx>, + tcx: TyCtxt<'tcx>, + ) -> Option> { + let is_op_disjoint = + |op: &Operand<'tcx>| op.place().map(|p| p.is_disjoint(stmt.0)).unwrap_or(true); + let ok_as_is = match &stmt.1 { + Rvalue::Use(op) | Rvalue::Repeat(op, _) => is_op_disjoint(op), + Rvalue::Aggregate(_, ops) => ops.iter().all(|op| is_op_disjoint(op)), + Rvalue::Ref(..) + | Rvalue::ThreadLocalRef(..) + | Rvalue::AddressOf(..) + | Rvalue::Len(..) + | Rvalue::Cast(..) + | Rvalue::BinaryOp(..) + | Rvalue::CheckedBinaryOp(..) + | Rvalue::NullaryOp(..) + | Rvalue::UnaryOp(..) + | Rvalue::Discriminant(..) + | Rvalue::ShallowInitBox(..) => true, + }; + + if !ok_as_is { + // We need to introduce a temporary + let new_decl = LocalDecl::new(stmt.0.ty(locals, tcx).ty, source.span); + let temp: Place<'tcx> = locals.push(new_decl).into(); + let new_statement = Statement { + source_info: source, + kind: StatementKind::Assign(Box::new((stmt.0, Rvalue::Use(Operand::Move(temp))))), + }; + stmt.0 = temp; + Some(new_statement) + } else { + None + } + } } impl<'tcx> Operand<'tcx> { diff --git a/compiler/rustc_mir_transform/src/instcombine.rs b/compiler/rustc_mir_transform/src/instcombine.rs index 4fbb764337863..d3b70115b05aa 100644 --- a/compiler/rustc_mir_transform/src/instcombine.rs +++ b/compiler/rustc_mir_transform/src/instcombine.rs @@ -19,16 +19,31 @@ impl<'tcx> MirPass<'tcx> for InstCombine { let (basic_blocks, local_decls) = body.basic_blocks_and_local_decls_mut(); let ctx = InstCombineContext { tcx, local_decls }; for block in basic_blocks.iter_mut() { - for statement in block.statements.iter_mut() { - match statement.kind { - StatementKind::Assign(box (_place, ref mut rvalue)) => { - ctx.combine_bool_cmp(&statement.source_info, rvalue); - ctx.combine_ref_deref(&statement.source_info, rvalue); - ctx.combine_len(&statement.source_info, rvalue); + let mut new_statements = Vec::new(); + for mut statement in std::mem::take(&mut block.statements) { + let extra_statement = match &mut statement.kind { + StatementKind::Assign(assign) => { + let rvalue = &mut assign.1; + let changed = ctx.combine_bool_cmp(&statement.source_info, rvalue) + || ctx.combine_ref_deref(&statement.source_info, rvalue) + || ctx.combine_len(&statement.source_info, rvalue); + if changed { + Rvalue::expand_assign( + assign, + statement.source_info, + ctx.local_decls, + tcx, + ) + } else { + None + } } - _ => {} - } + _ => None, + }; + new_statements.push(statement); + new_statements.extend(extra_statement); } + block.statements = new_statements; ctx.combine_primitive_clone( &mut block.terminator.as_mut().unwrap(), @@ -40,7 +55,7 @@ impl<'tcx> MirPass<'tcx> for InstCombine { struct InstCombineContext<'tcx, 'a> { tcx: TyCtxt<'tcx>, - local_decls: &'a LocalDecls<'tcx>, + local_decls: &'a mut LocalDecls<'tcx>, } impl<'tcx> InstCombineContext<'tcx, '_> { @@ -51,7 +66,7 @@ impl<'tcx> InstCombineContext<'tcx, '_> { } /// Transform boolean comparisons into logical operations. - fn combine_bool_cmp(&self, source_info: &SourceInfo, rvalue: &mut Rvalue<'tcx>) { + fn combine_bool_cmp(&self, source_info: &SourceInfo, rvalue: &mut Rvalue<'tcx>) -> bool { match rvalue { Rvalue::BinaryOp(op @ (BinOp::Eq | BinOp::Ne), box (a, b)) => { let new = match (op, self.try_eval_bool(a), self.try_eval_bool(b)) { @@ -84,10 +99,13 @@ impl<'tcx> InstCombineContext<'tcx, '_> { if let Some(new) = new && self.should_combine(source_info, rvalue) { *rvalue = new; + true + } else { + false } } - _ => {} + _ => false, } } @@ -97,7 +115,7 @@ impl<'tcx> InstCombineContext<'tcx, '_> { } /// Transform "&(*a)" ==> "a". - fn combine_ref_deref(&self, source_info: &SourceInfo, rvalue: &mut Rvalue<'tcx>) { + fn combine_ref_deref(&self, source_info: &SourceInfo, rvalue: &mut Rvalue<'tcx>) -> bool { if let Rvalue::Ref(_, _, place) = rvalue { if let Some((base, ProjectionElem::Deref)) = place.as_ref().last_projection() { if let ty::Ref(_, _, Mutability::Not) = @@ -105,35 +123,39 @@ impl<'tcx> InstCombineContext<'tcx, '_> { { // The dereferenced place must have type `&_`, so that we don't copy `&mut _`. } else { - return; + return false; } if !self.should_combine(source_info, rvalue) { - return; + return false; } *rvalue = Rvalue::Use(Operand::Copy(Place { local: base.local, projection: self.tcx.intern_place_elems(base.projection), })); + return true; } } + false } /// Transform "Len([_; N])" ==> "N". - fn combine_len(&self, source_info: &SourceInfo, rvalue: &mut Rvalue<'tcx>) { + fn combine_len(&self, source_info: &SourceInfo, rvalue: &mut Rvalue<'tcx>) -> bool { if let Rvalue::Len(ref place) = *rvalue { let place_ty = place.ty(self.local_decls, self.tcx).ty; if let ty::Array(_, len) = *place_ty.kind() { if !self.should_combine(source_info, rvalue) { - return; + return false; } let constant = Constant { span: source_info.span, literal: len.into(), user_ty: None }; *rvalue = Rvalue::Use(Operand::Constant(Box::new(constant))); + return true; } } + false } fn combine_primitive_clone(