Skip to content

Accelerate GVN a little #126991

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 7 commits into from
Jul 31, 2024
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
64 changes: 43 additions & 21 deletions compiler/rustc_mir_transform/src/gvn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ fn propagate_ssa<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
// Clone dominators as we need them while mutating the body.
let dominators = body.basic_blocks.dominators().clone();

let mut state = VnState::new(tcx, param_env, &ssa, &dominators, &body.local_decls);
let mut state = VnState::new(tcx, body, param_env, &ssa, &dominators, &body.local_decls);
ssa.for_each_assignment_mut(
body.basic_blocks.as_mut_preserves_cfg(),
|local, value, location| {
Expand Down Expand Up @@ -204,6 +204,7 @@ enum Value<'tcx> {
value: Const<'tcx>,
/// Some constants do not have a deterministic value. To avoid merging two instances of the
/// same `Const`, we assign them an additional integer index.
// `disambiguator` is 0 iff the constant is deterministic.
disambiguator: usize,
},
/// An aggregate value, either tuple/closure/struct/enum.
Expand Down Expand Up @@ -266,21 +267,29 @@ struct VnState<'body, 'tcx> {
impl<'body, 'tcx> VnState<'body, 'tcx> {
fn new(
tcx: TyCtxt<'tcx>,
body: &Body<'tcx>,
param_env: ty::ParamEnv<'tcx>,
ssa: &'body SsaLocals,
dominators: &'body Dominators<BasicBlock>,
local_decls: &'body LocalDecls<'tcx>,
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think you could get this from the passed-in body now, rather than needing the parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, they have different lifetimes. The caller will want to mutate part of the body, so we can only keep a reference to the local decls.

) -> 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
// we miss the exact amount. We estimate based on 2 values per statement (one in LHS and
// one in RHS) and 4 values per terminator (for call operands).
let num_values =
2 * body.basic_blocks.iter().map(|bbdata| bbdata.statements.len()).sum::<usize>()
+ 4 * body.basic_blocks.len();
VnState {
tcx,
ecx: InterpCx::new(tcx, DUMMY_SP, param_env, DummyMachine),
param_env,
local_decls,
locals: IndexVec::from_elem(None, local_decls),
rev_locals: IndexVec::default(),
values: FxIndexSet::default(),
evaluated: IndexVec::new(),
next_opaque: Some(0),
rev_locals: IndexVec::with_capacity(num_values),
values: FxIndexSet::with_capacity_and_hasher(num_values, Default::default()),
evaluated: IndexVec::with_capacity(num_values),
next_opaque: Some(1),
feature_unsized_locals: tcx.features().unsized_locals,
ssa,
dominators,
Expand All @@ -293,9 +302,15 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
let (index, new) = self.values.insert_full(value);
let index = VnIndex::from_usize(index);
if new {
// Grow `evaluated` and `rev_locals` here to amortize the allocations.
let evaluated = self.eval_to_const(index);
let _index = self.evaluated.push(evaluated);
debug_assert_eq!(index, _index);
// No need to push to `rev_locals` if we finished listing assignments.
if self.next_opaque.is_some() {
let _index = self.rev_locals.push(SmallVec::new());
debug_assert_eq!(index, _index);
}
}
index
}
Expand Down Expand Up @@ -332,7 +347,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
let is_sized = !self.feature_unsized_locals
|| self.local_decls[local].ty.is_sized(self.tcx, self.param_env);
if is_sized {
self.rev_locals.ensure_contains_elem(value, SmallVec::new).push(local);
self.rev_locals[value].push(local);
}
}

Expand All @@ -346,19 +361,25 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
let next_opaque = self.next_opaque.as_mut()?;
let disambiguator = *next_opaque;
*next_opaque += 1;
// `disambiguator: 0` means deterministic.
debug_assert_ne!(disambiguator, 0);
disambiguator
};
Some(self.insert(Value::Constant { value, disambiguator }))
}

fn insert_bool(&mut self, flag: bool) -> VnIndex {
// Booleans are deterministic.
self.insert(Value::Constant { value: Const::from_bool(self.tcx, flag), disambiguator: 0 })
let value = Const::from_bool(self.tcx, flag);
debug_assert!(value.is_deterministic());
self.insert(Value::Constant { value, disambiguator: 0 })
}

