Skip to content

GVN: Avoid merging borrows from the dereferenced value #150270

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.

Sign up for GitHub

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

Open
dianqk wants to merge 1 commit into
base: main
Choose a base branch
from
+167 −121
Open

GVN: Avoid merging borrows from the dereferenced value #150270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 43 additions & 12 deletions compiler/rustc_mir_transform/src/gvn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,13 @@ enum Value<'a, 'tcx> {

// Extractions.
/// This is the *value* obtained by projecting another value.
Projection(VnIndex, ProjectionElem<VnIndex, ()>),
Projection {
base: VnIndex,
elem: ProjectionElem<VnIndex, ()>,
/// Some values may be a borrow or pointer.
/// Give them a different provenance, so we don't merge them.
provenance: Option<VnOpaque>,
},
/// Discriminant of the given value.
Discriminant(VnIndex),

Expand Down Expand Up @@ -292,6 +298,7 @@ impl<'a, 'tcx> ValueSet<'a, 'tcx> {
debug_assert!(match value {
Value::Opaque(_) | Value::Address { .. } => true,
Value::Constant { disambiguator, .. } => disambiguator.is_some(),
Value::Projection { provenance, .. } => provenance.is_some(),
_ => false,
});

Expand Down Expand Up @@ -542,9 +549,23 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
}

fn insert_deref(&mut self, ty: Ty<'tcx>, value: VnIndex) -> VnIndex {
let value = self.insert(ty, Value::Projection(value, ProjectionElem::Deref));
self.derefs.push(value);
value
let index = if ty.is_any_ptr() {
// Give each borrow and pointer a different provenance, so we don't merge them.
let index = self.insert_unique(ty, |provenance| Value::Projection {
base: value,
elem: ProjectionElem::Deref,
provenance: Some(provenance),
});
self.evaluated[index] = Some(None);
index
} else {
self.insert(
ty,
Value::Projection { base: value, elem: ProjectionElem::Deref, provenance: None },
)
};
self.derefs.push(index);
index
}

fn invalidate_derefs(&mut self) {
Expand Down Expand Up @@ -648,7 +669,7 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
ImmTy::from_immediate(ptr_imm, ty).into()
}

