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/codegen-llvm/align-byval.rs

@@ -1,7 +1,7 @@
 // ignore-tidy-linelength
 //@ add-minicore
 //@ revisions:m68k x86_64-linux x86_64-windows i686-linux i686-windows
-
+//@ compile-flags: -Copt-level=1 -Cno-prepopulate-passes
 //@[m68k] compile-flags: --target m68k-unknown-linux-gnu
 //@[m68k] needs-llvm-components: m68k
 //@[x86_64-linux] compile-flags: --target x86_64-unknown-linux-gnu

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

@@ -1,20 +1,46 @@
-// 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) {
-    // CHECK-LABEL: fn f(
-    // CHECK: debug a => [[a:_.*]];
-    // CHECK: debug b => [[a]];
-    // CHECK: g::<T>(copy [[a]], copy [[a]])
-    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