fn insert_scalar(&mut self, scalar: Scalar, ty: Ty<'tcx>) -> VnIndex {
self.insert_constant(Const::from_scalar(self.tcx, scalar, ty))
.expect("scalars are deterministic")
// Scalars are deterministic.
let value = Const::from_scalar(self.tcx, scalar, ty);
debug_assert!(value.is_deterministic());
self.insert(Value::Constant { value, disambiguator: 0 })
}

fn insert_tuple(&mut self, values: Vec<VnIndex>) -> VnIndex {
Expand Down Expand Up @@ -671,7 +692,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
fn simplify_place_projection(&mut self, place: &mut Place<'tcx>, location: Location) {
// If the projection is indirect, we treat the local as a value, so can replace it with
// another local.
if place.is_indirect()
if place.is_indirect_first_projection()
&& let Some(base) = self.locals[place.local]
&& let Some(new_local) = self.try_as_local(base, location)
&& place.local != new_local
Expand Down Expand Up @@ -773,10 +794,7 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
location: Location,
) -> Option<VnIndex> {
match *operand {
Operand::Constant(ref mut constant) => {
let const_ = constant.const_.normalize(self.tcx, self.param_env);
self.insert_constant(const_)
}
Operand::Constant(ref constant) => self.insert_constant(constant.const_),
Operand::Copy(ref mut place) | Operand::Move(ref mut place) => {
let value = self.simplify_place_value(place, location)?;
if let Some(const_) = self.try_as_constant(value) {
Expand Down Expand Up @@ -1371,8 +1389,13 @@ fn op_to_prop_const<'tcx>(
// If this constant has scalar ABI, return it as a `ConstValue::Scalar`.
if let Abi::Scalar(abi::Scalar::Initialized { .. }) = op.layout.abi
&& let Ok(scalar) = ecx.read_scalar(op)
&& scalar.try_to_scalar_int().is_ok()
{
if !scalar.try_to_scalar_int().is_ok() {
// Check that we do not leak a pointer.
// Those pointers may lose part of their identity in codegen.
// FIXME: remove this hack once https://github.com/rust-lang/rust/issues/79738 is fixed.
return None;
}
return Some(ConstValue::Scalar(scalar));
}

Expand Down Expand Up @@ -1436,12 +1459,11 @@ impl<'tcx> VnState<'_, 'tcx> {

/// If `index` is a `Value::Constant`, return the `Constant` to be put in the MIR.
fn try_as_constant(&mut self, index: VnIndex) -> Option<ConstOperand<'tcx>> {
// This was already constant in MIR, do not change it.
if let Value::Constant { value, disambiguator: _ } = *self.get(index)
// If the constant is not deterministic, adding an additional mention of it in MIR will
// not give the same value as the former mention.
&& value.is_deterministic()
{
// 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) {
debug_assert!(value.is_deterministic());
return Some(ConstOperand { span: DUMMY_SP, user_ty: None, const_: value });
}

Expand Down
4 changes: 2 additions & 2 deletions tests/incremental/hashes/for_loops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,9 @@ pub fn change_iterable() {
}

#[cfg(not(any(cfail1,cfail4)))]
#[rustc_clean(cfg="cfail2", except="opt_hir_owner_nodes, promoted_mir, optimized_mir")]
#[rustc_clean(cfg="cfail2", except="opt_hir_owner_nodes, promoted_mir")]
#[rustc_clean(cfg="cfail3")]
#[rustc_clean(cfg="cfail5", except="opt_hir_owner_nodes, promoted_mir, optimized_mir")]
#[rustc_clean(cfg="cfail5", except="opt_hir_owner_nodes, promoted_mir")]
#[rustc_clean(cfg="cfail6")]
pub fn change_iterable() {
let mut _x = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,11 @@
StorageLive(_3);
StorageLive(_4);
_9 = const main::promoted[0];
- _4 = _9;
_4 = _9;
- _3 = _4;
- _2 = move _3 as &[u32] (PointerCoercion(Unsize));
+ _4 = const {ALLOC0<imm>: &[u32; 3]};
+ _3 = const {ALLOC0<imm>: &[u32; 3]};
+ _2 = const {ALLOC0<imm>: &[u32; 3]} as &[u32] (PointerCoercion(Unsize));
+ _3 = _9;
+ _2 = _9 as &[u32] (PointerCoercion(Unsize));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saethlin this is this kind of pattern.
The constant is still there, named main::promoted[0]. It's "evaluated" form const {ALLOC0<imm>: &[u32; 3]} does not appear in the MIR dump, so the dump does not print ALLOC0. This does not mean that ALLOC0 was not evaluated, just that it's not put in MIR and not printed.

Copy link
Member

Choose a reason for hiding this comment

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

So are you saying there consts that get their allocations dumped, and some consts that don't? That seems like an undesirable property of MIR dumps, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, Ideally promoteds would just be regular statics, but lazily created in the MIR opt pipeline. This is blocked on various works around DefId creation, most importantly #115613. I guess we could add a special case for showing promoteds like we show other allocations for now, but that seems orthogonal to this PR

Copy link
Member

Choose a reason for hiding this comment

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

Statics can't be generic so IMO they should be regular consts.

StorageDead(_3);
StorageLive(_6);
_6 = const 1_usize;
Expand All @@ -50,6 +49,4 @@
return;
}
}
+
+ ALLOC0 (size: 12, align: 4) { .. }

Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,11 @@
StorageLive(_3);
StorageLive(_4);
_9 = const main::promoted[0];
- _4 = _9;
_4 = _9;
- _3 = _4;
- _2 = move _3 as &[u32] (PointerCoercion(Unsize));
+ _4 = const {ALLOC0<imm>: &[u32; 3]};
+ _3 = const {ALLOC0<imm>: &[u32; 3]};
+ _2 = const {ALLOC0<imm>: &[u32; 3]} as &[u32] (PointerCoercion(Unsize));
+ _3 = _9;
+ _2 = _9 as &[u32] (PointerCoercion(Unsize));
StorageDead(_3);
StorageLive(_6);
_6 = const 1_usize;
Expand All @@ -50,6 +49,4 @@
return;
}
}
+
+ ALLOC0 (size: 12, align: 4) { .. }

Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,11 @@
StorageLive(_3);
StorageLive(_4);
_9 = const main::promoted[0];
- _4 = _9;
_4 = _9;
- _3 = _4;
- _2 = move _3 as &[u32] (PointerCoercion(Unsize));
+ _4 = const {ALLOC0<imm>: &[u32; 3]};
+ _3 = const {ALLOC0<imm>: &[u32; 3]};
+ _2 = const {ALLOC0<imm>: &[u32; 3]} as &[u32] (PointerCoercion(Unsize));
+ _3 = _9;
+ _2 = _9 as &[u32] (PointerCoercion(Unsize));
StorageDead(_3);
StorageLive(_6);
_6 = const 1_usize;
Expand All @@ -50,6 +49,4 @@
return;
}
}
+
+ ALLOC0 (size: 12, align: 4) { .. }

Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,11 @@
StorageLive(_3);
StorageLive(_4);
_9 = const main::promoted[0];
- _4 = _9;
_4 = _9;
- _3 = _4;
- _2 = move _3 as &[u32] (PointerCoercion(Unsize));
+ _4 = const {ALLOC0<imm>: &[u32; 3]};
+ _3 = const {ALLOC0<imm>: &[u32; 3]};
+ _2 = const {ALLOC0<imm>: &[u32; 3]} as &[u32] (PointerCoercion(Unsize));
+ _3 = _9;
+ _2 = _9 as &[u32] (PointerCoercion(Unsize));
StorageDead(_3);
StorageLive(_6);
_6 = const 1_usize;
Expand All @@ -50,6 +49,4 @@
return;
}
}
+
+ ALLOC0 (size: 12, align: 4) { .. }

