Skip to content
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

Turn copy into moves during DSE. #113758

Merged
merged 3 commits into from
Jul 20, 2023
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
2 changes: 1 addition & 1 deletion compiler/rustc_mir_dataflow/src/impls/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeLiveLocals {
}
}

struct TransferFunction<'a, T>(&'a mut T);
pub struct TransferFunction<'a, T>(pub &'a mut T);

impl<'tcx, T> Visitor<'tcx> for TransferFunction<'_, T>
where
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_mir_dataflow/src/impls/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pub use self::borrowed_locals::borrowed_locals;
pub use self::borrowed_locals::MaybeBorrowedLocals;
pub use self::liveness::MaybeLiveLocals;
pub use self::liveness::MaybeTransitiveLiveLocals;
pub use self::liveness::TransferFunction as LivenessTransferFunction;
pub use self::storage_liveness::{MaybeRequiresStorage, MaybeStorageDead, MaybeStorageLive};

/// `MaybeInitializedPlaces` tracks all places that might be
Expand Down
40 changes: 38 additions & 2 deletions compiler/rustc_mir_transform/src/dead_store_elimination.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,12 @@
//!

use rustc_index::bit_set::BitSet;
use rustc_middle::mir::visit::Visitor;
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;
use rustc_mir_dataflow::impls::{borrowed_locals, MaybeTransitiveLiveLocals};
use rustc_mir_dataflow::impls::{
borrowed_locals, LivenessTransferFunction, MaybeTransitiveLiveLocals,
};
use rustc_mir_dataflow::Analysis;

/// Performs the optimization on the body
Expand All @@ -28,8 +31,33 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, borrowed: &BitS
.iterate_to_fixpoint()
.into_results_cursor(body);

// For blocks with a call terminator, if an argument copy can be turned into a move,
// record it as (block, argument index).
let mut call_operands_to_move = Vec::new();
let mut patch = Vec::new();

