From dac354fc32bde196f1ee04b79cacfd6ea78683f0 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 21 Jan 2021 22:44:02 -0500 Subject: [PATCH] Revert "Simplify unscheduling of drops after moves" This reverts commit b766abc88f78f36193ddefb1079dbc832346b358. --- compiler/rustc_mir_build/src/build/scope.rs | 38 ++++++++++++------- .../box_expr.main.ElaborateDrops.before.mir | 10 +++-- .../issue_41110.main.ElaborateDrops.after.mir | 27 +++++++++++-- .../issue_41110.test.ElaborateDrops.after.mir | 26 +++++++------ ..._after_call.main.ElaborateDrops.before.mir | 10 ++++- 5 files changed, 80 insertions(+), 31 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/scope.rs b/compiler/rustc_mir_build/src/build/scope.rs index 538ad36a6fd..03717183707 100644 --- a/compiler/rustc_mir_build/src/build/scope.rs +++ b/compiler/rustc_mir_build/src/build/scope.rs @@ -83,7 +83,7 @@ use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder, CFG}; use crate::thir::{Expr, ExprRef, LintLevel}; -use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_data_structures::fx::FxHashMap; use rustc_index::vec::IndexVec; use rustc_middle::middle::region; use rustc_middle::mir::*; @@ -120,6 +120,8 @@ struct Scope { /// end of the vector (top of the stack) first. drops: Vec, + moved_locals: Vec, + /// The drop index that will drop everything in and below this scope on an /// unwind path. cached_unwind_block: Option, @@ -403,6 +405,7 @@ fn push_scope(&mut self, region_scope: (region::Scope, SourceInfo), vis_scope: S region_scope: region_scope.0, region_scope_span: region_scope.1.span, drops: vec![], + moved_locals: vec![], cached_unwind_block: None, cached_generator_drop_block: None, }); @@ -890,19 +893,20 @@ fn leave_top_scope(&mut self, block: BasicBlock) -> BasicBlock { assert_eq!(scope.region_scope, local_scope, "local scope is not the topmost scope!",); // look for moves of a local variable, like `MOVE(_X)` - let locals_moved = operands - .iter() - .filter_map(|operand| match operand { - Operand::Copy(_) | Operand::Constant(_) => None, - Operand::Move(place) => place.as_local(), - }) - .collect::>(); + let locals_moved = operands.iter().flat_map(|operand| match operand { + Operand::Copy(_) | Operand::Constant(_) => None, + Operand::Move(place) => place.as_local(), + }); - // Remove the drops for the moved operands. - scope - .drops - .retain(|drop| drop.kind == DropKind::Storage || !locals_moved.contains(&drop.local)); - scope.invalidate_cache(); + for local in locals_moved { + // check if we have a Drop for this operand and -- if so + // -- add it to the list of moved operands. Note that this + // local might not have been an operand created for this + // call, it could come from other places too. + if scope.drops.iter().any(|drop| drop.local == local && drop.kind == DropKind::Value) { + scope.moved_locals.push(local); + } + } } // Other @@ -1147,6 +1151,14 @@ fn build_scope_drops<'tcx>( debug_assert_eq!(unwind_drops.drops[unwind_to].0.kind, drop_data.kind); unwind_to = unwind_drops.drops[unwind_to].1; + // If the operand has been moved, and we are not on an unwind + // path, then don't generate the drop. (We only take this into + // account for non-unwind paths so as not to disturb the + // caching mechanism.) + if scope.moved_locals.iter().any(|&o| o == local) { + continue; + } + unwind_drops.add_entry(block, unwind_to); let next = cfg.start_new_block(); diff --git a/src/test/mir-opt/box_expr.main.ElaborateDrops.before.mir b/src/test/mir-opt/box_expr.main.ElaborateDrops.before.mir index ce5cb395952..cfbd3a58637 100644 --- a/src/test/mir-opt/box_expr.main.ElaborateDrops.before.mir +++ b/src/test/mir-opt/box_expr.main.ElaborateDrops.before.mir @@ -14,7 +14,7 @@ fn main() -> () { StorageLive(_1); // scope 0 at $DIR/box_expr.rs:7:9: 7:10 StorageLive(_2); // scope 0 at $DIR/box_expr.rs:7:13: 7:25 _2 = Box(S); // scope 0 at $DIR/box_expr.rs:7:13: 7:25 - (*_2) = S::new() -> [return: bb1, unwind: bb6]; // scope 0 at $DIR/box_expr.rs:7:17: 7:25 + (*_2) = S::new() -> [return: bb1, unwind: bb7]; // scope 0 at $DIR/box_expr.rs:7:17: 7:25 // mir::Constant // + span: $DIR/box_expr.rs:7:17: 7:23 // + literal: Const { ty: fn() -> S {S::new}, val: Value(Scalar()) } @@ -49,14 +49,18 @@ fn main() -> () { } bb5 (cleanup): { - drop(_1) -> bb7; // scope 0 at $DIR/box_expr.rs:9:1: 9:2 + drop(_4) -> bb6; // scope 1 at $DIR/box_expr.rs:8:11: 8:12 } bb6 (cleanup): { - drop(_2) -> bb7; // scope 0 at $DIR/box_expr.rs:7:24: 7:25 + drop(_1) -> bb8; // scope 0 at $DIR/box_expr.rs:9:1: 9:2 } bb7 (cleanup): { + drop(_2) -> bb8; // scope 0 at $DIR/box_expr.rs:7:24: 7:25 + } + + bb8 (cleanup): { resume; // scope 0 at $DIR/box_expr.rs:6:1: 9:2 } } diff --git a/src/test/mir-opt/issue_41110.main.ElaborateDrops.after.mir b/src/test/mir-opt/issue_41110.main.ElaborateDrops.after.mir index bbbd2bcf128..7113c42b9c7 100644 --- a/src/test/mir-opt/issue_41110.main.ElaborateDrops.after.mir +++ b/src/test/mir-opt/issue_41110.main.ElaborateDrops.after.mir @@ -6,18 +6,21 @@ fn main() -> () { let mut _2: S; // in scope 0 at $DIR/issue-41110.rs:8:13: 8:14 let mut _3: S; // in scope 0 at $DIR/issue-41110.rs:8:21: 8:27 let mut _4: S; // in scope 0 at $DIR/issue-41110.rs:8:21: 8:22 + let mut _5: bool; // in scope 0 at $DIR/issue-41110.rs:8:27: 8:28 scope 1 { debug x => _1; // in scope 1 at $DIR/issue-41110.rs:8:9: 8:10 } bb0: { + _5 = const false; // scope 0 at $DIR/issue-41110.rs:8:9: 8:10 StorageLive(_1); // scope 0 at $DIR/issue-41110.rs:8:9: 8:10 StorageLive(_2); // scope 0 at $DIR/issue-41110.rs:8:13: 8:14 + _5 = const true; // scope 0 at $DIR/issue-41110.rs:8:13: 8:14 _2 = S; // scope 0 at $DIR/issue-41110.rs:8:13: 8:14 StorageLive(_3); // scope 0 at $DIR/issue-41110.rs:8:21: 8:27 StorageLive(_4); // scope 0 at $DIR/issue-41110.rs:8:21: 8:22 _4 = S; // scope 0 at $DIR/issue-41110.rs:8:21: 8:22 - _3 = S::id(move _4) -> [return: bb1, unwind: bb3]; // scope 0 at $DIR/issue-41110.rs:8:21: 8:27 + _3 = S::id(move _4) -> [return: bb1, unwind: bb4]; // scope 0 at $DIR/issue-41110.rs:8:21: 8:27 // mir::Constant // + span: $DIR/issue-41110.rs:8:23: 8:25 // + literal: Const { ty: fn(S) -> S {S::id}, val: Value(Scalar()) } @@ -25,7 +28,8 @@ fn main() -> () { bb1: { StorageDead(_4); // scope 0 at $DIR/issue-41110.rs:8:26: 8:27 - _1 = S::other(move _2, move _3) -> bb2; // scope 0 at $DIR/issue-41110.rs:8:13: 8:28 + _5 = const false; // scope 0 at $DIR/issue-41110.rs:8:13: 8:28 + _1 = S::other(move _2, move _3) -> [return: bb2, unwind: bb3]; // scope 0 at $DIR/issue-41110.rs:8:13: 8:28 // mir::Constant // + span: $DIR/issue-41110.rs:8:15: 8:20 // + literal: Const { ty: fn(S, S) {S::other}, val: Value(Scalar()) } @@ -33,6 +37,7 @@ fn main() -> () { bb2: { StorageDead(_3); // scope 0 at $DIR/issue-41110.rs:8:27: 8:28 + _5 = const false; // scope 0 at $DIR/issue-41110.rs:8:27: 8:28 StorageDead(_2); // scope 0 at $DIR/issue-41110.rs:8:27: 8:28 _0 = const (); // scope 0 at $DIR/issue-41110.rs:7:11: 9:2 StorageDead(_1); // scope 0 at $DIR/issue-41110.rs:9:1: 9:2 @@ -40,10 +45,26 @@ fn main() -> () { } bb3 (cleanup): { - drop(_2) -> bb4; // scope 0 at $DIR/issue-41110.rs:8:27: 8:28 + goto -> bb5; // scope 0 at $DIR/issue-41110.rs:8:27: 8:28 } bb4 (cleanup): { + goto -> bb5; // scope 0 at $DIR/issue-41110.rs:8:26: 8:27 + } + + bb5 (cleanup): { + goto -> bb8; // scope 0 at $DIR/issue-41110.rs:8:27: 8:28 + } + + bb6 (cleanup): { resume; // scope 0 at $DIR/issue-41110.rs:7:1: 9:2 } + + bb7 (cleanup): { + drop(_2) -> bb6; // scope 0 at $DIR/issue-41110.rs:8:27: 8:28 + } + + bb8 (cleanup): { + switchInt(_5) -> [false: bb6, otherwise: bb7]; // scope 0 at $DIR/issue-41110.rs:8:27: 8:28 + } } diff --git a/src/test/mir-opt/issue_41110.test.ElaborateDrops.after.mir b/src/test/mir-opt/issue_41110.test.ElaborateDrops.after.mir index be730402be9..c4e852ca321 100644 --- a/src/test/mir-opt/issue_41110.test.ElaborateDrops.after.mir +++ b/src/test/mir-opt/issue_41110.test.ElaborateDrops.after.mir @@ -37,7 +37,7 @@ fn test() -> () { StorageLive(_5); // scope 2 at $DIR/issue-41110.rs:18:9: 18:10 _6 = const false; // scope 2 at $DIR/issue-41110.rs:18:9: 18:10 _5 = move _1; // scope 2 at $DIR/issue-41110.rs:18:9: 18:10 - goto -> bb11; // scope 2 at $DIR/issue-41110.rs:18:5: 18:6 + goto -> bb12; // scope 2 at $DIR/issue-41110.rs:18:5: 18:6 } bb2: { @@ -47,7 +47,7 @@ fn test() -> () { bb3: { StorageDead(_5); // scope 2 at $DIR/issue-41110.rs:18:9: 18:10 _0 = const (); // scope 0 at $DIR/issue-41110.rs:14:15: 19:2 - drop(_2) -> [return: bb4, unwind: bb8]; // scope 1 at $DIR/issue-41110.rs:19:1: 19:2 + drop(_2) -> [return: bb4, unwind: bb9]; // scope 1 at $DIR/issue-41110.rs:19:1: 19:2 } bb4: { @@ -62,36 +62,40 @@ fn test() -> () { } bb6 (cleanup): { - goto -> bb7; // scope 2 at $DIR/issue-41110.rs:18:9: 18:10 + goto -> bb8; // scope 2 at $DIR/issue-41110.rs:18:9: 18:10 } bb7 (cleanup): { - goto -> bb8; // scope 1 at $DIR/issue-41110.rs:19:1: 19:2 + goto -> bb8; // scope 2 at $DIR/issue-41110.rs:17:11: 17:12 } bb8 (cleanup): { - goto -> bb13; // scope 0 at $DIR/issue-41110.rs:19:1: 19:2 + goto -> bb9; // scope 1 at $DIR/issue-41110.rs:19:1: 19:2 } bb9 (cleanup): { - resume; // scope 0 at $DIR/issue-41110.rs:14:1: 19:2 + goto -> bb14; // scope 0 at $DIR/issue-41110.rs:19:1: 19:2 } bb10 (cleanup): { + resume; // scope 0 at $DIR/issue-41110.rs:14:1: 19:2 + } + + bb11 (cleanup): { _2 = move _5; // scope 2 at $DIR/issue-41110.rs:18:5: 18:6 goto -> bb6; // scope 2 at $DIR/issue-41110.rs:18:5: 18:6 } - bb11: { + bb12: { _2 = move _5; // scope 2 at $DIR/issue-41110.rs:18:5: 18:6 goto -> bb2; // scope 2 at $DIR/issue-41110.rs:18:5: 18:6 } - bb12 (cleanup): { - drop(_1) -> bb9; // scope 0 at $DIR/issue-41110.rs:19:1: 19:2 + bb13 (cleanup): { + drop(_1) -> bb10; // scope 0 at $DIR/issue-41110.rs:19:1: 19:2 } - bb13 (cleanup): { - switchInt(_6) -> [false: bb9, otherwise: bb12]; // scope 0 at $DIR/issue-41110.rs:19:1: 19:2 + bb14 (cleanup): { + switchInt(_6) -> [false: bb10, otherwise: bb13]; // scope 0 at $DIR/issue-41110.rs:19:1: 19:2 } } diff --git a/src/test/mir-opt/no_spurious_drop_after_call.main.ElaborateDrops.before.mir b/src/test/mir-opt/no_spurious_drop_after_call.main.ElaborateDrops.before.mir index 2f95931d2b2..bbb433dbe25 100644 --- a/src/test/mir-opt/no_spurious_drop_after_call.main.ElaborateDrops.before.mir +++ b/src/test/mir-opt/no_spurious_drop_after_call.main.ElaborateDrops.before.mir @@ -28,7 +28,7 @@ fn main() -> () { bb1: { StorageDead(_3); // scope 0 at $DIR/no-spurious-drop-after-call.rs:9:33: 9:34 - _1 = std::mem::drop::(move _2) -> bb2; // scope 0 at $DIR/no-spurious-drop-after-call.rs:9:5: 9:35 + _1 = std::mem::drop::(move _2) -> [return: bb2, unwind: bb3]; // scope 0 at $DIR/no-spurious-drop-after-call.rs:9:5: 9:35 // mir::Constant // + span: $DIR/no-spurious-drop-after-call.rs:9:5: 9:19 // + literal: Const { ty: fn(std::string::String) {std::mem::drop::}, val: Value(Scalar()) } @@ -41,4 +41,12 @@ fn main() -> () { _0 = const (); // scope 0 at $DIR/no-spurious-drop-after-call.rs:8:11: 10:2 return; // scope 0 at $DIR/no-spurious-drop-after-call.rs:10:2: 10:2 } + + bb3 (cleanup): { + drop(_2) -> bb4; // scope 0 at $DIR/no-spurious-drop-after-call.rs:9:34: 9:35 + } + + bb4 (cleanup): { + resume; // scope 0 at $DIR/no-spurious-drop-after-call.rs:8:1: 10:2 + } } -- 2.44.0