2 changes: 1 addition & 1 deletion tests/mir-opt/const_prop/slice_len.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
fn main() {
// CHECK-LABEL: fn main(
// CHECK: debug a => [[a:_.*]];
// CHECK: [[slice:_.*]] = const {{.*}} as &[u32] (PointerCoercion(Unsize));
// CHECK: [[slice:_.*]] = {{.*}} as &[u32] (PointerCoercion(Unsize));
// CHECK: assert(const true,
// CHECK: [[a]] = const 2_u32;
let a = (&[1u32, 2, 3] as &[u32])[1];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,12 @@
StorageLive(_6);
StorageLive(_7);
_9 = const main::promoted[0];
- _7 = _9;
+ _7 = const {ALLOC1<imm>: &std::alloc::Global};
_7 = _9;
StorageLive(_8);
- _8 = _1;
- _6 = std::alloc::Global::alloc_impl(move _7, move _8, const false) -> [return: bb4, unwind unreachable];
+ _8 = const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): std::ptr::alignment::AlignmentEnum) }};
+ _6 = std::alloc::Global::alloc_impl(const {ALLOC1<imm>: &std::alloc::Global}, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb4, unwind unreachable];
+ _6 = std::alloc::Global::alloc_impl(_9, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb4, unwind unreachable];
}