for (bb, bb_data) in traversal::preorder(body) {
if let TerminatorKind::Call { ref args, .. } = bb_data.terminator().kind {
let loc = Location { block: bb, statement_index: bb_data.statements.len() };

// Position ourselves between the evaluation of `args` and the write to `destination`.
live.seek_to_block_end(bb);
let mut state = live.get().clone();

for (index, arg) in args.iter().enumerate().rev() {
if let Operand::Copy(place) = *arg
&& !place.is_indirect()
&& !borrowed.contains(place.local)
&& !state.contains(place.local)
{
call_operands_to_move.push((bb, index));
}

// Account that `arg` is read from, so we don't promote another argument to a move.
LivenessTransferFunction(&mut state).visit_operand(arg, loc);
}
}

for (statement_index, statement) in bb_data.statements.iter().enumerate().rev() {
let loc = Location { block: bb, statement_index };
if let StatementKind::Assign(assign) = &statement.kind {
Expand Down Expand Up @@ -64,14 +92,22 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, borrowed: &BitS
}
}

if patch.is_empty() {
if patch.is_empty() && call_operands_to_move.is_empty() {
return;
}

let bbs = body.basic_blocks.as_mut_preserves_cfg();
for Location { block, statement_index } in patch {
bbs[block].statements[statement_index].make_nop();
}
for (block, argument_index) in call_operands_to_move {
let TerminatorKind::Call { ref mut args, .. } = bbs[block].terminator_mut().kind else {
bug!()
};
let arg = &mut args[argument_index];
let Operand::Copy(place) = *arg else { bug!() };
*arg = Operand::Move(place);
}

crate::simplify::simplify_locals(body, tcx)
}
Expand Down
2 changes: 1 addition & 1 deletion tests/codegen/iter-repeat-n-trivial-drop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub fn iter_repeat_n_next(it: &mut std::iter::RepeatN<NotCopy>) -> Option<NotCop

// CHECK: [[EMPTY]]:
// CHECK-NOT: br
// CHECK: phi i16 [ %[[VAL]], %[[NOT_EMPTY]] ], [ undef, %start ]
// CHECK: phi i16
// CHECK-NOT: br
// CHECK: ret

Expand Down
3 changes: 2 additions & 1 deletion tests/codegen/move-operands.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// compile-flags: -C no-prepopulate-passes -Zmir-enable-passes=+DestinationPropagation,-CopyProp
// Verify that optimized MIR only copies `a` once.
// compile-flags: -O -C no-prepopulate-passes

#![crate_type = "lib"]

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
- // MIR for `move_simple` before DeadStoreElimination
+ // MIR for `move_simple` after DeadStoreElimination

fn move_simple(_1: i32) -> () {
debug x => _1;
let mut _0: ();
let _2: ();
- let mut _3: i32;
- let mut _4: i32;

bb0: {
StorageLive(_2);
- _2 = use_both(_1, _1) -> [return: bb1, unwind unreachable];
+ _2 = use_both(_1, move _1) -> [return: bb1, unwind unreachable];
}

bb1: {
StorageDead(_2);
_0 = const ();
return;
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
- // MIR for `move_simple` before DeadStoreElimination
+ // MIR for `move_simple` after DeadStoreElimination

fn move_simple(_1: i32) -> () {
debug x => _1;
let mut _0: ();
let _2: ();
- let mut _3: i32;
- let mut _4: i32;

bb0: {
StorageLive(_2);
- _2 = use_both(_1, _1) -> [return: bb1, unwind continue];
+ _2 = use_both(_1, move _1) -> [return: bb1, unwind continue];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RalfJung IMO, the thorny question is here. It this transformation ok?

Copy link
Member

Choose a reason for hiding this comment

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

That's a very good question. It is okay under the move semantics I implemented in Miri recently (assuming _1 is not read from again later), because there we do left-to-right evaluation so the first _1 is copied before the move action. But I don't know if there's a risk of codegen backends doing things that would cause problems here. Also move semantics are far from clear; I don't think we already have the semantics we want here -- we just have something that justifies what codegen does.

I think, similar to how we avoid making assumptions about the aliasing model, we should be conservative and avoid this if any of the other arguments (or the return value) could alias the newly moved argument.

Cc @bjorn3

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine given that the first _1 is copied before the call passes the address of the second argument. It may even be ok to do use_both(move _1, _1) with current backends, but I don't think whatever semantics we give to move arguments should allow that as semantically the move of the first arg happens before the copy of the second arg.

}

bb1: {
StorageDead(_2);
_0 = const ();
return;
}
}

15 changes: 15 additions & 0 deletions tests/mir-opt/dead-store-elimination/call_arg_copy.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// EMIT_MIR_FOR_EACH_PANIC_STRATEGY
// unit-test: DeadStoreElimination
// compile-flags: -Zmir-enable-passes=+CopyProp

#[inline(never)]
fn use_both(_: i32, _: i32) {}

// EMIT_MIR call_arg_copy.move_simple.DeadStoreElimination.diff
fn move_simple(x: i32) {
use_both(x, x);
}

fn main() {
move_simple(1);
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
- _0 = try_execute_query::<<Q as Query>::C>(move _4) -> [return: bb2, unwind unreachable];
+ StorageLive(_5);
+ _5 = _4 as &dyn Cache<V = <Q as Query>::V> (PointerCoercion(Unsize));
+ _0 = <dyn Cache<V = <Q as Query>::V> as Cache>::store_nocache(_5) -> [return: bb2, unwind unreachable];
+ _0 = <dyn Cache<V = <Q as Query>::V> as Cache>::store_nocache(move _5) -> [return: bb2, unwind unreachable];
}

bb2: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
- _0 = try_execute_query::<<Q as Query>::C>(move _4) -> [return: bb2, unwind continue];
+ StorageLive(_5);
+ _5 = _4 as &dyn Cache<V = <Q as Query>::V> (PointerCoercion(Unsize));
+ _0 = <dyn Cache<V = <Q as Query>::V> as Cache>::store_nocache(_5) -> [return: bb2, unwind continue];
+ _0 = <dyn Cache<V = <Q as Query>::V> as Cache>::store_nocache(move _5) -> [return: bb2, unwind continue];
}

bb2: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
_2 = move _3 as &dyn Cache<V = <C as Cache>::V> (PointerCoercion(Unsize));
StorageDead(_3);
- _0 = mk_cycle::<<C as Cache>::V>(move _2) -> [return: bb1, unwind unreachable];
+ _0 = <dyn Cache<V = <C as Cache>::V> as Cache>::store_nocache(_2) -> [return: bb1, unwind unreachable];
+ _0 = <dyn Cache<V = <C as Cache>::V> as Cache>::store_nocache(move _2) -> [return: bb1, unwind unreachable];
}

bb1: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
_2 = move _3 as &dyn Cache<V = <C as Cache>::V> (PointerCoercion(Unsize));
StorageDead(_3);
- _0 = mk_cycle::<<C as Cache>::V>(move _2) -> [return: bb1, unwind continue];
+ _0 = <dyn Cache<V = <C as Cache>::V> as Cache>::store_nocache(_2) -> [return: bb1, unwind continue];
+ _0 = <dyn Cache<V = <C as Cache>::V> as Cache>::store_nocache(move _2) -> [return: bb1, unwind continue];
}

