Various hacks around storage statements

Author: Amanieu

compiler/rustc_mir_build/src/builder/scope.rs

@@ -268,7 +268,7 @@ impl Scope {
     fn needs_cleanup(&self) -> bool {
         self.drops.iter().any(|drop| match drop.kind {
             DropKind::Value | DropKind::ForLint => true,
-            DropKind::Storage => false,
+            DropKind::Storage => true,
         })
     }
 
@@ -1239,7 +1239,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             block,
             unwind_to,
             dropline_to,
-            is_coroutine && needs_cleanup,
+            true && needs_cleanup,
             self.arg_count,
             |v: Local| Self::is_async_drop_impl(self.tcx, &self.local_decls, typing_env, v),
         )
@@ -1479,7 +1479,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         // path, we only need to invalidate the cache for drops that happen on
         // the unwind or coroutine drop paths. This means that for
         // non-coroutines we don't need to invalidate caches for `DropKind::Storage`.
-        let invalidate_caches = needs_drop || self.coroutine.is_some();
+        let invalidate_caches = needs_drop || self.coroutine.is_some() || true;
         for scope in self.scopes.scopes.iter_mut().rev() {
             if invalidate_caches {
                 scope.invalidate_cache();
@@ -1630,7 +1630,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         let is_coroutine = self.coroutine.is_some();
         for scope in &mut self.scopes.scopes[uncached_scope..=target] {
             for drop in &scope.drops {
-                if is_coroutine || drop.kind == DropKind::Value {
+                if is_coroutine || true || drop.kind == DropKind::Value {
                     cached_drop = self.scopes.unwind_drops.add_drop(*drop, cached_drop);
                 }
             }
@@ -2005,7 +2005,7 @@ impl<'a, 'tcx: 'a> Builder<'a, 'tcx> {
             for (drop_idx, drop_node) in drops.drop_nodes.iter_enumerated().skip(1) {
                 match drop_node.data.kind {
                     DropKind::Storage | DropKind::ForLint => {
-                        if is_coroutine {
+                        if true || is_coroutine {
                             let unwind_drop = self
                                 .scopes
                                 .unwind_drops

compiler/rustc_mir_transform/src/inline.rs

@@ -904,7 +904,7 @@ fn inline_call<'tcx, I: Inliner<'tcx>>(
             destination,
         );
         let dest_ty = dest.ty(caller_body, tcx);
-        let temp = Place::from(new_call_temp(caller_body, callsite, dest_ty, return_block));
+        let temp = Place::from(new_call_temp(caller_body, callsite, dest_ty, return_block, unwind));
         caller_body[callsite.block].statements.push(Statement::new(
             callsite.source_info,
             StatementKind::Assign(Box::new((temp, dest))),
@@ -921,12 +921,19 @@ fn inline_call<'tcx, I: Inliner<'tcx>>(
     } else {
         (
             true,
-            new_call_temp(caller_body, callsite, destination.ty(caller_body, tcx).ty, return_block),
+            new_call_temp(
+                caller_body,
+                callsite,
+                destination.ty(caller_body, tcx).ty,
+                return_block,
+                unwind,
+            ),
         )
     };
 
     // Copy the arguments if needed.
-    let args = make_call_args(inliner, args, callsite, caller_body, &callee_body, return_block);
+    let args =
+        make_call_args(inliner, args, callsite, caller_body, &callee_body, return_block, unwind);
 
     let mut integrator = Integrator {
         args: &args,
@@ -983,6 +990,25 @@ fn inline_call<'tcx, I: Inliner<'tcx>>(
         }
         caller_body[block].statements.rotate_right(n);
     }
+    if let UnwindAction::Cleanup(block) = unwind {
+        // To avoid repeated O(n) insert, push any new statements to the end and rotate
+        // the slice once.
+        let mut n = 0;
+        for local in callee_body.vars_and_temps_iter().rev() {
+            // Do this for *all* locals instead of just the always-live ones.
+            // This is necessary because StorageDead is often not preserved on
+            // unwind paths.
+            //
+            // FIXME: Restrict this to only locals that are storage-live at an
+            // UnwindAction::Continue.
+            let new_local = integrator.map_local(local);
+            caller_body[block]
+                .statements
+                .push(Statement::new(callsite.source_info, StatementKind::StorageDead(new_local)));
+            n += 1;
+        }
+        caller_body[block].statements.rotate_right(n);
+    }
 
     // Insert all of the (mapped) parts of the callee body into the caller.
     caller_body.local_decls.extend(callee_body.drain_vars_and_temps());
@@ -1045,6 +1071,7 @@ fn make_call_args<'tcx, I: Inliner<'tcx>>(
     caller_body: &mut Body<'tcx>,
     callee_body: &Body<'tcx>,
     return_block: Option<BasicBlock>,
+    unwind: UnwindAction,
 ) -> Box<[Local]> {
     let tcx = inliner.tcx();
 
@@ -1080,13 +1107,15 @@ fn make_call_args<'tcx, I: Inliner<'tcx>>(
             callsite,
             caller_body,
             return_block,
+            unwind,
         );
         let tuple = create_temp_if_necessary(
             inliner,
             args.next().unwrap().node,
             callsite,
             caller_body,
             return_block,
+            unwind,
         );
         assert!(args.next().is_none());
 
@@ -1104,13 +1133,29 @@ fn make_call_args<'tcx, I: Inliner<'tcx>>(
             let tuple_field = Operand::Move(tcx.mk_place_field(tuple, FieldIdx::new(i), ty));
 
             // Spill to a local to make e.g., `tmp0`.
-            create_temp_if_necessary(inliner, tuple_field, callsite, caller_body, return_block)
+            create_temp_if_necessary(
+                inliner,
+                tuple_field,
+                callsite,
+                caller_body,
+                return_block,
+                unwind,
+            )
         });
 
         closure_ref_arg.chain(tuple_tmp_args).collect()
     } else {
         args.into_iter()
-            .map(|a| create_temp_if_necessary(inliner, a.node, callsite, caller_body, return_block))
+            .map(|a| {
+                create_temp_if_necessary(
+                    inliner,
+                    a.node,
+                    callsite,
+                    caller_body,
+                    return_block,
+                    unwind,
+                )
+            })
             .collect()
     }
 }
@@ -1123,6 +1168,7 @@ fn create_temp_if_necessary<'tcx, I: Inliner<'tcx>>(
     callsite: &CallSite<'tcx>,
     caller_body: &mut Body<'tcx>,
     return_block: Option<BasicBlock>,
+    unwind: UnwindAction,
 ) -> Local {
     // Reuse the operand if it is a moved temporary.
     if let Operand::Move(place) = &arg
@@ -1135,7 +1181,7 @@ fn create_temp_if_necessary<'tcx, I: Inliner<'tcx>>(
     // Otherwise, create a temporary for the argument.
     trace!("creating temp for argument {:?}", arg);
     let arg_ty = arg.ty(caller_body, inliner.tcx());
-    let local = new_call_temp(caller_body, callsite, arg_ty, return_block);
+    let local = new_call_temp(caller_body, callsite, arg_ty, return_block, unwind);
     caller_body[callsite.block].statements.push(Statement::new(
         callsite.source_info,
         StatementKind::Assign(Box::new((Place::from(local), Rvalue::Use(arg, WithRetag::Yes)))),
@@ -1149,6 +1195,7 @@ fn new_call_temp<'tcx>(
     callsite: &CallSite<'tcx>,
     ty: Ty<'tcx>,
     return_block: Option<BasicBlock>,
+    unwind: UnwindAction,
 ) -> Local {
     let local = caller_body.local_decls.push(LocalDecl::new(ty, callsite.source_info.span));
 
@@ -1161,6 +1208,11 @@ fn new_call_temp<'tcx>(
             .statements
             .insert(0, Statement::new(callsite.source_info, StatementKind::StorageDead(local)));
     }
+    if let UnwindAction::Cleanup(block) = unwind {
+        caller_body[block]
+            .statements
+            .insert(0, Statement::new(callsite.source_info, StatementKind::StorageDead(local)));
+    }
 
     local
 }

compiler/rustc_mir_transform/src/move_elimination.rs

@@ -39,12 +39,12 @@ impl<'tcx> crate::MirPass<'tcx> for MoveElimination {
         let remapped_locals =
             PlaceUnification::run(tcx, body, &mut liveness_matrix, &mut unprojectable_locals);
 
-        dump_liveness_matrix(tcx, body, "MoveElimination.post-liveness", &points, &liveness_matrix);
+        apply_mappings(tcx, body, &remapped_locals);
 
-        let storage_to_rebuild = apply_mappings(tcx, body, &remapped_locals);
+        dump_liveness_matrix(tcx, body, "MoveElimination.post-liveness", &points, &liveness_matrix);
 
         if tcx.sess.emit_lifetime_markers() {
-            reconstruct_storage(body, &points, &liveness_matrix, &storage_to_rebuild);
+            reconstruct_storage(body, &points, &liveness_matrix);
         }
 
         apply_alias_fixup(tcx, body);
@@ -377,26 +377,14 @@ fn apply_mappings<'tcx>(
     tcx: TyCtxt<'tcx>,
     body: &mut Body<'tcx>,
     remapped_locals: &IndexVec<Local, Option<Place<'tcx>>>,
-) -> DenseBitSet<Local> {
-    let mut unified_locals = DenseBitSet::new_empty(body.local_decls.len());
-    for (src, dst) in remapped_locals.iter_enumerated() {
-        if let Some(dst_place) = dst {
-            unified_locals.insert(src);
-            unified_locals.insert(dst_place.local);
-        }
-    }
-
-    let storage_to_rebuild = DenseBitSet::new_empty(body.local_decls.len());
-    let mut rewriter = PlaceUpdater { tcx, remapped_locals, unified_locals, storage_to_rebuild };
+) {
+    let mut rewriter = PlaceUpdater { tcx, remapped_locals };
     rewriter.visit_body_preserves_cfg(body);
-    rewriter.storage_to_rebuild
 }
 
 struct PlaceUpdater<'a, 'tcx> {
     tcx: TyCtxt<'tcx>,
     remapped_locals: &'a IndexVec<Local, Option<Place<'tcx>>>,
-    unified_locals: DenseBitSet<Local>,
-    storage_to_rebuild: DenseBitSet<Local>,
 }
 
 impl<'tcx> MutVisitor<'tcx> for PlaceUpdater<'_, 'tcx> {
@@ -425,17 +413,11 @@ impl<'tcx> MutVisitor<'tcx> for PlaceUpdater<'_, 'tcx> {
 
     fn visit_statement(&mut self, statement: &mut Statement<'tcx>, location: Location) {
         match statement.kind {
-            StatementKind::StorageDead(local) | StatementKind::StorageLive(local)
-                if self.unified_locals.contains(local) =>
-            {
-                // Only rebuild storage for locals that previously had storage
-                // statements.
-                let mapped_local = match self.remapped_locals[local] {
-                    Some(place) => place.local,
-                    None => local,
-                };
-                self.storage_to_rebuild.insert(mapped_local);
-
+            // Remove *all* storage statements. These are rebuilt from liveness
+            // information later. Also, since we've preserved StorageDead in
+            // unwind paths until now, we will want to remove those since they
+            // hurt LLVM's codegen.
+            StatementKind::StorageDead(_) | StatementKind::StorageLive(_) => {
                 statement.make_nop(true);
                 return;
             }
@@ -476,21 +458,21 @@ fn get_or_split_edge<'tcx>(
     split_bb
 }
 
+/// Don't insert storage statements in cleanup blocks and in unreachable blocks.
+fn should_insert_storage<'tcx>(block_data: &BasicBlockData<'tcx>) -> bool {
+    !block_data.is_cleanup && !matches!(block_data.terminator().kind, TerminatorKind::Unreachable)
+}
+
 /// Re-constructs storage statements for all locals.
 fn reconstruct_storage<'tcx>(
     body: &mut Body<'tcx>,
     points: &DenseLocationMap,
     liveness_matrix: &SparseIntervalMatrix<Local, SplitPointIndex>,
-    storage_to_rebuild: &DenseBitSet<Local>,
 ) {
     let mut patcher = MirPatch::new(body);
     let mut split_edges: FxHashMap<(BasicBlock, BasicBlock), BasicBlock> = Default::default();
 
     for local in body.local_decls.indices() {
-        if !storage_to_rebuild.contains(local) {
-            continue;
-        }
-
         // Arguments and return values don't use storage statements.
         match body.local_kind(local) {
             LocalKind::Arg | LocalKind::ReturnPointer => continue,
@@ -543,9 +525,8 @@ fn reconstruct_storage<'tcx>(
              local: Local,
              block: BasicBlock| {
                 for succ in body.basic_blocks[block].terminator().successors() {
-                    // Don't emit StorageDead in cleanup blocks.
-                    if body.basic_blocks[succ].is_cleanup {
-                        continue;
+                    if !should_insert_storage(&body.basic_blocks[succ]) {
+                        return;
                     }
 
                     if !row.contains(SplitPointIndex::new(
@@ -576,19 +557,24 @@ fn reconstruct_storage<'tcx>(
             // If the live range starts at the `Early` point then it means that
             // the value came from a predecessor block. A write from the first
             // statement would happen at the `Late` point instead.
-            if range.start
-                == SplitPointIndex::new(points.entry_point(start_block), SplitPointEffect::Early)
-            {
-                // If the local is dead at the end of any predecessor block then
-                // emit a `StorageLive` before the terminator.
-                emit_storage_live_in_preds(body, &mut patcher, local, start_block);
-            } else {
-                // Otherwise just add `StorageLive` before the statement that
-                // starts the live range.
-                patcher.add_statement(
-                    points.to_location(range.start.point()),
-                    StatementKind::StorageLive(local),
-                );
+            if should_insert_storage(&body.basic_blocks[start_block]) {
+                if range.start
+                    == SplitPointIndex::new(
+                        points.entry_point(start_block),
+                        SplitPointEffect::Early,
+                    )
+                {
+                    // If the local is dead at the end of any predecessor block then
+                    // emit a `StorageLive` before the terminator.
+                    emit_storage_live_in_preds(body, &mut patcher, local, start_block);
+                } else {
+                    // Otherwise just add `StorageLive` before the statement that
+                    // starts the live range.
+                    patcher.add_statement(
+                        points.to_location(range.start.point()),
+                        StatementKind::StorageLive(local),
+                    );
+                }
             }
 
             // The live range may span multiple blocks because
@@ -598,24 +584,30 @@ fn reconstruct_storage<'tcx>(
             let mut current_block = start_block;
             debug_assert!(start_block <= end_block);
             while current_block != end_block {
-                emit_storage_dead_in_succs(body, &mut patcher, local, current_block);
+                if should_insert_storage(&body.basic_blocks[current_block]) {
+                    emit_storage_dead_in_succs(body, &mut patcher, local, current_block);
+                }
                 current_block = BasicBlock::from_usize(current_block.index() + 1);
-                emit_storage_live_in_preds(body, &mut patcher, local, current_block);
+                if should_insert_storage(&body.basic_blocks[current_block]) {
+                    emit_storage_live_in_preds(body, &mut patcher, local, current_block);
+                }
             }
 
             // We need to insert `StorageDead` after the last statement that
             // uses a local. If this is a terminator then we need to instead
             // insert it at the start of every successor block where the local
             // is dead on entry.
-            if range.last.point() == points.terminator(end_block) {
-                emit_storage_dead_in_succs(body, &mut patcher, local, current_block);
-            } else {
-                // Don't emit StorageDead in cleanup blocks.
-                if !body.basic_blocks[end_block].is_cleanup {
-                    patcher.add_statement(
-                        points.to_location(range.last.point()).successor_within_block(),
-                        StatementKind::StorageDead(local),
-                    );
+            if should_insert_storage(&body.basic_blocks[end_block]) {
+                if range.last.point() == points.terminator(end_block) {
+                    emit_storage_dead_in_succs(body, &mut patcher, local, current_block);
+                } else {
+                    // Don't emit StorageDead in cleanup blocks.
+                    if !body.basic_blocks[end_block].is_cleanup {
+                        patcher.add_statement(
+                            points.to_location(range.last.point()).successor_within_block(),
+                            StatementKind::StorageDead(local),
+                        );
+                    }
                 }
             }
         }