InstCombine can incorrectly deinitialize locals that are later used

[jonas-schievink]

Since #72093 InstCombine can now introduce Operand::Move where there was none before, without checking that the value is in fact not used afterwards. This may cause a value to be marked as uninitialized by the MaybeInitializedLocals dataflow, but still be used in that uninitialized state.

If InstCombine ran before the generator transform this would be acutely unsound and could probably be exploited directly, but other than that transform I don't think we rely on this property yet.

This impacts #72632, which does run after InstCombine and relies on the MaybeInitializedLocals dataflow.

I've observed this on BinaryHeap::peek_mut, where it produces this diff:

-// MIR for `collections::binary_heap::<impl at src/liballoc/collections/binary_heap.rs:338:1: 696:2>::peek_mut` before InstCombine
+// MIR for `collections::binary_heap::<impl at src/liballoc/collections/binary_heap.rs:338:1: 696:2>::peek_mut` after InstCombine
 
 fn collections::binary_heap::<impl at src/liballoc/collections/binary_heap.rs:338:1: 696:2>::peek_mut(_1: &mut collections::binary_heap::BinaryHeap<T>) -> core::option::Option<collections::binary_heap::PeekMut<T>> {
     debug self => _1;                    // in scope 0 at src/liballoc/collections/binary_heap.rs:403:21: 403:30
     let mut _0: core::option::Option<collections::binary_heap::PeekMut<T>>; // return place in scope 0 at src/liballoc/collections/binary_heap.rs:403:35: 403:57
     let mut _2: bool;                    // in scope 0 at src/liballoc/collections/binary_heap.rs:404:12: 404:27
     let mut _3: &collections::binary_heap::BinaryHeap<T>; // in scope 0 at src/liballoc/collections/binary_heap.rs:404:12: 404:16
     let mut _4: collections::binary_heap::PeekMut<T>; // in scope 0 at src/liballoc/collections/binary_heap.rs:404:49: 404:83
     let mut _5: &mut collections::binary_heap::BinaryHeap<T>; // in scope 0 at src/liballoc/collections/binary_heap.rs:404:65: 404:69
 
     bb0: {
         StorageLive(_2);                 // scope 0 at src/liballoc/collections/binary_heap.rs:404:12: 404:27
         StorageLive(_3);                 // scope 0 at src/liballoc/collections/binary_heap.rs:404:12: 404:16
-        _3 = &(*_1);                     // scope 0 at src/liballoc/collections/binary_heap.rs:404:12: 404:16
+        _3 = move _1;                    // scope 0 at src/liballoc/collections/binary_heap.rs:404:12: 404:16
         _2 = const collections::binary_heap::BinaryHeap::<T>::is_empty(move _3) -> bb1; // scope 0 at src/liballoc/collections/binary_heap.rs:404:12: 404:27
                                          // ty::Const
                                          // + ty: for<'r> fn(&'r collections::binary_heap::BinaryHeap<T>) -> bool {collections::binary_heap::BinaryHeap::<T>::is_empty}
                                          // + val: Value(Scalar(<ZST>))
                                          // mir::Constant
                                          // + span: src/liballoc/collections/binary_heap.rs:404:17: 404:25
                                          // + literal: Const { ty: for<'r> fn(&'r collections::binary_heap::BinaryHeap<T>) -> bool {collections::binary_heap::BinaryHeap::<T>::is_empty}, val: Value(Scalar(<ZST>)) }
     }
 
     bb1: {
         StorageDead(_3);                 // scope 0 at src/liballoc/collections/binary_heap.rs:404:26: 404:27
         switchInt(_2) -> [false: bb2, otherwise: bb3]; // scope 0 at src/liballoc/collections/binary_heap.rs:404:9: 404:86
     }
 
     bb2: {
         StorageLive(_4);                 // scope 0 at src/liballoc/collections/binary_heap.rs:404:49: 404:83
         StorageLive(_5);                 // scope 0 at src/liballoc/collections/binary_heap.rs:404:65: 404:69
-        _5 = &mut (*_1);                 // scope 0 at src/liballoc/collections/binary_heap.rs:404:65: 404:69
+        _5 = move _1;                    // scope 0 at src/liballoc/collections/binary_heap.rs:404:65: 404:69
         _4 = collections::binary_heap::PeekMut::<T> { heap: move _5, sift: const true }; // scope 0 at src/liballoc/collections/binary_heap.rs:404:49: 404:83
                                          // ty::Const
                                          // + ty: bool
                                          // + val: Value(Scalar(0x01))
                                          // mir::Constant
                                          // + span: src/liballoc/collections/binary_heap.rs:404:77: 404:81
                                          // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
         StorageDead(_5);                 // scope 0 at src/liballoc/collections/binary_heap.rs:404:82: 404:83
         _0 = core::option::Option::<collections::binary_heap::PeekMut<T>>::Some(move _4); // scope 0 at src/liballoc/collections/binary_heap.rs:404:44: 404:84
         StorageDead(_4);                 // scope 0 at src/liballoc/collections/binary_heap.rs:404:83: 404:84
         goto -> bb4;                     // scope 0 at src/liballoc/collections/binary_heap.rs:404:9: 404:86
     }
 
     bb3: {
         _0 = core::option::Option::<collections::binary_heap::PeekMut<T>>::None; // scope 0 at src/liballoc/collections/binary_heap.rs:404:30: 404:34
         goto -> bb4;                     // scope 0 at src/liballoc/collections/binary_heap.rs:404:9: 404:86
     }
 
     bb4: {
         StorageDead(_2);                 // scope 0 at src/liballoc/collections/binary_heap.rs:405:5: 405:6
         return;                          // scope 0 at src/liballoc/collections/binary_heap.rs:405:6: 405:6
     }
 }

The second use of _1 occurs despite the first move deinitializing the local.

cc @rust-lang/wg-mir-opt

[oli-obk]

Maybe we should stop making instcombine convert &*some_mut_ref to some_mut_ref? This could be done in the pre-codegen cleanups, but for MIR it is wrong to use Copy or Move on such values in general.

Alternatively instcombine needs to start using dataflow to only insert moves when the site is the last use of the local.