Auto merge of #147804 - tmiasko:move-copy, r=cjgillot,saethlin

Turn moves into copies after copy propagation

Previously copy propagation presumed that there is further unspecified distinction between move operands and copy operands in assignments and propagated moves from assignments into terminators. This is inconsistent with current operational semantics.

Turn moves into copies after copy propagation to preserve existing behavior.

Fixes #137936.
Fixes #146423.

r? `@cjgillot`

Author: bors

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.f.CopyProp.panic-abort.diff

@@ -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/move_arg.f.CopyProp.panic-unwind.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/move_arg.rs

@@ -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.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

@@ -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
 }