Rollup merge of #94305 - JakobDegen:dp-1, r=oli-obk

Remove an unnecessary restriction in `dest_prop`

I had asked about this [on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Do.20unions.20have.20active.20fields.3F) but didn't receive a response, so putting up this PR that makes the change I think we can. If it turns out that this is wrong, hopefully I'll find out here. Reposting my Zulip comment:
> Not sure what channel to put this into, so using this as a fallback. The dest prop MIR opt has this comment:
>
> ```rust
> //!   Subtle case: If `dest` is a, or projects through a union, then we have to make sure that there
> //!   remains an assignment to it, since that sets the "active field" of the union. But if `src` is
> //!   a ZST, it might not be initialized, so there might not be any use of it before the assignment,
> //!   and performing the optimization would simply delete the assignment, leaving `dest`
> //!   uninitialized.
> ```
>
> In particular, the claim seems to be that we can't take
> ```
> x = ();
> y.field = x;
> ```
> where `y` is a union having `field: ()` as one of its variants, and optimize the entire thing away (assuming `x` is unused otherwise). As far as I know though, Rust unions don't have active fields. Is this comment correct and am I missing something? Is there a worry about this interacting poorly with FFI code/C unions/LTO or something?

This PR just removes that comment and the associated code. Also it fixes one unrelated comment that did not match the code it was commenting on.

r? rust-lang/mir-opt

Author: matthiaskrgr

compiler/rustc_mir_transform/src/dest_prop.rs

@@ -38,12 +38,6 @@
 //!   It must also not contain any indexing projections, since those take an arbitrary `Local` as
 //!   the index, and that local might only be initialized shortly before `dest` is used.
 //!
-//!   Subtle case: If `dest` is a, or projects through a union, then we have to make sure that there
-//!   remains an assignment to it, since that sets the "active field" of the union. But if `src` is
-//!   a ZST, it might not be initialized, so there might not be any use of it before the assignment,
-//!   and performing the optimization would simply delete the assignment, leaving `dest`
-//!   uninitialized.
-//!
 //! * `src` must be a bare `Local` without any indirections or field projections (FIXME: Is this a
 //!   fundamental restriction or just current impl state?). It can be copied or moved by the
 //!   assignment.
@@ -103,7 +97,6 @@ use rustc_index::{
     bit_set::{BitMatrix, BitSet},
     vec::IndexVec,
 };