bb4: {
Expand Down Expand Up @@ -119,11 +118,9 @@
+ nop;
return;
}
}
+ }
+
+ ALLOC0 (size: 8, align: 4) {
+ 00 00 00 00 __ __ __ __ │ ....░░░░
+ }
+
+ ALLOC1 (size: 0, align: 1) {}
}

Original file line number Diff line number Diff line change
Expand Up @@ -81,25 +81,22 @@
StorageLive(_6);
StorageLive(_7);
_9 = const main::promoted[0];
- _7 = _9;
+ _7 = const {ALLOC1<imm>: &std::alloc::Global};
_7 = _9;
StorageLive(_8);
- _8 = _1;
- _6 = std::alloc::Global::alloc_impl(move _7, move _8, const false) -> [return: bb5, unwind continue];
+ _8 = const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): std::ptr::alignment::AlignmentEnum) }};
+ _6 = std::alloc::Global::alloc_impl(const {ALLOC1<imm>: &std::alloc::Global}, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb5, unwind continue];
+ _6 = std::alloc::Global::alloc_impl(_9, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(4 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x00000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb5, unwind continue];
}

bb5: {
StorageDead(_8);
StorageDead(_7);
_5 = Result::<NonNull<[u8]>, std::alloc::AllocError>::unwrap(move _6) -> [return: bb1, unwind continue];
}
}
+ }
+
+ ALLOC0 (size: 8, align: 4) {
+ 00 00 00 00 __ __ __ __ │ ....░░░░
+ }
+
+ ALLOC1 (size: 0, align: 1) {}
}

Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,12 @@
StorageLive(_6);
StorageLive(_7);
_9 = const main::promoted[0];
- _7 = _9;
+ _7 = const {ALLOC1<imm>: &std::alloc::Global};
_7 = _9;
StorageLive(_8);
- _8 = _1;
- _6 = std::alloc::Global::alloc_impl(move _7, move _8, const false) -> [return: bb4, unwind unreachable];
+ _8 = const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(8 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x0000000000000000): std::ptr::alignment::AlignmentEnum) }};
+ _6 = std::alloc::Global::alloc_impl(const {ALLOC1<imm>: &std::alloc::Global}, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(8 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x0000000000000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb4, unwind unreachable];
+ _6 = std::alloc::Global::alloc_impl(_9, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(8 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x0000000000000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb4, unwind unreachable];
}

bb4: {
Expand Down Expand Up @@ -119,11 +118,9 @@
+ nop;
return;
}
}
+ }
+
+ ALLOC0 (size: 16, align: 8) {
+ 00 00 00 00 00 00 00 00 __ __ __ __ __ __ __ __ │ ........░░░░░░░░
+ }
+
+ ALLOC1 (size: 0, align: 1) {}
}

Original file line number Diff line number Diff line change
Expand Up @@ -81,25 +81,22 @@
StorageLive(_6);
StorageLive(_7);
_9 = const main::promoted[0];
- _7 = _9;
+ _7 = const {ALLOC1<imm>: &std::alloc::Global};
_7 = _9;
StorageLive(_8);
- _8 = _1;
- _6 = std::alloc::Global::alloc_impl(move _7, move _8, const false) -> [return: bb5, unwind continue];
+ _8 = const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(8 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x0000000000000000): std::ptr::alignment::AlignmentEnum) }};
+ _6 = std::alloc::Global::alloc_impl(const {ALLOC1<imm>: &std::alloc::Global}, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(8 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x0000000000000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb5, unwind continue];
+ _6 = std::alloc::Global::alloc_impl(_9, const Layout {{ size: Indirect { alloc_id: ALLOC0, offset: Size(8 bytes) }: usize, align: std::ptr::Alignment(Scalar(0x0000000000000000): std::ptr::alignment::AlignmentEnum) }}, const false) -> [return: bb5, unwind continue];
}

bb5: {
StorageDead(_8);
StorageDead(_7);
_5 = Result::<NonNull<[u8]>, std::alloc::AllocError>::unwrap(move _6) -> [return: bb1, unwind continue];
}
}
+ }
+
+ ALLOC0 (size: 16, align: 8) {
+ 00 00 00 00 00 00 00 00 __ __ __ __ __ __ __ __ │ ........░░░░░░░░
+ }
+
+ ALLOC1 (size: 0, align: 1) {}
}

Loading