Skip to content

gvn: bail out unavoidable non-ssa locals in repeat #141252

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

Merged
merged 1 commit into from
May 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 7 additions & 1 deletion compiler/rustc_mir_transform/src/gvn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
place: PlaceRef<'tcx>,
value: VnIndex,
proj: PlaceElem<'tcx>,
from_non_ssa_index: &mut bool,
) -> Option<VnIndex> {
let proj = match proj {
ProjectionElem::Deref => {
Expand Down Expand Up @@ -682,6 +683,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(*inner);
}
let idx = self.locals[idx]?;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saethlin Yes, right here. This is an exception for the repeat array, because whichever value we access, it's all the same. However, we have overlooked the out-of-bounds case here.

Expand Down Expand Up @@ -774,6 +776,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {

// Invariant: `value` holds the value up-to the `index`th projection excluded.
let mut value = self.locals[place.local]?;
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)
Expand All @@ -791,7 +794,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
}

let base = PlaceRef { local: place.local, projection: &place.projection[..index] };
value = self.project(base, value, proj)?;
value = self.project(base, value, proj, &mut from_non_ssa_index)?;
}

if let Value::Projection(pointer, ProjectionElem::Deref) = *self.get(value)
Expand All @@ -804,6 +807,9 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
}
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;
}

if place_ref.local != place.local || place_ref.projection.len() < place.projection.len() {
Expand Down
18 changes: 18 additions & 0 deletions tests/mir-opt/gvn_repeat.repeat_local.GVN.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
- // MIR for `repeat_local` before GVN
+ // MIR for `repeat_local` after GVN

fn repeat_local(_1: usize, _2: usize, _3: i32) -> i32 {
let mut _0: i32;
let mut _4: [i32; 5];
let mut _5: &i32;

bb0: {
_4 = [copy _3; 5];
_5 = &_4[_1];
_1 = copy _2;
- _0 = copy (*_5);
+ _0 = copy _3;
return;
}
}

17 changes: 17 additions & 0 deletions tests/mir-opt/gvn_repeat.repeat_place.GVN.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
- // MIR for `repeat_place` before GVN
+ // MIR for `repeat_place` after GVN

fn repeat_place(_1: usize, _2: usize, _3: &i32) -> i32 {
let mut _0: i32;
let mut _4: [i32; 5];
let mut _5: &i32;

bb0: {
_4 = [copy (*_3); 5];
_5 = &_4[_1];
_1 = copy _2;
_0 = copy (*_5);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return;
}
}

49 changes: 49 additions & 0 deletions tests/mir-opt/gvn_repeat.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
//@ test-mir-pass: GVN

#![feature(custom_mir, core_intrinsics)]

// Check that we do not introduce out-of-bounds access.

use std::intrinsics::mir::*;

// EMIT_MIR gvn_repeat.repeat_place.GVN.diff
#[custom_mir(dialect = "runtime")]
pub fn repeat_place(mut idx1: usize, idx2: usize, val: &i32) -> i32 {
// CHECK-LABEL: fn repeat_place(
// CHECK: let mut [[ELEM:.*]]: &i32;
// CHECK: _0 = copy (*[[ELEM]])
mir! {
let array;
let elem;
{
array = [*val; 5];
elem = &array[idx1];
idx1 = idx2;
RET = *elem;
Return()
}
}
}

// EMIT_MIR gvn_repeat.repeat_local.GVN.diff
#[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
mir! {
let array;
let elem;
{
array = [val; 5];
elem = &array[idx1];
idx1 = idx2;
RET = *elem;
Return()
}
}
}

fn main() {
assert_eq!(repeat_place(0, 5, &0), 0);
assert_eq!(repeat_local(0, 5, 0), 0);
}
Loading