Turn moves into copies after copy propagation

compiler/rustc_mir_transform/src/copy_prop.rs

@@ -16,7 +16,7 @@ use crate::ssa::SsaLocals;
 /// _d = move? _c
 /// where each of the locals is only assigned once.
 ///
-/// We want to replace all those locals by `_a`, either copied or moved.
+/// We want to replace all those locals by `copy _a`.
 pub(super) struct CopyProp;
 
 impl<'tcx> crate::MirPass<'tcx> for CopyProp {
@@ -34,23 +34,21 @@ impl<'tcx> crate::MirPass<'tcx> for CopyProp {
  debug!(copy_classes = ?ssa.copy_classes());
 
  let mut any_replacement = false;
- let mut storage_to_remove = DenseBitSet::new_empty(body.local_decls.len());
+ // Locals that participate in copy propagation either as a source or a destination.
+ let mut unified = DenseBitSet::new_empty(body.local_decls.len());
  for (local, &head) in ssa.copy_classes().iter_enumerated() {
  if local != head {
  any_replacement = true;
- storage_to_remove.insert(head);
+ unified.insert(head);
+ unified.insert(local);
  }
  }
 
  if !any_replacement {
  return;
  }
 
- let fully_moved = fully_moved_locals(&ssa, body);
- debug!(?fully_moved);
-
- Replacer { tcx, copy_classes: ssa.copy_classes(), fully_moved, storage_to_remove }
- .visit_body_preserves_cfg(body);
+ Replacer { tcx, copy_classes: ssa.copy_classes(), unified }.visit_body_preserves_cfg(body);
 
  crate::simplify::remove_unused_definitions(body);
  }
@@ -60,44 +58,10 @@ impl<'tcx> crate::MirPass<'tcx> for CopyProp {
  }
 }
 
-/// `SsaLocals` computed equivalence classes between locals considering copy/move assignments.
-///
-/// This function also returns whether all the `move?` in the pattern are `move` and not copies.
-/// A local which is in the bitset can be replaced by `move _a`. Otherwise, it must be
-/// replaced by `copy _a`, as we cannot move multiple times from `_a`.
-///
-/// If an operand copies `_c`, it must happen before the assignment `_d = _c`, otherwise it is UB.
-/// This means that replacing it by a copy of `_a` if ok, since this copy happens before `_c` is
-/// moved, and therefore that `_d` is moved.
-#[instrument(level = "trace", skip(ssa, body))]
-fn fully_moved_locals(ssa: &SsaLocals, body: &Body<'_>) -> DenseBitSet<Local> {
- let mut fully_moved = DenseBitSet::new_filled(body.local_decls.len());
-
- for (_, rvalue, _) in ssa.assignments(body) {
- let Rvalue::Use(Operand::Copy(place) | Operand::Move(place)) = rvalue else {
- continue;
- };
-
- let Some(rhs) = place.as_local() else { continue };
- if !ssa.is_ssa(rhs) {
- continue;
- }
-
- if let Rvalue::Use(Operand::Copy(_)) = rvalue {
- fully_moved.remove(rhs);
- }
- }
-
- ssa.meet_copy_equivalence(&mut fully_moved);
-
- fully_moved
-}
-
 /// Utility to help performing substitution of `*pattern` by `target`.
 struct Replacer<'a, 'tcx> {
  tcx: TyCtxt<'tcx>,
- fully_moved: DenseBitSet<Local>,
- storage_to_remove: DenseBitSet<Local>,
+ unified: DenseBitSet<Local>,
  copy_classes: &'a IndexSlice<Local, Local>,
 }
 