Projection(base, elem) => {
Projection { base, elem, .. } => {
let base = self.eval_to_const(base)?;
// `Index` by constants should have been replaced by `ConstantIndex` by
// `simplify_place_projection`.
Expand Down Expand Up @@ -827,8 +848,9 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
ProjectionElem::Field(f, _) => match self.get(value) {
Value::Aggregate(_, fields) => return Some((projection_ty, fields[f.as_usize()])),
Value::Union(active, field) if active == f => return Some((projection_ty, field)),
Value::Projection(outer_value, ProjectionElem::Downcast(_, read_variant))
if let Value::Aggregate(written_variant, fields) = self.get(outer_value)
Value::Projection {
base, elem: ProjectionElem::Downcast(_, read_variant), ..
} if let Value::Aggregate(written_variant, fields) = self.get(base)
// 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
// ```
Expand Down Expand Up @@ -881,7 +903,10 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
ProjectionElem::UnwrapUnsafeBinder(_) => ProjectionElem::UnwrapUnsafeBinder(()),
};

let value = self.insert(projection_ty.ty, Value::Projection(value, proj));
let value = self.insert(
projection_ty.ty,
Value::Projection { base: value, elem: proj, provenance: None },
);
Some((projection_ty, value))
}

Expand Down Expand Up @@ -1114,11 +1139,17 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, 'tcx> {
fields: &[VnIndex],
) -> Option<VnIndex> {
let Some(&first_field) = fields.first() else { return None };
let Value::Projection(copy_from_value, _) = self.get(first_field) else { return None };
let Value::Projection { base: 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 {
base: pointer,
elem: ProjectionElem::Field(from_index, _),
..
} = self.get(v)
&& copy_from_value == pointer
&& from_index.index() == index
{
Expand All @@ -1130,7 +1161,7 @@ impl<'body, 'a, 'tcx> VnState<'body, 'a, '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 { base: pointer, elem: proj, .. } = self.get(copy_from_value)
&& let ProjectionElem::Downcast(_, read_variant) = proj
{
if variant_index == read_variant {
Expand Down Expand Up @@ -1843,7 +1874,7 @@ impl<'tcx> VnState<'_, '_, 'tcx> {
// 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)
} else if let Value::Projection { base: pointer, elem: proj, .. } = self.get(index)
&& (allow_complex_projection || proj.is_stable_offset())
&& let Some(proj) = self.try_as_place_elem(self.ty(index), proj, loc)
{
Expand Down
16 changes: 6 additions & 10 deletions tests/mir-opt/gvn_copy_aggregate.all_copy_2.GVN.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,25 +29,21 @@
_8 = copy (*_1);
_2 = copy ((*_8).0: i32);
- StorageLive(_3);
- _9 = copy (*_1);
- _3 = copy ((*_9).1: u64);
- StorageLive(_4);
- _10 = copy (*_1);
- _4 = copy ((*_10).2: [i8; 3]);
+ nop;
+ _9 = copy _8;
+ _3 = copy ((*_8).1: u64);
_9 = copy (*_1);
_3 = copy ((*_9).1: u64);
- StorageLive(_4);
+ nop;
+ _10 = copy _8;
+ _4 = copy ((*_8).2: [i8; 3]);
_10 = copy (*_1);
_4 = copy ((*_10).2: [i8; 3]);
StorageLive(_5);
_5 = copy _2;
StorageLive(_6);
_6 = copy _3;
StorageLive(_7);
_7 = copy _4;
- _0 = AllCopy { a: move _5, b: move _6, c: move _7 };
+ _0 = copy (*_8);
+ _0 = AllCopy { a: copy _2, b: copy _3, c: copy _4 };
StorageDead(_7);
StorageDead(_6);
StorageDead(_5);
Expand Down
5 changes: 2 additions & 3 deletions tests/mir-opt/gvn_copy_aggregate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,12 @@ fn all_copy(v: &AllCopy) -> AllCopy {
AllCopy { a, b, c }
}

// FIXME: For nested references, we need knowing each borrows can be merged.
// EMIT_MIR gvn_copy_aggregate.all_copy_2.GVN.diff
fn all_copy_2(v: &&AllCopy) -> AllCopy {
// CHECK-LABEL: fn all_copy_2(
// CHECK: bb0: {
// CHECK-NOT: = AllCopy { {{.*}} };
// CHECK: [[V1:_.*]] = copy (*_1);
// CHECK: _0 = copy (*[[V1]]);
// CHECK: _0 = AllCopy { {{.*}} };
let a = v.a;
let b = v.b;
let c = v.c;
Expand Down
8 changes: 8 additions & 0 deletions tests/mir-opt/pre-codegen/deref_nested_borrows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

fn src(x: &&u8) -> bool {
// CHECK-LABEL: fn src(
// CHECK: debug y => [[Y:_.*]];
// CHECK: bb0:
// CHECK: [[BORROW_u8:_.*]] = copy (*_1);
// CHECK: [[Y]] = copy (*[[BORROW_u8]]);
// CHECK: bb1:
// BORROW_u8 outside its lifetime in bb1.
// CHECK-NOT: copy (*[[BORROW_u8]]);
// CHECK: copy (*_1);
// CHECK-NOT: _0 = const true;
// CHECK: _0 = Eq({{.*}}, {{.*}});
// CHECK-NOT: _0 = const true;
Expand Down
150 changes: 78 additions & 72 deletions tests/mir-opt/pre-codegen/slice_filter.variant_a-{closure#0}.PreCodegen.after.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,101 +3,107 @@
fn variant_a::{closure#0}(_1: &mut {closure@$DIR/slice_filter.rs:7:25: 7:39}, _2: &&(usize, usize, usize, usize)) -> bool {
let mut _0: bool;
let mut _3: &(usize, usize, usize, usize);
let mut _6: bool;
let mut _4: &(usize, usize, usize, usize);
let mut _5: &(usize, usize, usize, usize);
let mut _6: &(usize, usize, usize, usize);
let mut _9: bool;
let mut _10: bool;
let _13: &usize;
let _14: &usize;
let _15: &usize;
let mut _12: bool;
let mut _13: bool;
let _16: &usize;
let mut _17: &&usize;
let mut _18: &&usize;
let mut _19: &&usize;
let _17: &usize;
let _18: &usize;
let _19: &usize;
let mut _20: &&usize;
let mut _21: &&usize;
let mut _22: &&usize;
let mut _23: &&usize;
let mut _24: &&usize;
let mut _25: &&usize;
let mut _26: &&usize;
let mut _27: &&usize;
scope 1 {
debug a => _13;
debug b => _14;
debug c => _15;
debug d => _16;
debug a => _16;
debug b => _17;
debug c => _18;
debug d => _19;
scope 2 (inlined std::cmp::impls::<impl PartialOrd for &usize>::le) {
debug self => _17;
debug other => _18;
debug self => _20;
debug other => _21;
scope 3 (inlined std::cmp::impls::<impl PartialOrd for usize>::le) {
debug self => _13;
debug other => _15;
let mut _4: usize;
let mut _5: usize;
debug self => _16;
debug other => _18;
let mut _7: usize;
let mut _8: usize;
}
}
scope 4 (inlined std::cmp::impls::<impl PartialOrd for &usize>::le) {
debug self => _19;
debug other => _20;
debug self => _22;
debug other => _23;
scope 5 (inlined std::cmp::impls::<impl PartialOrd for usize>::le) {
debug self => _16;
debug other => _14;
let mut _7: usize;
let mut _8: usize;
debug self => _19;
debug other => _17;
let mut _10: usize;
let mut _11: usize;
}
}
scope 6 (inlined std::cmp::impls::<impl PartialOrd for &usize>::le) {
debug self => _21;
debug other => _22;
debug self => _24;
debug other => _25;
scope 7 (inlined std::cmp::impls::<impl PartialOrd for usize>::le) {
debug self => _15;
debug other => _13;
debug self => _18;
debug other => _16;
}
}
scope 8 (inlined std::cmp::impls::<impl PartialOrd for &usize>::le) {
debug self => _23;
debug other => _24;
debug self => _26;
debug other => _27;
scope 9 (inlined std::cmp::impls::<impl PartialOrd for usize>::le) {
debug self => _14;
debug other => _16;
let mut _11: usize;
let mut _12: usize;
debug self => _17;
debug other => _19;
let mut _14: usize;
let mut _15: usize;
}
}
}

bb0: {
_3 = copy (*_2);
// DBG: _13 = &((*_3).0: usize);
// DBG: _14 = &((*_3).1: usize);
// DBG: _15 = &((*_3).2: usize);
// DBG: _16 = &((*_3).3: usize);
StorageLive(_6);
// DBG: _17 = &_13;
// DBG: _18 = &?;
_4 = copy ((*_3).0: usize);
_5 = copy ((*_3).2: usize);
_6 = Le(copy _4, copy _5);
switchInt(move _6) -> [0: bb2, otherwise: bb1];
// DBG: _16 = &((*_3).0: usize);
_4 = copy (*_2);
// DBG: _17 = &((*_4).1: usize);
_5 = copy (*_2);
// DBG: _18 = &((*_5).2: usize);
_6 = copy (*_2);
// DBG: _19 = &((*_6).3: usize);
StorageLive(_9);
// DBG: _20 = &_16;
// DBG: _21 = &?;
_7 = copy ((*_3).0: usize);
_8 = copy ((*_5).2: usize);
_9 = Le(copy _7, copy _8);
switchInt(move _9) -> [0: bb2, otherwise: bb1];
}

bb1: {
StorageLive(_9);
// DBG: _19 = &_16;
// DBG: _20 = &?;
StorageLive(_7);
_7 = copy ((*_3).3: usize);
StorageLive(_8);
_8 = copy ((*_3).1: usize);
_9 = Le(move _7, move _8);
StorageDead(_8);
StorageDead(_7);
switchInt(move _9) -> [0: bb2, otherwise: bb6];
StorageLive(_12);
// DBG: _22 = &_19;
// DBG: _23 = &?;
StorageLive(_10);
_10 = copy ((*_6).3: usize);
StorageLive(_11);
_11 = copy ((*_4).1: usize);
_12 = Le(move _10, move _11);
StorageDead(_11);
StorageDead(_10);
switchInt(move _12) -> [0: bb2, otherwise: bb6];
}

bb2: {
StorageLive(_10);
// DBG: _21 = &_15;
// DBG: _22 = &?;
_10 = Le(copy _5, copy _4);
switchInt(move _10) -> [0: bb3, otherwise: bb4];
StorageLive(_13);
// DBG: _24 = &_18;
// DBG: _25 = &?;
_13 = Le(copy _8, copy _7);
switchInt(move _13) -> [0: bb3, otherwise: bb4];
}

bb3: {
Expand All @@ -106,20 +112,20 @@ fn variant_a::{closure#0}(_1: &mut {closure@$DIR/slice_filter.rs:7:25: 7:39}, _2
}

bb4: {
// DBG: _23 = &_14;
// DBG: _24 = &?;
StorageLive(_11);
_11 = copy ((*_3).1: usize);
StorageLive(_12);
_12 = copy ((*_3).3: usize);
_0 = Le(move _11, move _12);
StorageDead(_12);
StorageDead(_11);
// DBG: _26 = &_17;
// DBG: _27 = &?;
StorageLive(_14);
_14 = copy ((*_4).1: usize);
StorageLive(_15);
_15 = copy ((*_6).3: usize);
_0 = Le(move _14, move _15);
StorageDead(_15);
StorageDead(_14);
goto -> bb5;
}

bb5: {
StorageDead(_10);
StorageDead(_13);
goto -> bb7;
}

Expand All @@ -129,8 +135,8 @@ fn variant_a::{closure#0}(_1: &mut {closure@$DIR/slice_filter.rs:7:25: 7:39}, _2
}

bb7: {
StorageDead(_12);
StorageDead(_9);
StorageDead(_6);
return;
}
}
Loading
Loading