From ca3d3c4782000999ec59d0d4210ffdc74d288d04 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 2 Jul 2025 12:33:37 +0000 Subject: [PATCH 1/4] Make Value Copy by arena-allocating vectors. --- compiler/rustc_mir_transform/src/gvn.rs | 115 ++++++++++++------------ 1 file changed, 59 insertions(+), 56 deletions(-) diff --git a/compiler/rustc_mir_transform/src/gvn.rs b/compiler/rustc_mir_transform/src/gvn.rs index 5a13394543b4a..0769382f06bda 100644 --- a/compiler/rustc_mir_transform/src/gvn.rs +++ b/compiler/rustc_mir_transform/src/gvn.rs @@ -88,6 +88,7 @@ use std::borrow::Cow; use either::Either; use rustc_abi::{self as abi, BackendRepr, FIRST_VARIANT, FieldIdx, Primitive, Size, VariantIdx}; +use rustc_arena::DroplessArena; use rustc_const_eval::const_eval::DummyMachine; use rustc_const_eval::interpret::{ ImmTy, Immediate, InterpCx, MemPlaceMeta, MemoryKind, OpTy, Projectable, Scalar, @@ -126,7 +127,9 @@ impl<'tcx> crate::MirPass<'tcx> for GVN { // Clone dominators because we need them while mutating the body. let dominators = body.basic_blocks.dominators().clone(); - let mut state = VnState::new(tcx, body, typing_env, &ssa, dominators, &body.local_decls); + let arena = DroplessArena::default(); + let mut state = + VnState::new(tcx, body, typing_env, &ssa, dominators, &body.local_decls, &arena); for local in body.args_iter().filter(|&local| ssa.is_ssa(local)) { let opaque = state.new_opaque(body.local_decls[local].ty); @@ -160,8 +163,8 @@ enum AddressKind { Address(RawPtrKind), } -#[derive(Debug, PartialEq, Eq, Hash)] -enum Value<'tcx> { +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +enum Value<'a, 'tcx> { // Root values. /// Used to represent values we know nothing about. /// The `usize` is a counter incremented by `new_opaque`. @@ -176,7 +179,7 @@ enum Value<'tcx> { }, /// An aggregate value, either tuple/closure/struct/enum. /// This does not contain unions, as we cannot reason with the value. - Aggregate(VariantIdx, Vec), + Aggregate(VariantIdx, &'a [VnIndex]), /// A raw pointer aggregate built from a thin pointer and metadata. RawPtr { /// Thin pointer component. This is field 0 in MIR. @@ -212,7 +215,7 @@ enum Value<'tcx> { }, } -struct VnState<'body, 'tcx> { +struct VnState<'body, 'a, 'tcx> { tcx: TyCtxt<'tcx>, ecx: InterpCx<'tcx, DummyMachine>, local_decls: &'body LocalDecls<'tcx>, @@ -222,7 +225,7 @@ struct VnState<'body, 'tcx> { /// Locals that are assigned that value. // This vector does not hold all the values of `VnIndex` that we create. rev_locals: IndexVec>, - values: FxIndexSet<(Value<'tcx>, Ty<'tcx>)>, + values: FxIndexSet<(Value<'a, 'tcx>, Ty<'tcx>)>, /// Values evaluated as constants if possible. evaluated: IndexVec>>, /// Counter to generate different values. @@ -232,9 +235,10 @@ struct VnState<'body, 'tcx> { ssa: &'body SsaLocals, dominators: Dominators, reused_locals: DenseBitSet, + arena: &'a DroplessArena, } -impl<'body, 'tcx> VnState<'body, 'tcx> { +impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> { fn new( tcx: TyCtxt<'tcx>, body: &Body<'tcx>, @@ -242,6 +246,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { ssa: &'body SsaLocals, dominators: Dominators, local_decls: &'body LocalDecls<'tcx>, + arena: &'a DroplessArena, ) -> Self { // Compute a rough estimate of the number of values in the body from the number of // statements. This is meant to reduce the number of allocations, but it's all right if @@ -264,6 +269,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { ssa, dominators, reused_locals: DenseBitSet::new_empty(local_decls.len()), + arena, } } @@ -272,7 +278,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { } #[instrument(level = "trace", skip(self), ret)] - fn insert(&mut self, ty: Ty<'tcx>, value: Value<'tcx>) -> VnIndex { + fn insert(&mut self, ty: Ty<'tcx>, value: Value<'a, 'tcx>) -> VnIndex { let (index, new) = self.values.insert_full((value, ty)); let index = VnIndex::from_usize(index); if new { @@ -315,8 +321,8 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { } #[inline] - fn get(&self, index: VnIndex) -> &Value<'tcx> { - &self.values.get_index(index.as_usize()).unwrap().0 + fn get(&self, index: VnIndex) -> Value<'a, 'tcx> { + self.values.get_index(index.as_usize()).unwrap().0 } #[inline] @@ -361,8 +367,8 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { self.insert(ty, Value::Constant { value, disambiguator: 0 }) } - fn insert_tuple(&mut self, ty: Ty<'tcx>, values: Vec) -> VnIndex { - self.insert(ty, Value::Aggregate(VariantIdx::ZERO, values)) + fn insert_tuple(&mut self, ty: Ty<'tcx>, values: &[VnIndex]) -> VnIndex { + self.insert(ty, Value::Aggregate(VariantIdx::ZERO, self.arena.alloc_slice(values))) } fn insert_deref(&mut self, ty: Ty<'tcx>, value: VnIndex) -> VnIndex { @@ -388,7 +394,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { } else { return None; }; - let op = match *self.get(value) { + let op = match self.get(value) { _ if ty.is_zst() => ImmTy::uninit(ty).into(), Opaque(_) => return None, @@ -608,7 +614,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { if let Value::Aggregate(_, fields) = self.get(value) { return Some((projection_ty, fields[f.as_usize()])); } else if let Value::Projection(outer_value, ProjectionElem::Downcast(_, read_variant)) = self.get(value) - && let Value::Aggregate(written_variant, fields) = self.get(*outer_value) + && let Value::Aggregate(written_variant, fields) = self.get(outer_value) // This pass is not aware of control-flow, so we do not know whether the // replacement we are doing is actually reachable. We could be in any arm of // ``` @@ -633,7 +639,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { ProjectionElem::Index(idx) => { if let Value::Repeat(inner, _) = self.get(value) { *from_non_ssa_index |= self.locals[idx].is_none(); - return Some((projection_ty, *inner)); + return Some((projection_ty, inner)); } let idx = self.locals[idx]?; ProjectionElem::Index(idx) @@ -641,7 +647,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { ProjectionElem::ConstantIndex { offset, min_length, from_end } => { match self.get(value) { Value::Repeat(inner, _) => { - return Some((projection_ty, *inner)); + return Some((projection_ty, inner)); } Value::Aggregate(_, operands) => { let offset = if from_end { @@ -731,8 +737,8 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { let mut place_ty = PlaceTy::from_ty(self.local_decls[place.local].ty); let mut from_non_ssa_index = false; for (index, proj) in place.projection.iter().enumerate() { - if let Value::Projection(pointer, ProjectionElem::Deref) = *self.get(value) - && let Value::Address { place: mut pointee, kind, .. } = *self.get(pointer) + if let Value::Projection(pointer, ProjectionElem::Deref) = self.get(value) + && let Value::Address { place: mut pointee, kind, .. } = self.get(pointer) && let AddressKind::Ref(BorrowKind::Shared) = kind && let Some(v) = self.simplify_place_value(&mut pointee, location) { @@ -755,8 +761,8 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { (place_ty, value) = self.project(place_ty, value, proj, &mut from_non_ssa_index)?; } - if let Value::Projection(pointer, ProjectionElem::Deref) = *self.get(value) - && let Value::Address { place: mut pointee, kind, .. } = *self.get(pointer) + if let Value::Projection(pointer, ProjectionElem::Deref) = self.get(value) + && let Value::Address { place: mut pointee, kind, .. } = self.get(pointer) && let AddressKind::Ref(BorrowKind::Shared) = kind && let Some(v) = self.simplify_place_value(&mut pointee, location) { @@ -868,7 +874,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { fn simplify_discriminant(&mut self, place: VnIndex) -> Option { let enum_ty = self.ty(place); if enum_ty.is_enum() - && let Value::Aggregate(variant, _) = *self.get(place) + && let Value::Aggregate(variant, _) = self.get(place) { let discr = self.ecx.discriminant_for_variant(enum_ty, variant).discard_err()?; return Some(self.insert_scalar(discr.layout.ty, discr.to_scalar())); @@ -904,12 +910,12 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { let Some(&first_field) = fields.first() else { return None; }; - let Value::Projection(copy_from_value, _) = *self.get(first_field) else { + let Value::Projection(copy_from_value, _) = self.get(first_field) else { return None; }; // All fields must correspond one-to-one and come from the same aggregate value. if fields.iter().enumerate().any(|(index, &v)| { - if let Value::Projection(pointer, ProjectionElem::Field(from_index, _)) = *self.get(v) + if let Value::Projection(pointer, ProjectionElem::Field(from_index, _)) = self.get(v) && copy_from_value == pointer && from_index.index() == index { @@ -921,7 +927,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { } let mut copy_from_local_value = copy_from_value; - if let Value::Projection(pointer, proj) = *self.get(copy_from_value) + if let Value::Projection(pointer, proj) = self.get(copy_from_value) && let ProjectionElem::Downcast(_, read_variant) = proj { if variant_index == read_variant { @@ -979,13 +985,10 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { } } - let fields: Vec<_> = field_ops - .iter_mut() - .map(|op| { - self.simplify_operand(op, location) - .unwrap_or_else(|| self.new_opaque(op.ty(self.local_decls, self.tcx))) - }) - .collect(); + let fields = self.arena.alloc_from_iter(field_ops.iter_mut().map(|op| { + self.simplify_operand(op, location) + .unwrap_or_else(|| self.new_opaque(op.ty(self.local_decls, self.tcx))) + })); let variant_index = match *kind { AggregateKind::Array(..) | AggregateKind::Tuple => { @@ -1006,12 +1009,12 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { let mut was_updated = false; while let Value::Cast { kind: CastKind::PtrToPtr, value: cast_value } = self.get(pointer) - && let ty::RawPtr(from_pointee_ty, from_mtbl) = self.ty(*cast_value).kind() + && let ty::RawPtr(from_pointee_ty, from_mtbl) = self.ty(cast_value).kind() && let ty::RawPtr(_, output_mtbl) = ty.kind() && from_mtbl == output_mtbl && from_pointee_ty.is_sized(self.tcx, self.typing_env()) { - pointer = *cast_value; + pointer = cast_value; was_updated = true; } @@ -1069,9 +1072,9 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { // To allow things like `*mut (?A, ?T)` <-> `*mut (?B, ?T)`, // it's fine to get a projection as the type. Value::Cast { kind: CastKind::PtrToPtr, value: inner } - if self.pointers_have_same_metadata(self.ty(*inner), arg_ty) => + if self.pointers_have_same_metadata(self.ty(inner), arg_ty) => { - arg_index = *inner; + arg_index = inner; was_updated = true; continue; } @@ -1098,15 +1101,15 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { } let value = match (op, self.get(arg_index)) { - (UnOp::Not, Value::UnaryOp(UnOp::Not, inner)) => return Some(*inner), - (UnOp::Neg, Value::UnaryOp(UnOp::Neg, inner)) => return Some(*inner), + (UnOp::Not, Value::UnaryOp(UnOp::Not, inner)) => return Some(inner), + (UnOp::Neg, Value::UnaryOp(UnOp::Neg, inner)) => return Some(inner), (UnOp::Not, Value::BinaryOp(BinOp::Eq, lhs, rhs)) => { - Value::BinaryOp(BinOp::Ne, *lhs, *rhs) + Value::BinaryOp(BinOp::Ne, lhs, rhs) } (UnOp::Not, Value::BinaryOp(BinOp::Ne, lhs, rhs)) => { - Value::BinaryOp(BinOp::Eq, *lhs, *rhs) + Value::BinaryOp(BinOp::Eq, lhs, rhs) } - (UnOp::PtrMetadata, Value::RawPtr { metadata, .. }) => return Some(*metadata), + (UnOp::PtrMetadata, Value::RawPtr { metadata, .. }) => return Some(metadata), // We have an unsizing cast, which assigns the length to wide pointer metadata. ( UnOp::PtrMetadata, @@ -1115,7 +1118,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { value: inner, }, ) if let ty::Slice(..) = arg_ty.builtin_deref(true).unwrap().kind() - && let ty::Array(_, len) = self.ty(*inner).builtin_deref(true).unwrap().kind() => + && let ty::Array(_, len) = self.ty(inner).builtin_deref(true).unwrap().kind() => { return Some(self.insert_constant(Const::Ty(self.tcx.types.usize, *len))); } @@ -1148,12 +1151,12 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { && lhs_ty.is_any_ptr() && let Value::Cast { kind: CastKind::PtrToPtr, value: lhs_value } = self.get(lhs) && let Value::Cast { kind: CastKind::PtrToPtr, value: rhs_value } = self.get(rhs) - && let lhs_from = self.ty(*lhs_value) - && lhs_from == self.ty(*rhs_value) + && let lhs_from = self.ty(lhs_value) + && lhs_from == self.ty(rhs_value) && self.pointers_have_same_metadata(lhs_from, lhs_ty) { - lhs = *lhs_value; - rhs = *rhs_value; + lhs = lhs_value; + rhs = rhs_value; if let Some(lhs_op) = self.try_as_operand(lhs, location) && let Some(rhs_op) = self.try_as_operand(rhs, location) { @@ -1287,7 +1290,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { if op.is_overflowing() { let ty = Ty::new_tup(self.tcx, &[self.ty(result), self.tcx.types.bool]); let false_val = self.insert_bool(false); - Some(self.insert_tuple(ty, vec![result, false_val])) + Some(self.insert_tuple(ty, &[result, false_val])) } else { Some(result) } @@ -1340,11 +1343,11 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { && let ty::RawPtr(to_pointee, _) = to.kind() && to_pointee.is_sized(self.tcx, self.typing_env()) { - from = self.ty(*pointer); - value = *pointer; + from = self.ty(pointer); + value = pointer; was_updated_this_iteration = true; if from == to { - return Some(*pointer); + return Some(pointer); } } @@ -1353,7 +1356,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { if let Transmute = kind && let Value::Aggregate(variant_idx, field_values) = self.get(value) && let Some((field_idx, field_ty)) = - self.value_is_all_in_one_field(from, *variant_idx) + self.value_is_all_in_one_field(from, variant_idx) { from = field_ty; value = field_values[field_idx.as_usize()]; @@ -1364,7 +1367,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { } // Various cast-then-cast cases can be simplified. - if let Value::Cast { kind: inner_kind, value: inner_value } = *self.get(value) { + if let Value::Cast { kind: inner_kind, value: inner_value } = self.get(value) { let inner_from = self.ty(inner_value); let new_kind = match (inner_kind, kind) { // Even if there's a narrowing cast in here that's fine, because @@ -1438,7 +1441,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> { // We have an unsizing cast, which assigns the length to wide pointer metadata. if let Value::Cast { kind, value: from } = self.get(inner) && let CastKind::PointerCoercion(ty::adjustment::PointerCoercion::Unsize, _) = kind - && let Some(from) = self.ty(*from).builtin_deref(true) + && let Some(from) = self.ty(from).builtin_deref(true) && let ty::Array(_, len) = from.kind() && let Some(to) = self.ty(inner).builtin_deref(true) && let ty::Slice(..) = to.kind() @@ -1596,7 +1599,7 @@ fn op_to_prop_const<'tcx>( None } -impl<'tcx> VnState<'_, 'tcx> { +impl<'tcx> VnState<'_, '_, 'tcx> { /// If either [`Self::try_as_constant`] as [`Self::try_as_place`] succeeds, /// returns that result as an [`Operand`]. fn try_as_operand(&mut self, index: VnIndex, location: Location) -> Option> { @@ -1615,7 +1618,7 @@ impl<'tcx> VnState<'_, 'tcx> { // This was already constant in MIR, do not change it. If the constant is not // deterministic, adding an additional mention of it in MIR will not give the same value as // the former mention. - if let Value::Constant { value, disambiguator: 0 } = *self.get(index) { + if let Value::Constant { value, disambiguator: 0 } = self.get(index) { debug_assert!(value.is_deterministic()); return Some(ConstOperand { span: DUMMY_SP, user_ty: None, const_: value }); } @@ -1654,7 +1657,7 @@ impl<'tcx> VnState<'_, 'tcx> { let place = Place { local, projection: self.tcx.mk_place_elems(projection.as_slice()) }; return Some(place); - } else if let Value::Projection(pointer, proj) = *self.get(index) + } else if let Value::Projection(pointer, proj) = self.get(index) && (allow_complex_projection || proj.is_stable_offset()) && let Some(proj) = self.try_as_place_elem(self.ty(index), proj, loc) { @@ -1677,7 +1680,7 @@ impl<'tcx> VnState<'_, 'tcx> { } } -impl<'tcx> MutVisitor<'tcx> for VnState<'_, 'tcx> { +impl<'tcx> MutVisitor<'tcx> for VnState<'_, '_, 'tcx> { fn tcx(&self) -> TyCtxt<'tcx> { self.tcx } From 87eb56cfcb90ccfd0d381b16dbacc41e10f74128 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 1 Jul 2025 10:01:12 +0000 Subject: [PATCH 2/4] Use a VnIndex in Address projection. --- compiler/rustc_mir_transform/src/gvn.rs | 203 +++++++++++------- ....dereference_indexing.GVN.panic-abort.diff | 3 +- ...dereference_indexing.GVN.panic-unwind.diff | 3 +- tests/mir-opt/gvn.rs | 4 +- tests/mir-opt/gvn_repeat.index_place.GVN.diff | 15 ++ .../mir-opt/gvn_repeat.repeat_local.GVN.diff | 3 +- tests/mir-opt/gvn_repeat.rs | 21 +- 7 files changed, 173 insertions(+), 79 deletions(-) create mode 100644 tests/mir-opt/gvn_repeat.index_place.GVN.diff diff --git a/compiler/rustc_mir_transform/src/gvn.rs b/compiler/rustc_mir_transform/src/gvn.rs index 0769382f06bda..5392e088d0cde 100644 --- a/compiler/rustc_mir_transform/src/gvn.rs +++ b/compiler/rustc_mir_transform/src/gvn.rs @@ -163,6 +163,14 @@ enum AddressKind { Address(RawPtrKind), } +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +enum AddressBase { + /// This address is based on this local. + Local(Local), + /// This address is based on the deref of this pointer. + Deref(VnIndex), +} + #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] enum Value<'a, 'tcx> { // Root values. @@ -191,7 +199,10 @@ enum Value<'a, 'tcx> { Repeat(VnIndex, ty::Const<'tcx>), /// The address of a place. Address { - place: Place<'tcx>, + base: AddressBase, + // We do not use a plain `Place` as we want to be able to reason about indices. + // This does not contain any `Deref` projection. + projection: &'a [ProjectionElem>], kind: AddressKind, /// Give each borrow and pointer a different provenance, so we don't merge them. provenance: usize, @@ -308,7 +319,12 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> { /// Create a new `Value::Address` distinct from all the others. #[instrument(level = "trace", skip(self), ret)] - fn new_pointer(&mut self, place: Place<'tcx>, kind: AddressKind) -> VnIndex { + fn new_pointer( + &mut self, + place: Place<'tcx>, + kind: AddressKind, + location: Location, + ) -> Option { let pty = place.ty(self.local_decls, self.tcx).ty; let ty = match kind { AddressKind::Ref(bk) => { @@ -316,8 +332,25 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> { } AddressKind::Address(mutbl) => Ty::new_ptr(self.tcx, pty, mutbl.to_mutbl_lossy()), }; - let value = Value::Address { place, kind, provenance: self.next_opaque() }; - self.insert(ty, value) + + let mut projection = place.projection.iter(); + let base = if place.is_indirect_first_projection() { + let base = self.locals[place.local]?; + // Skip the initial `Deref`. + projection.next(); + AddressBase::Deref(base) + } else { + AddressBase::Local(place.local) + }; + // Do not try evaluating inside `Index`, this has been done by `simplify_place_value`. + let projection = self + .arena + .try_alloc_from_iter( + projection.map(|proj| proj.try_map(|value| self.locals[value], |ty| ty).ok_or(())), + ) + .ok()?; + let value = Value::Address { base, projection, kind, provenance: self.next_opaque() }; + Some(self.insert(ty, value)) } #[inline] @@ -458,14 +491,15 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> { let elem = elem.try_map(|_| None, |()| ty.ty)?; self.ecx.project(base, elem).discard_err()? } - Address { place, kind: _, provenance: _ } => { - if !place.is_indirect_first_projection() { - return None; - } - let local = self.locals[place.local]?; - let pointer = self.evaluated[local].as_ref()?; + Address { base, projection, .. } => { + debug_assert!(!projection.contains(&ProjectionElem::Deref)); + let pointer = match base { + AddressBase::Deref(pointer) => self.evaluated[pointer].as_ref()?, + // We have no stack to point to. + AddressBase::Local(_) => return None, + }; let mut mplace = self.ecx.deref_pointer(pointer).discard_err()?; - for elem in place.projection.iter().skip(1) { + for elem in projection { // `Index` by constants should have been replaced by `ConstantIndex` by // `simplify_place_projection`. let elem = elem.try_map(|_| None, |ty| ty)?; @@ -589,12 +623,38 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> { Some(op) } + /// Represent the *value* we obtain by dereferencing an `Address` value. + #[instrument(level = "trace", skip(self), ret)] + fn dereference_address( + &mut self, + base: AddressBase, + projection: &[ProjectionElem>], + ) -> Option { + let (mut place_ty, mut value) = match base { + // The base is a local, so we take the local's value and project from it. + AddressBase::Local(local) => { + let local = self.locals[local]?; + let place_ty = PlaceTy::from_ty(self.ty(local)); + (place_ty, local) + } + // The base is a pointer's deref, so we introduce the implicit deref. + AddressBase::Deref(reborrow) => { + let place_ty = PlaceTy::from_ty(self.ty(reborrow)); + self.project(place_ty, reborrow, ProjectionElem::Deref)? + } + }; + for &proj in projection { + (place_ty, value) = self.project(place_ty, value, proj)?; + } + Some(value) + } + + #[instrument(level = "trace", skip(self), ret)] fn project( &mut self, place_ty: PlaceTy<'tcx>, value: VnIndex, - proj: PlaceElem<'tcx>, - from_non_ssa_index: &mut bool, + proj: ProjectionElem>, ) -> Option<(PlaceTy<'tcx>, VnIndex)> { let projection_ty = place_ty.projection_ty(self.tcx, proj); let proj = match proj { @@ -602,6 +662,12 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> { if let Some(Mutability::Not) = place_ty.ty.ref_mutability() && projection_ty.ty.is_freeze(self.tcx, self.typing_env()) { + if let Value::Address { base, projection, .. } = self.get(value) + && let Some(value) = self.dereference_address(base, projection) + { + return Some((projection_ty, value)); + } + // An immutable borrow `_x` always points to the same value for the // lifetime of the borrow, so we can merge all instances of `*_x`. return Some((projection_ty, self.insert_deref(projection_ty.ty, value))); @@ -638,10 +704,8 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> { } ProjectionElem::Index(idx) => { if let Value::Repeat(inner, _) = self.get(value) { - *from_non_ssa_index |= self.locals[idx].is_none(); return Some((projection_ty, inner)); } - let idx = self.locals[idx]?; ProjectionElem::Index(idx) } ProjectionElem::ConstantIndex { offset, min_length, from_end } => { @@ -720,37 +784,20 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> { /// Represent the *value* which would be read from `place`, and point `place` to a preexisting /// place with the same value (if that already exists). #[instrument(level = "trace", skip(self), ret)] - fn simplify_place_value( + fn compute_place_value( &mut self, - place: &mut Place<'tcx>, + place: Place<'tcx>, location: Location, - ) -> Option { - self.simplify_place_projection(place, location); - + ) -> Result> { // Invariant: `place` and `place_ref` point to the same value, even if they point to // different memory locations. let mut place_ref = place.as_ref(); // Invariant: `value` holds the value up-to the `index`th projection excluded. - let mut value = self.locals[place.local]?; + let Some(mut value) = self.locals[place.local] else { return Err(place_ref) }; // Invariant: `value` has type `place_ty`, with optional downcast variant if needed. let mut place_ty = PlaceTy::from_ty(self.local_decls[place.local].ty); - let mut from_non_ssa_index = false; for (index, proj) in place.projection.iter().enumerate() { - if let Value::Projection(pointer, ProjectionElem::Deref) = self.get(value) - && let Value::Address { place: mut pointee, kind, .. } = self.get(pointer) - && let AddressKind::Ref(BorrowKind::Shared) = kind - && let Some(v) = self.simplify_place_value(&mut pointee, location) - { - value = v; - // `pointee` holds a `Place`, so `ProjectionElem::Index` holds a `Local`. - // That local is SSA, but we otherwise have no guarantee on that local's value at - // the current location compared to its value where `pointee` was borrowed. - if pointee.projection.iter().all(|elem| !matches!(elem, ProjectionElem::Index(_))) { - place_ref = - pointee.project_deeper(&place.projection[index..], self.tcx).as_ref(); - } - } if let Some(local) = self.try_as_local(value, location) { // Both `local` and `Place { local: place.local, projection: projection[..index] }` // hold the same value. Therefore, following place holds the value in the original @@ -758,36 +805,50 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> { place_ref = PlaceRef { local, projection: &place.projection[index..] }; } - (place_ty, value) = self.project(place_ty, value, proj, &mut from_non_ssa_index)?; + let Some(proj) = proj.try_map(|value| self.locals[value], |ty| ty) else { + return Err(place_ref); + }; + let Some(ty_and_value) = self.project(place_ty, value, proj) else { + return Err(place_ref); + }; + (place_ty, value) = ty_and_value; } - if let Value::Projection(pointer, ProjectionElem::Deref) = self.get(value) - && let Value::Address { place: mut pointee, kind, .. } = self.get(pointer) - && let AddressKind::Ref(BorrowKind::Shared) = kind - && let Some(v) = self.simplify_place_value(&mut pointee, location) - { - value = v; - // `pointee` holds a `Place`, so `ProjectionElem::Index` holds a `Local`. - // That local is SSA, but we otherwise have no guarantee on that local's value at - // the current location compared to its value where `pointee` was borrowed. - if pointee.projection.iter().all(|elem| !matches!(elem, ProjectionElem::Index(_))) { - place_ref = pointee.project_deeper(&[], self.tcx).as_ref(); - } - } - if let Some(new_local) = self.try_as_local(value, location) { - place_ref = PlaceRef { local: new_local, projection: &[] }; - } else if from_non_ssa_index { - // If access to non-SSA locals is unavoidable, bail out. - return None; - } + Ok(value) + } - if place_ref.local != place.local || place_ref.projection.len() < place.projection.len() { - // By the invariant on `place_ref`. - *place = place_ref.project_deeper(&[], self.tcx); - self.reused_locals.insert(place_ref.local); - } + /// Represent the *value* which would be read from `place`, and point `place` to a preexisting + /// place with the same value (if that already exists). + #[instrument(level = "trace", skip(self), ret)] + fn simplify_place_value( + &mut self, + place: &mut Place<'tcx>, + location: Location, + ) -> Option { + self.simplify_place_projection(place, location); - Some(value) + match self.compute_place_value(*place, location) { + Ok(value) => { + if let Some(new_place) = self.try_as_place(value, location, true) + && (new_place.local != place.local + || new_place.projection.len() < place.projection.len()) + { + *place = new_place; + self.reused_locals.insert(new_place.local); + } + Some(value) + } + Err(place_ref) => { + if place_ref.local != place.local + || place_ref.projection.len() < place.projection.len() + { + // By the invariant on `place_ref`. + *place = place_ref.project_deeper(&[], self.tcx); + self.reused_locals.insert(place_ref.local); + } + None + } + } } #[instrument(level = "trace", skip(self), ret)] @@ -834,11 +895,11 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> { Rvalue::Aggregate(..) => return self.simplify_aggregate(lhs, rvalue, location), Rvalue::Ref(_, borrow_kind, ref mut place) => { self.simplify_place_projection(place, location); - return Some(self.new_pointer(*place, AddressKind::Ref(borrow_kind))); + return self.new_pointer(*place, AddressKind::Ref(borrow_kind), location); } Rvalue::RawPtr(mutbl, ref mut place) => { self.simplify_place_projection(place, location); - return Some(self.new_pointer(*place, AddressKind::Address(mutbl))); + return self.new_pointer(*place, AddressKind::Address(mutbl), location); } Rvalue::WrapUnsafeBinder(ref mut op, _) => { let value = self.simplify_operand(op, location)?; @@ -1080,12 +1141,10 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> { } // `&mut *p`, `&raw *p`, etc don't change metadata. - Value::Address { place, kind: _, provenance: _ } - if let PlaceRef { local, projection: [PlaceElem::Deref] } = - place.as_ref() - && let Some(local_index) = self.locals[local] => + Value::Address { base: AddressBase::Deref(reborrowed), projection, .. } + if projection.is_empty() => { - arg_index = local_index; + arg_index = reborrowed; was_updated = true; continue; } @@ -1431,9 +1490,9 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> { // The length information is stored in the wide pointer. // Reborrowing copies length information from one pointer to the other. - while let Value::Address { place: borrowed, .. } = self.get(inner) - && let [PlaceElem::Deref] = borrowed.projection[..] - && let Some(borrowed) = self.locals[borrowed.local] + while let Value::Address { base: AddressBase::Deref(borrowed), projection, .. } = + self.get(inner) + && projection.is_empty() { inner = borrowed; } diff --git a/tests/mir-opt/gvn.dereference_indexing.GVN.panic-abort.diff b/tests/mir-opt/gvn.dereference_indexing.GVN.panic-abort.diff index 9bdcc2f108a6a..1a07638cbafea 100644 --- a/tests/mir-opt/gvn.dereference_indexing.GVN.panic-abort.diff +++ b/tests/mir-opt/gvn.dereference_indexing.GVN.panic-abort.diff @@ -43,7 +43,8 @@ + nop; StorageLive(_8); StorageLive(_9); - _9 = copy (*_3); +- _9 = copy (*_3); ++ _9 = copy _1[_4]; _8 = opaque::(move _9) -> [return: bb2, unwind unreachable]; } diff --git a/tests/mir-opt/gvn.dereference_indexing.GVN.panic-unwind.diff b/tests/mir-opt/gvn.dereference_indexing.GVN.panic-unwind.diff index f38bc51adc480..6e67b66e783c4 100644 --- a/tests/mir-opt/gvn.dereference_indexing.GVN.panic-unwind.diff +++ b/tests/mir-opt/gvn.dereference_indexing.GVN.panic-unwind.diff @@ -43,7 +43,8 @@ + nop; StorageLive(_8); StorageLive(_9); - _9 = copy (*_3); +- _9 = copy (*_3); ++ _9 = copy _1[_4]; _8 = opaque::(move _9) -> [return: bb2, unwind continue]; } diff --git a/tests/mir-opt/gvn.rs b/tests/mir-opt/gvn.rs index 98f94fbf0b146..3c3241fefe22e 100644 --- a/tests/mir-opt/gvn.rs +++ b/tests/mir-opt/gvn.rs @@ -1065,8 +1065,8 @@ fn dereference_indexing(array: [u8; 2], index: usize) { &array[i] }; - // CHECK-NOT: [{{.*}}] - // CHECK: [[tmp:_.*]] = copy (*[[a]]); + // CHECK-NOT: StorageDead([[i]]); + // CHECK: [[tmp:_.*]] = copy _1[[[i]]]; // CHECK: opaque::(move [[tmp]]) opaque(*a); } diff --git a/tests/mir-opt/gvn_repeat.index_place.GVN.diff b/tests/mir-opt/gvn_repeat.index_place.GVN.diff new file mode 100644 index 0000000000000..1eb7e9015cc3f --- /dev/null +++ b/tests/mir-opt/gvn_repeat.index_place.GVN.diff @@ -0,0 +1,15 @@ +- // MIR for `index_place` before GVN ++ // MIR for `index_place` after GVN + + fn index_place(_1: usize, _2: usize, _3: [i32; 5]) -> i32 { + let mut _0: i32; + let mut _4: &i32; + + bb0: { + _4 = &_3[_1]; + _1 = copy _2; + _0 = copy (*_4); + return; + } + } + diff --git a/tests/mir-opt/gvn_repeat.repeat_local.GVN.diff b/tests/mir-opt/gvn_repeat.repeat_local.GVN.diff index fd04782528117..eb3f885b8665b 100644 --- a/tests/mir-opt/gvn_repeat.repeat_local.GVN.diff +++ b/tests/mir-opt/gvn_repeat.repeat_local.GVN.diff @@ -10,8 +10,7 @@ _4 = [copy _3; 5]; _5 = &_4[_1]; _1 = copy _2; -- _0 = copy (*_5); -+ _0 = copy _3; + _0 = copy (*_5); return; } } diff --git a/tests/mir-opt/gvn_repeat.rs b/tests/mir-opt/gvn_repeat.rs index bbbb2a7ccbaf5..49364c6bfd232 100644 --- a/tests/mir-opt/gvn_repeat.rs +++ b/tests/mir-opt/gvn_repeat.rs @@ -6,6 +6,23 @@ use std::intrinsics::mir::*; +// EMIT_MIR gvn_repeat.index_place.GVN.diff +#[custom_mir(dialect = "runtime")] +pub fn index_place(mut idx1: usize, idx2: usize, array: [i32; 5]) -> i32 { + // CHECK-LABEL: fn index_place( + // CHECK: let mut [[ELEM:.*]]: &i32; + // CHECK: _0 = copy (*[[ELEM]]) + mir! { + let elem; + { + elem = &array[idx1]; + idx1 = idx2; + RET = *elem; + Return() + } + } +} + // EMIT_MIR gvn_repeat.repeat_place.GVN.diff #[custom_mir(dialect = "runtime")] pub fn repeat_place(mut idx1: usize, idx2: usize, val: &i32) -> i32 { @@ -29,7 +46,8 @@ pub fn repeat_place(mut idx1: usize, idx2: usize, val: &i32) -> i32 { #[custom_mir(dialect = "runtime")] pub fn repeat_local(mut idx1: usize, idx2: usize, val: i32) -> i32 { // CHECK-LABEL: fn repeat_local( - // CHECK: _0 = copy _3 + // CHECK: let mut [[ELEM:.*]]: &i32; + // CHECK: _0 = copy (*[[ELEM]]); mir! { let array; let elem; @@ -44,6 +62,7 @@ pub fn repeat_local(mut idx1: usize, idx2: usize, val: i32) -> i32 { } fn main() { + assert_eq!(index_place(0, 5, [0; 5]), 0); assert_eq!(repeat_place(0, 5, &0), 0); assert_eq!(repeat_local(0, 5, 0), 0); } From 7d9c3eec7730540e765793f8f30c6aa9e717c8c6 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Mon, 23 Jun 2025 16:05:09 +0000 Subject: [PATCH 3/4] Introduce PlaceContext::may_observe_address. --- compiler/rustc_middle/src/mir/visit.rs | 17 +++++++++++++++++ .../rustc_mir_dataflow/src/value_analysis.rs | 9 ++------- compiler/rustc_mir_transform/src/ssa.rs | 4 +++- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_middle/src/mir/visit.rs b/compiler/rustc_middle/src/mir/visit.rs index 904d78d69b61c..029a402ed1bf2 100644 --- a/compiler/rustc_middle/src/mir/visit.rs +++ b/compiler/rustc_middle/src/mir/visit.rs @@ -1415,6 +1415,23 @@ impl PlaceContext { ) } + /// Returns `true` if this place context may be used to know the address of the given place. + pub fn may_observe_address(self) -> bool { + matches!( + self, + PlaceContext::NonMutatingUse( + NonMutatingUseContext::SharedBorrow + | NonMutatingUseContext::RawBorrow + | NonMutatingUseContext::FakeBorrow + ) | PlaceContext::MutatingUse( + MutatingUseContext::Drop + | MutatingUseContext::Borrow + | MutatingUseContext::RawBorrow + | MutatingUseContext::AsmOutput + ) + ) + } + /// Returns `true` if this place context represents a storage live or storage dead marker. #[inline] pub fn is_storage_marker(self) -> bool { diff --git a/compiler/rustc_mir_dataflow/src/value_analysis.rs b/compiler/rustc_mir_dataflow/src/value_analysis.rs index 005e797313070..9a00831dc012b 100644 --- a/compiler/rustc_mir_dataflow/src/value_analysis.rs +++ b/compiler/rustc_mir_dataflow/src/value_analysis.rs @@ -6,7 +6,7 @@ use rustc_data_structures::fx::{FxHashMap, FxIndexSet, StdEntry}; use rustc_data_structures::stack::ensure_sufficient_stack; use rustc_index::IndexVec; use rustc_index::bit_set::DenseBitSet; -use rustc_middle::mir::visit::{MutatingUseContext, PlaceContext, Visitor}; +use rustc_middle::mir::visit::{PlaceContext, Visitor}; use rustc_middle::mir::*; use rustc_middle::ty::{self, Ty, TyCtxt}; use tracing::debug; @@ -917,12 +917,7 @@ pub fn excluded_locals(body: &Body<'_>) -> DenseBitSet { impl<'tcx> Visitor<'tcx> for Collector { fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, _location: Location) { - if (context.is_borrow() - || context.is_address_of() - || context.is_drop() - || context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput)) - && !place.is_indirect() - { + if context.may_observe_address() && !place.is_indirect() { // A pointer to a place could be used to access other places with the same local, // hence we have to exclude the local completely. self.result.insert(place.local); diff --git a/compiler/rustc_mir_transform/src/ssa.rs b/compiler/rustc_mir_transform/src/ssa.rs index cd9a7f4a39dfe..73c249a3c8cdb 100644 --- a/compiler/rustc_mir_transform/src/ssa.rs +++ b/compiler/rustc_mir_transform/src/ssa.rs @@ -225,6 +225,9 @@ impl SsaVisitor<'_, '_> { impl<'tcx> Visitor<'tcx> for SsaVisitor<'_, 'tcx> { fn visit_local(&mut self, local: Local, ctxt: PlaceContext, loc: Location) { + if ctxt.may_observe_address() { + self.borrowed_locals.insert(local); + } match ctxt { PlaceContext::MutatingUse(MutatingUseContext::Projection) | PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection) => bug!(), @@ -237,7 +240,6 @@ impl<'tcx> Visitor<'tcx> for SsaVisitor<'_, 'tcx> { PlaceContext::NonMutatingUse( NonMutatingUseContext::SharedBorrow | NonMutatingUseContext::FakeBorrow, ) => { - self.borrowed_locals.insert(local); self.check_dominates(local, loc); self.direct_uses[local] += 1; } From 525d69c2555c1b3ffde45e21b9696d7ed380f696 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 2 Jul 2025 22:09:46 +0000 Subject: [PATCH 4/4] Ensure indirect is first projection in try_as_place. --- compiler/rustc_mir_transform/src/gvn.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/compiler/rustc_mir_transform/src/gvn.rs b/compiler/rustc_mir_transform/src/gvn.rs index 5392e088d0cde..4d7a27f612c3a 100644 --- a/compiler/rustc_mir_transform/src/gvn.rs +++ b/compiler/rustc_mir_transform/src/gvn.rs @@ -1716,6 +1716,11 @@ impl<'tcx> VnState<'_, '_, 'tcx> { let place = Place { local, projection: self.tcx.mk_place_elems(projection.as_slice()) }; return Some(place); + } else if projection.last() == Some(&PlaceElem::Deref) { + // `Deref` can only be the first projection in a place. + // If we are here, we failed to find a local, and we already have a `Deref`. + // Trying to add projections will only result in an ill-formed place. + return None; } else if let Value::Projection(pointer, proj) = self.get(index) && (allow_complex_projection || proj.is_stable_offset()) && let Some(proj) = self.try_as_place_elem(self.ty(index), proj, loc)