@@ -108,13 +72,7 @@ impl<'tcx> MutVisitor<'tcx> for Replacer<'_, 'tcx> {
 
  #[tracing::instrument(level = "trace", skip(self))]
  fn visit_local(&mut self, local: &mut Local, ctxt: PlaceContext, _: Location) {
- let new_local = self.copy_classes[*local];
- match ctxt {
- // Do not modify the local in storage statements.
- PlaceContext::NonUse(NonUseContext::StorageLive | NonUseContext::StorageDead) => {}
- // We access the value.
- _ => *local = new_local,
- }
+ *local = self.copy_classes[*local];
  }
 
  #[tracing::instrument(level = "trace", skip(self))]
@@ -123,7 +81,7 @@ impl<'tcx> MutVisitor<'tcx> for Replacer<'_, 'tcx> {
  // A move out of a projection of a copy is equivalent to a copy of the original
  // projection.
  && !place.is_indirect_first_projection()
- && !self.fully_moved.contains(place.local)
+ && self.unified.contains(place.local)
  {
  *operand = Operand::Copy(place);
  }
@@ -134,10 +92,9 @@ impl<'tcx> MutVisitor<'tcx> for Replacer<'_, 'tcx> {
  fn visit_statement(&mut self, stmt: &mut Statement<'tcx>, loc: Location) {
  // When removing storage statements, we need to remove both (#107511).
  if let StatementKind::StorageLive(l) | StatementKind::StorageDead(l) = stmt.kind
- && self.storage_to_remove.contains(l)
+ && self.unified.contains(l)
  {
  stmt.make_nop(true);
- return;
  }
 
  self.super_statement(stmt, loc);

tests/mir-opt/copy-prop/move_arg.rs

@@ -1,17 +1,46 @@
-// skip-filecheck
-// EMIT_MIR_FOR_EACH_PANIC_STRATEGY
 // Test that we do not move multiple times from the same local.
 //@ test-mir-pass: CopyProp
+//@ compile-flags: --crate-type=lib -Cpanic=abort
+#![feature(custom_mir, core_intrinsics)]
+use core::intrinsics::mir::*;
+use core::mem::MaybeUninit;
+extern crate core;
 
-// EMIT_MIR move_arg.f.CopyProp.diff
-pub fn f<T: Copy>(a: T) {
- let b = a;
- g(a, b);
+#[custom_mir(dialect = "runtime", phase = "initial")]
+pub fn moved_and_copied<T: Copy>(_1: T) {
+ // CHECK-LABEL: fn moved_and_copied(
+ // CHECK: _0 = f::<T>(copy _1, copy _1)
+ mir! {
+ {
+ let _2 = _1;
+ Call(RET = f(Move(_1), Move(_2)), ReturnTo(bb1), UnwindUnreachable())
+ }
+ bb1 = {
+ Return()
+ }
+ }
 }
 
-#[inline(never)]
-pub fn g<T: Copy>(_: T, _: T) {}
-
-fn main() {
- f(5)
+#[custom_mir(dialect = "runtime", phase = "initial")]
+pub fn moved_twice<T: Copy>(_1: MaybeUninit<T>) {
+ // In a future we would like to propagate moves instead of copies here. The resulting program
+ // would have an undefined behavior due to overlap in a call terminator, so we need to change
+ // operational semantics to explain why the original program has undefined behavior.
+ // https://github.com/rust-lang/unsafe-code-guidelines/issues/556
+ //
+ // CHECK-LABEL: fn moved_twice(
+ // CHECK: _0 = f::<MaybeUninit<T>>(copy _1, copy _1)
+ mir! {
+ {
+ let _2 = Move(_1);
+ let _3 = Move(_1);
+ Call(RET = f(Move(_2), Move(_3)), ReturnTo(bb1), UnwindUnreachable())
+ }
+ bb1 = {
+ Return()
+ }
+ }
 }
+
+#[inline(never)]
+pub fn f<T: Copy>(_: T, _: T) {}

tests/mir-opt/copy-prop/reborrow.remut.CopyProp.panic-abort.diff

@@ -31,7 +31,7 @@
 - StorageLive(_6);
 - _6 = move _4;
 - _5 = opaque::<&mut u8>(move _6) -> [return: bb1, unwind unreachable];
-+ _5 = opaque::<&mut u8>(move _2) -> [return: bb1, unwind unreachable];
++ _5 = opaque::<&mut u8>(copy _2) -> [return: bb1, unwind unreachable];
  }
 
  bb1: {

tests/mir-opt/copy-prop/reborrow.remut.CopyProp.panic-unwind.diff

@@ -31,7 +31,7 @@
 - StorageLive(_6);
 - _6 = move _4;
 - _5 = opaque::<&mut u8>(move _6) -> [return: bb1, unwind continue];
-+ _5 = opaque::<&mut u8>(move _2) -> [return: bb1, unwind continue];
++ _5 = opaque::<&mut u8>(copy _2) -> [return: bb1, unwind continue];
  }
 
  bb1: {

tests/mir-opt/copy-prop/reborrow.reraw.CopyProp.panic-abort.diff

@@ -31,7 +31,7 @@
 - StorageLive(_6);
 - _6 = move _4;
 - _5 = opaque::<&mut u8>(move _6) -> [return: bb1, unwind unreachable];
-+ _5 = opaque::<&mut u8>(move _2) -> [return: bb1, unwind unreachable];
++ _5 = opaque::<&mut u8>(copy _2) -> [return: bb1, unwind unreachable];
  }
 
  bb1: {

tests/mir-opt/copy-prop/reborrow.reraw.CopyProp.panic-unwind.diff

@@ -31,7 +31,7 @@
 - StorageLive(_6);
 - _6 = move _4;
 - _5 = opaque::<&mut u8>(move _6) -> [return: bb1, unwind continue];
-+ _5 = opaque::<&mut u8>(move _2) -> [return: bb1, unwind continue];
++ _5 = opaque::<&mut u8>(copy _2) -> [return: bb1, unwind continue];
  }
 
  bb1: {

tests/ui/lint/large_assignments/inline_mir.rs

@@ -20,5 +20,5 @@
 pub fn main() {
  let data = [10u8; 9999];
  let cell = std::cell::UnsafeCell::new(data); //~ ERROR large_assignments
- std::hint::black_box(cell);
+ std::hint::black_box(cell); //~ ERROR large_assignments
 }

tests/ui/lint/large_assignments/inline_mir.stderr

@@ -11,5 +11,13 @@ note: the lint level is defined here
 LL | #![deny(large_assignments)]
  | ^^^^^^^^^^^^^^^^^
 
-error: aborting due to 1 previous error
+error: moving 9999 bytes
+ --> $DIR/inline_mir.rs:23:5
+ |
+LL | std::hint::black_box(cell);
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^ value moved from here
+ |
+ = note: The current maximum size is 1000, but it can be customized with the move_size_limit attribute: `#![move_size_limit = "..."]`
+
+error: aborting due to 2 previous errors
 

tests/ui/lint/large_assignments/move_into_fn.rs

@@ -16,7 +16,7 @@ fn main() {
  // Looking at llvm-ir output, we can see that there is no memcpy involved in
  // this function call. Instead, just a pointer is passed to the function. So
  // the lint shall not trigger here.
- take_data(data);
+ take_data(data); //~ ERROR large_assignments
 }
 
 fn take_data(data: Data) {}

tests/ui/lint/large_assignments/move_into_fn.stderr

@@ -11,5 +11,13 @@ note: the lint level is defined here
 LL | #![deny(large_assignments)]
  | ^^^^^^^^^^^^^^^^^
 
-error: aborting due to 1 previous error
+error: moving 9999 bytes
+ --> $DIR/move_into_fn.rs:19:15
+ |
+LL | take_data(data);
+ | ^^^^ value moved from here
+ |
+ = note: The current maximum size is 1000, but it can be customized with the move_size_limit attribute: `#![move_size_limit = "..."]`
+
+error: aborting due to 2 previous errors