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.

Author: tmiasko

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,7 +92,7 @@ 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;

tests/mir-opt/copy-prop/borrowed_local.borrow_in_loop.CopyProp.panic-abort.diff

@@ -46,12 +46,14 @@
 -         StorageLive(_6);
           StorageLive(_7);
           _7 = copy (*_2);
-          _6 = Not(move _7);
+-         _6 = Not(move _7);
++         _6 = Not(copy _7);
           StorageDead(_7);
 -         StorageLive(_8);
           StorageLive(_9);
           _9 = copy (*_2);
-          _8 = Not(move _9);
+-         _8 = Not(move _9);
++         _8 = Not(copy _9);
           StorageDead(_9);
 -         StorageLive(_10);
 -         _10 = copy _6;
@@ -62,7 +64,8 @@
           StorageLive(_12);
           _12 = &_1;
           _11 = &(*_12);
-          _2 = move _11;
+-         _2 = move _11;
++         _2 = copy _11;
           StorageDead(_11);
           StorageDead(_12);
           StorageLive(_13);
@@ -71,8 +74,9 @@
 -         StorageLive(_15);
 -         _15 = copy _8;
 -         _13 = Ne(move _14, move _15);
+-         switchInt(move _13) -> [0: bb3, otherwise: bb2];
 +         _13 = Ne(copy _6, copy _8);
-          switchInt(move _13) -> [0: bb3, otherwise: bb2];
++         switchInt(copy _13) -> [0: bb3, otherwise: bb2];
       }
 
       bb2: {

tests/mir-opt/copy-prop/cycle.main.CopyProp.panic-abort.diff

@@ -38,7 +38,8 @@
           StorageLive(_5);
           StorageLive(_6);
           _6 = copy _1;
-          _5 = std::mem::drop::<i32>(move _6) -> [return: bb2, unwind unreachable];
+-         _5 = std::mem::drop::<i32>(move _6) -> [return: bb2, unwind unreachable];
++         _5 = std::mem::drop::<i32>(copy _6) -> [return: bb2, unwind unreachable];
       }
 
       bb2: {

tests/mir-opt/copy-prop/dead_stores_79191.f.CopyProp.after.panic-abort.mir

@@ -16,7 +16,7 @@ fn f(_1: usize) -> usize {
         _1 = copy _2;
         StorageLive(_4);
         _4 = copy _1;
-        _0 = id::<usize>(move _4) -> [return: bb1, unwind unreachable];
+        _0 = id::<usize>(copy _4) -> [return: bb1, unwind unreachable];
     }
 
     bb1: {

tests/mir-opt/copy-prop/dead_stores_better.f.CopyProp.after.panic-abort.mir

@@ -16,7 +16,7 @@ fn f(_1: usize) -> usize {
         _1 = copy _2;
         StorageLive(_4);
         _4 = copy _1;
-        _0 = id::<usize>(move _4) -> [return: bb1, unwind unreachable];
+        _0 = id::<usize>(copy _4) -> [return: bb1, unwind unreachable];
     }
 
     bb1: {

tests/mir-opt/copy-prop/issue_107511.main.CopyProp.panic-abort.diff

@@ -46,22 +46,27 @@
           StorageLive(_6);
           StorageLive(_7);
           _7 = &_2;
-          _6 = move _7 as &[i32] (PointerCoercion(Unsize, Implicit));
+-         _6 = move _7 as &[i32] (PointerCoercion(Unsize, Implicit));
++         _6 = copy _7 as &[i32] (PointerCoercion(Unsize, Implicit));
           StorageDead(_7);
-          _5 = core::slice::<impl [i32]>::len(move _6) -> [return: bb1, unwind unreachable];
+-         _5 = core::slice::<impl [i32]>::len(move _6) -> [return: bb1, unwind unreachable];
++         _5 = core::slice::<impl [i32]>::len(copy _6) -> [return: bb1, unwind unreachable];
       }
 
       bb1: {
           StorageDead(_6);
-          _4 = std::ops::Range::<usize> { start: const 0_usize, end: move _5 };
+-         _4 = std::ops::Range::<usize> { start: const 0_usize, end: move _5 };
++         _4 = std::ops::Range::<usize> { start: const 0_usize, end: copy _5 };
           StorageDead(_5);
-          _3 = <std::ops::Range<usize> as IntoIterator>::into_iter(move _4) -> [return: bb2, unwind unreachable];
+-         _3 = <std::ops::Range<usize> as IntoIterator>::into_iter(move _4) -> [return: bb2, unwind unreachable];
++         _3 = <std::ops::Range<usize> as IntoIterator>::into_iter(copy _4) -> [return: bb2, unwind unreachable];
       }
 
       bb2: {
           StorageDead(_4);
           StorageLive(_8);
-          _8 = move _3;
+-         _8 = move _3;
++         _8 = copy _3;
           goto -> bb3;
       }
 
@@ -72,13 +77,15 @@
           StorageLive(_13);
           _13 = &mut _8;
           _12 = &mut (*_13);
-          _11 = <std::ops::Range<usize> as Iterator>::next(move _12) -> [return: bb4, unwind unreachable];
+-         _11 = <std::ops::Range<usize> as Iterator>::next(move _12) -> [return: bb4, unwind unreachable];
++         _11 = <std::ops::Range<usize> as Iterator>::next(copy _12) -> [return: bb4, unwind unreachable];
       }
 
       bb4: {
           StorageDead(_12);
           _14 = discriminant(_11);
-          switchInt(move _14) -> [0: bb7, 1: bb6, otherwise: bb5];
+-         switchInt(move _14) -> [0: bb7, 1: bb6, otherwise: bb5];
++         switchInt(copy _14) -> [0: bb7, 1: bb6, otherwise: bb5];
       }
 
       bb5: {
@@ -94,7 +101,7 @@
 -         _19 = Lt(copy _18, const 4_usize);
 -         assert(move _19, "index out of bounds: the length is {} but the index is {}", const 4_usize, copy _18) -> [success: bb8, unwind unreachable];
 +         _19 = Lt(copy _16, const 4_usize);
-+         assert(move _19, "index out of bounds: the length is {} but the index is {}", const 4_usize, copy _16) -> [success: bb8, unwind unreachable];
++         assert(copy _19, "index out of bounds: the length is {} but the index is {}", const 4_usize, copy _16) -> [success: bb8, unwind unreachable];
       }
 
       bb7: {
@@ -111,8 +118,9 @@
 
       bb8: {
 -         _17 = copy _2[_18];
+-         _1 = Add(copy _1, move _17);
 +         _17 = copy _2[_16];
-          _1 = Add(copy _1, move _17);
++         _1 = Add(copy _1, copy _17);
           StorageDead(_17);
 -         StorageDead(_18);
 -         _10 = const ();

tests/mir-opt/copy-prop/move_arg.f.CopyProp.panic-abort.diff

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

@@ -15,7 +15,8 @@
       }
 
       bb1: {
-          _0 = opaque::<u8>(move _3) -> [return: bb2, unwind unreachable];
+-         _0 = opaque::<u8>(move _3) -> [return: bb2, unwind unreachable];
++         _0 = opaque::<u8>(copy _3) -> [return: bb2, unwind unreachable];
       }
 
       bb2: {