-use rustc_middle::mir::tcx::PlaceTy;
 use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor};
 use rustc_middle::mir::{dump_mir, PassWhere};
 use rustc_middle::mir::{
@@ -135,7 +128,7 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation {
     fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
         let def_id = body.source.def_id();
 
-        let candidates = find_candidates(tcx, body);
+        let candidates = find_candidates(body);
         if candidates.is_empty() {
             debug!("{:?}: no dest prop candidates, done", def_id);
             return;
@@ -803,9 +796,8 @@ struct CandidateAssignment<'tcx> {
 /// comment) and also throw out assignments that involve a local that has its address taken or is
 /// otherwise ineligible (eg. locals used as array indices are ignored because we cannot propagate
 /// arbitrary places into array indices).
-fn find_candidates<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) -> Vec<CandidateAssignment<'tcx>> {
+fn find_candidates<'tcx>(body: &Body<'tcx>) -> Vec<CandidateAssignment<'tcx>> {
     let mut visitor = FindAssignments {
-        tcx,
         body,
         candidates: Vec::new(),
         ever_borrowed_locals: ever_borrowed_locals(body),
@@ -816,7 +808,6 @@ fn find_candidates<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) -> Vec<CandidateA
 }
 
 struct FindAssignments<'a, 'tcx> {
-    tcx: TyCtxt<'tcx>,
     body: &'a Body<'tcx>,
     candidates: Vec<CandidateAssignment<'tcx>>,
     ever_borrowed_locals: BitSet<Local>,
@@ -845,10 +836,11 @@ impl<'tcx> Visitor<'tcx> for FindAssignments<'_, 'tcx> {
                 return;
             }
 
-            // Can't optimize if both locals ever have their address taken (can introduce
-            // aliasing).
-            // FIXME: This can be smarter and take `StorageDead` into account (which
-            // invalidates borrows).
+            // Can't optimize if either local ever has their address taken. This optimization does
+            // liveness analysis only based on assignments, and a local can be live even if its
+            // never assigned to again, because a reference to it might be live.
+            // FIXME: This can be smarter and take `StorageDead` into  account (which invalidates
+            // borrows).
             if self.ever_borrowed_locals.contains(dest.local)
                 || self.ever_borrowed_locals.contains(src.local)
             {
@@ -862,22 +854,11 @@ impl<'tcx> Visitor<'tcx> for FindAssignments<'_, 'tcx> {
                 return;
             }
 
-            // Handle the "subtle case" described above by rejecting any `dest` that is or
-            // projects through a union.
-            let mut place_ty = PlaceTy::from_ty(self.body.local_decls[dest.local].ty);
-            if place_ty.ty.is_union() {
-                return;
-            }
             for elem in dest.projection {
                 if let PlaceElem::Index(_) = elem {
                     // `dest` contains an indexing projection.
                     return;
                 }
-
-                place_ty = place_ty.projection_ty(self.tcx, elem);
-                if place_ty.ty.is_union() {
-                    return;
-                }
             }
 
             self.candidates.push(CandidateAssignment {

src/test/mir-opt/dest-prop/union.main.DestinationPropagation.diff

@@ -17,23 +17,29 @@
       }
 
       bb0: {
-          StorageLive(_1);                 // scope 0 at $DIR/union.rs:13:9: 13:11
-          StorageLive(_2);                 // scope 0 at $DIR/union.rs:13:23: 13:28
-          _2 = val() -> bb1;               // scope 0 at $DIR/union.rs:13:23: 13:28
+-         StorageLive(_1);                 // scope 0 at $DIR/union.rs:13:9: 13:11
+-         StorageLive(_2);                 // scope 0 at $DIR/union.rs:13:23: 13:28
+-         _2 = val() -> bb1;               // scope 0 at $DIR/union.rs:13:23: 13:28
++         nop;                             // scope 0 at $DIR/union.rs:13:9: 13:11
++         nop;                             // scope 0 at $DIR/union.rs:13:23: 13:28
++         (_1.0: u32) = val() -> bb1;      // scope 0 at $DIR/union.rs:13:23: 13:28
                                            // mir::Constant
                                            // + span: $DIR/union.rs:13:23: 13:26
                                            // + literal: Const { ty: fn() -> u32 {val}, val: Value(Scalar(<ZST>)) }
       }
 
       bb1: {
-          (_1.0: u32) = move _2;           // scope 0 at $DIR/union.rs:13:14: 13:30
-          StorageDead(_2);                 // scope 0 at $DIR/union.rs:13:29: 13:30
+-         (_1.0: u32) = move _2;           // scope 0 at $DIR/union.rs:13:14: 13:30
+-         StorageDead(_2);                 // scope 0 at $DIR/union.rs:13:29: 13:30
++         nop;                             // scope 0 at $DIR/union.rs:13:14: 13:30
++         nop;                             // scope 0 at $DIR/union.rs:13:29: 13:30
           StorageLive(_3);                 // scope 1 at $DIR/union.rs:15:5: 15:27
           StorageLive(_4);                 // scope 1 at $DIR/union.rs:15:10: 15:26
           _4 = (_1.0: u32);                // scope 2 at $DIR/union.rs:15:19: 15:24
           StorageDead(_4);                 // scope 1 at $DIR/union.rs:15:26: 15:27
           StorageDead(_3);                 // scope 1 at $DIR/union.rs:15:27: 15:28
-          StorageDead(_1);                 // scope 0 at $DIR/union.rs:16:1: 16:2
+-         StorageDead(_1);                 // scope 0 at $DIR/union.rs:16:1: 16:2
++         nop;                             // scope 0 at $DIR/union.rs:16:1: 16:2
           return;                          // scope 0 at $DIR/union.rs:16:2: 16:2
       }
   }

src/test/mir-opt/dest-prop/union.rs

@@ -1,4 +1,4 @@
-//! Tests that projections through unions cancel `DestinationPropagation`.
+//! Tests that we can propogate into places that are projections into unions
 // compile-flags: -Zunsound-mir-opts
 fn val() -> u32 {
     1