bb1: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@
+ StorageDead(_14);
+ StorageLive(_9);
+ _13 = const _;
+ _9 = std::alloc::Global::alloc_impl(_13, _8, const false) -> [return: bb5, unwind unreachable];
+ _9 = std::alloc::Global::alloc_impl(move _13, _8, const false) -> [return: bb5, unwind unreachable];
}

bb1: {
Expand All @@ -144,7 +144,7 @@
}

bb2: {
+ _12 = handle_alloc_error(_8) -> unwind unreachable;
+ _12 = handle_alloc_error(move _8) -> unwind unreachable;
+ }
+
+ bb3: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@
+ StorageDead(_14);
+ StorageLive(_9);
+ _13 = const _;
+ _9 = std::alloc::Global::alloc_impl(_13, _8, const false) -> [return: bb7, unwind: bb3];
+ _9 = std::alloc::Global::alloc_impl(move _13, _8, const false) -> [return: bb7, unwind: bb3];
}

bb1: {
Expand All @@ -161,7 +161,7 @@
- bb4 (cleanup): {
- resume;
+ bb4: {
+ _12 = handle_alloc_error(_8) -> bb3;
+ _12 = handle_alloc_error(move _8) -> bb3;
+ }
+
+ bb5: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ fn test2(_1: &dyn X) -> bool {
_3 = &(*_1);
_2 = move _3 as &dyn X (PointerCoercion(Unsize));
StorageDead(_3);
_0 = <dyn X as X>::y(_2) -> [return: bb1, unwind unreachable];
_0 = <dyn X as X>::y(move _2) -> [return: bb1, unwind unreachable];
}

bb1: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ fn test2(_1: &dyn X) -> bool {
_3 = &(*_1);
_2 = move _3 as &dyn X (PointerCoercion(Unsize));
StorageDead(_3);
_0 = <dyn X as X>::y(_2) -> [return: bb1, unwind continue];
_0 = <dyn X as X>::y(move _2) -> [return: bb1, unwind continue];
}

bb1: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
bb2: {
_6 = Shl(move _7, const 1_i32);
StorageDead(_7);
_3 = rotate_right::<u32>(_4, _6) -> [return: bb3, unwind unreachable];
_3 = rotate_right::<u32>(move _4, move _6) -> [return: bb3, unwind unreachable];
}

bb3: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
bb2: {
_6 = Shl(move _7, const 1_i32);
StorageDead(_7);
_3 = rotate_right::<u32>(_4, _6) -> [return: bb3, unwind unreachable];
_3 = rotate_right::<u32>(move _4, move _6) -> [return: bb3, unwind unreachable];
}

bb3: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ fn num_to_digit(_1: char) -> u32 {

bb2: {
StorageLive(_4);
_4 = char::methods::<impl char>::to_digit(_1, const 8_u32) -> [return: bb3, unwind unreachable];
_4 = char::methods::<impl char>::to_digit(move _1, const 8_u32) -> [return: bb3, unwind unreachable];
}

bb3: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ fn num_to_digit(_1: char) -> u32 {

bb2: {
StorageLive(_4);
_4 = char::methods::<impl char>::to_digit(_1, const 8_u32) -> [return: bb3, unwind continue];
_4 = char::methods::<impl char>::to_digit(move _1, const 8_u32) -> [return: bb3, unwind continue];
}

bb3: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ fn checked_shl(_1: u32, _2: u32) -> Option<u32> {
StorageDead(_4);
_6 = Ge(_2, const _);
StorageLive(_7);
_7 = unlikely(_6) -> [return: bb1, unwind unreachable];
_7 = unlikely(move _6) -> [return: bb1, unwind unreachable];
}

bb1: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ fn int_range(_1: usize, _2: usize) -> () {

bb7: {
_10 = ((_6 as Some).0: usize);
_11 = opaque::<usize>(_10) -> [return: bb8, unwind continue];
_11 = opaque::<usize>(move _10) -> [return: bb8, unwind continue];
}

bb8: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ fn inclusive_loop(_1: u32, _2: u32, _3: impl Fn(u32)) -> () {
StorageLive(_7);
StorageLive(_6);
_6 = &mut _5;
_7 = <RangeInclusive<u32> as iter::range::RangeInclusiveIteratorImpl>::spec_next(_6) -> [return: bb2, unwind unreachable];
_7 = <RangeInclusive<u32> as iter::range::RangeInclusiveIteratorImpl>::spec_next(move _6) -> [return: bb2, unwind unreachable];
}

bb2: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ fn inclusive_loop(_1: u32, _2: u32, _3: impl Fn(u32)) -> () {
StorageLive(_7);
StorageLive(_6);
_6 = &mut _5;
_7 = <RangeInclusive<u32> as iter::range::RangeInclusiveIteratorImpl>::spec_next(_6) -> [return: bb2, unwind: bb8];
_7 = <RangeInclusive<u32> as iter::range::RangeInclusiveIteratorImpl>::spec_next(move _6) -> [return: bb2, unwind: bb8];
}

bb2: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ fn range_inclusive_iter_next(_1: &mut RangeInclusive<u32>) -> Option<u32> {
}

bb0: {
_0 = <RangeInclusive<u32> as iter::range::RangeInclusiveIteratorImpl>::spec_next(_1) -> [return: bb1, unwind unreachable];
_0 = <RangeInclusive<u32> as iter::range::RangeInclusiveIteratorImpl>::spec_next(move _1) -> [return: bb1, unwind unreachable];
}

bb1: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ fn range_inclusive_iter_next(_1: &mut RangeInclusive<u32>) -> Option<u32> {
}

bb0: {
_0 = <RangeInclusive<u32> as iter::range::RangeInclusiveIteratorImpl>::spec_next(_1) -> [return: bb1, unwind continue];
_0 = <RangeInclusive<u32> as iter::range::RangeInclusiveIteratorImpl>::spec_next(move _1) -> [return: bb1, unwind continue];
}

bb1: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ fn slice_index_range(_1: &[u32], _2: std::ops::Range<usize>) -> &[u32] {

bb0: {
StorageLive(_3);
_3 = <std::ops::Range<usize> as SliceIndex<[u32]>>::index(move _2, _1) -> [return: bb1, unwind unreachable];
_3 = <std::ops::Range<usize> as SliceIndex<[u32]>>::index(move _2, move _1) -> [return: bb1, unwind unreachable];
}

bb1: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ fn slice_index_range(_1: &[u32], _2: std::ops::Range<usize>) -> &[u32] {

bb0: {
StorageLive(_3);
_3 = <std::ops::Range<usize> as SliceIndex<[u32]>>::index(move _2, _1) -> [return: bb1, unwind continue];
_3 = <std::ops::Range<usize> as SliceIndex<[u32]>>::index(move _2, move _1) -> [return: bb1, unwind continue];
}

bb1: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ fn slice_iter_mut_next_back(_1: &mut std::slice::IterMut<'_, T>) -> Option<&mut
let mut _0: std::option::Option<&mut T>;

bb0: {
_0 = <std::slice::IterMut<'_, T> as DoubleEndedIterator>::next_back(_1) -> [return: bb1, unwind unreachable];
_0 = <std::slice::IterMut<'_, T> as DoubleEndedIterator>::next_back(move _1) -> [return: bb1, unwind unreachable];
}

bb1: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ fn slice_iter_mut_next_back(_1: &mut std::slice::IterMut<'_, T>) -> Option<&mut
let mut _0: std::option::Option<&mut T>;

bb0: {
_0 = <std::slice::IterMut<'_, T> as DoubleEndedIterator>::next_back(_1) -> [return: bb1, unwind continue];
_0 = <std::slice::IterMut<'_, T> as DoubleEndedIterator>::next_back(move _1) -> [return: bb1, unwind continue];
}

bb1: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ fn slice_iter_next(_1: &mut std::slice::Iter<'_, T>) -> Option<&T> {
let mut _0: std::option::Option<&T>;

bb0: {
_0 = <std::slice::Iter<'_, T> as Iterator>::next(_1) -> [return: bb1, unwind unreachable];
_0 = <std::slice::Iter<'_, T> as Iterator>::next(move _1) -> [return: bb1, unwind unreachable];
}

bb1: {
Expand Down
Loading