From ecad4341af86665a2fb94dac732362d47608c73f Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 18 Feb 2020 10:31:01 -0800 Subject: [PATCH] Port `RequiresStorage` to new dataflow framework --- src/librustc_mir/dataflow/impls/mod.rs | 2 +- .../dataflow/impls/storage_liveness.rs | 119 ++++++++++-------- src/librustc_mir/transform/generator.rs | 76 ++++------- 3 files changed, 97 insertions(+), 100 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/mod.rs b/src/librustc_mir/dataflow/impls/mod.rs index 59aa91ab824..87d8e9e411c 100644 --- a/src/librustc_mir/dataflow/impls/mod.rs +++ b/src/librustc_mir/dataflow/impls/mod.rs @@ -14,7 +14,7 @@ use crate::util::elaborate_drops::DropFlagState; use super::generic::{AnalysisDomain, GenKill, GenKillAnalysis}; use super::move_paths::{HasMoveData, InitIndex, InitKind, LookupResult, MoveData, MovePathIndex}; -use super::{BottomValue, GenKillSet}; +use super::BottomValue; use super::drop_flag_effects_for_function_entry; use super::drop_flag_effects_for_location; diff --git a/src/librustc_mir/dataflow/impls/storage_liveness.rs b/src/librustc_mir/dataflow/impls/storage_liveness.rs index fdc34f2204b..828321f7031 100644 --- a/src/librustc_mir/dataflow/impls/storage_liveness.rs +++ b/src/librustc_mir/dataflow/impls/storage_liveness.rs @@ -76,7 +76,7 @@ pub struct RequiresStorage<'mir, 'tcx> { borrowed_locals: RefCell>, } -impl<'mir, 'tcx: 'mir> RequiresStorage<'mir, 'tcx> { +impl<'mir, 'tcx> RequiresStorage<'mir, 'tcx> { pub fn new( body: ReadOnlyBodyAndCache<'mir, 'tcx>, borrowed_locals: &'mir Results<'tcx, MaybeBorrowedLocals>, @@ -86,45 +86,47 @@ impl<'mir, 'tcx: 'mir> RequiresStorage<'mir, 'tcx> { borrowed_locals: RefCell::new(ResultsRefCursor::new(*body, borrowed_locals)), } } - - pub fn body(&self) -> &Body<'tcx> { - &self.body - } } -impl<'mir, 'tcx> BitDenotation<'tcx> for RequiresStorage<'mir, 'tcx> { +impl<'mir, 'tcx> dataflow::AnalysisDomain<'tcx> for RequiresStorage<'mir, 'tcx> { type Idx = Local; - fn name() -> &'static str { - "requires_storage" - } - fn bits_per_block(&self) -> usize { - self.body.local_decls.len() + + const NAME: &'static str = "requires_storage"; + + fn bits_per_block(&self, body: &mir::Body<'tcx>) -> usize { + body.local_decls.len() } - fn start_block_effect(&self, on_entry: &mut BitSet) { + fn initialize_start_block(&self, body: &mir::Body<'tcx>, on_entry: &mut BitSet) { // The resume argument is live on function entry (we don't care about // the `self` argument) - for arg in self.body.args_iter().skip(1) { + for arg in body.args_iter().skip(1) { on_entry.insert(arg); } } +} - fn before_statement_effect(&self, sets: &mut GenKillSet, loc: Location) { - let stmt = &self.body[loc.block].statements[loc.statement_index]; - +impl<'mir, 'tcx> dataflow::GenKillAnalysis<'tcx> for RequiresStorage<'mir, 'tcx> { + fn before_statement_effect( + &self, + trans: &mut impl GenKill, + stmt: &mir::Statement<'tcx>, + loc: Location, + ) { // If a place is borrowed in a statement, it needs storage for that statement. - self.borrowed_locals.borrow().analysis().statement_effect(sets, stmt, loc); + self.borrowed_locals.borrow().analysis().statement_effect(trans, stmt, loc); - // If a place is assigned to in a statement, it needs storage for that statement. match &stmt.kind { - StatementKind::StorageDead(l) => sets.kill(*l), + StatementKind::StorageDead(l) => trans.kill(*l), + + // If a place is assigned to in a statement, it needs storage for that statement. StatementKind::Assign(box (place, _)) | StatementKind::SetDiscriminant { box place, .. } => { - sets.gen(place.local); + trans.gen(place.local); } - StatementKind::InlineAsm(box InlineAsm { outputs, .. }) => { - for place in &**outputs { - sets.gen(place.local); + StatementKind::InlineAsm(asm) => { + for place in &*asm.outputs { + trans.gen(place.local); } } @@ -138,22 +140,30 @@ impl<'mir, 'tcx> BitDenotation<'tcx> for RequiresStorage<'mir, 'tcx> { } } - fn statement_effect(&self, sets: &mut GenKillSet, loc: Location) { + fn statement_effect( + &self, + trans: &mut impl GenKill, + _: &mir::Statement<'tcx>, + loc: Location, + ) { // If we move from a place then only stops needing storage *after* // that statement. - self.check_for_move(sets, loc); + self.check_for_move(trans, loc); } - fn before_terminator_effect(&self, sets: &mut GenKillSet, loc: Location) { - let terminator = self.body[loc.block].terminator(); - + fn before_terminator_effect( + &self, + trans: &mut impl GenKill, + terminator: &mir::Terminator<'tcx>, + loc: Location, + ) { // If a place is borrowed in a terminator, it needs storage for that terminator. - self.borrowed_locals.borrow().analysis().terminator_effect(sets, terminator, loc); + self.borrowed_locals.borrow().analysis().terminator_effect(trans, terminator, loc); match &terminator.kind { - TerminatorKind::Call { destination: Some((Place { local, .. }, _)), .. } - | TerminatorKind::Yield { resume_arg: Place { local, .. }, .. } => { - sets.gen(*local); + TerminatorKind::Call { destination: Some((place, _)), .. } + | TerminatorKind::Yield { resume_arg: place, .. } => { + trans.gen(place.local); } // Nothing to do for these. Match exhaustively so this fails to compile when new @@ -174,14 +184,19 @@ impl<'mir, 'tcx> BitDenotation<'tcx> for RequiresStorage<'mir, 'tcx> { } } - fn terminator_effect(&self, sets: &mut GenKillSet, loc: Location) { - match &self.body[loc.block].terminator().kind { + fn terminator_effect( + &self, + trans: &mut impl GenKill, + terminator: &mir::Terminator<'tcx>, + loc: Location, + ) { + match &terminator.kind { // For call terminators the destination requires storage for the call // and after the call returns successfully, but not after a panic. // Since `propagate_call_unwind` doesn't exist, we have to kill the - // destination here, and then gen it again in `propagate_call_return`. - TerminatorKind::Call { destination: Some((Place { local, .. }, _)), .. } => { - sets.kill(*local); + // destination here, and then gen it again in `call_return_effect`. + TerminatorKind::Call { destination: Some((place, _)), .. } => { + trans.kill(place.local); } // Nothing to do for these. Match exhaustively so this fails to compile when new @@ -202,24 +217,25 @@ impl<'mir, 'tcx> BitDenotation<'tcx> for RequiresStorage<'mir, 'tcx> { | TerminatorKind::Unreachable => {} } - self.check_for_move(sets, loc); + self.check_for_move(trans, loc); } - fn propagate_call_return( + fn call_return_effect( &self, - in_out: &mut BitSet, - _call_bb: mir::BasicBlock, - _dest_bb: mir::BasicBlock, - dest_place: &mir::Place<'tcx>, + trans: &mut impl GenKill, + _block: BasicBlock, + _func: &mir::Operand<'tcx>, + _args: &[mir::Operand<'tcx>], + return_place: &mir::Place<'tcx>, ) { - in_out.insert(dest_place.local); + trans.gen(return_place.local); } } impl<'mir, 'tcx> RequiresStorage<'mir, 'tcx> { /// Kill locals that are fully moved and have not been borrowed. - fn check_for_move(&self, sets: &mut GenKillSet, loc: Location) { - let mut visitor = MoveVisitor { sets, borrowed_locals: &self.borrowed_locals }; + fn check_for_move(&self, trans: &mut impl GenKill, loc: Location) { + let mut visitor = MoveVisitor { trans, borrowed_locals: &self.borrowed_locals }; visitor.visit_location(self.body, loc); } } @@ -229,18 +245,21 @@ impl<'mir, 'tcx> BottomValue for RequiresStorage<'mir, 'tcx> { const BOTTOM_VALUE: bool = false; } -struct MoveVisitor<'a, 'mir, 'tcx> { +struct MoveVisitor<'a, 'mir, 'tcx, T> { borrowed_locals: &'a RefCell>, - sets: &'a mut GenKillSet, + trans: &'a mut T, } -impl<'a, 'mir: 'a, 'tcx> Visitor<'tcx> for MoveVisitor<'a, 'mir, 'tcx> { +impl<'a, 'mir, 'tcx, T> Visitor<'tcx> for MoveVisitor<'a, 'mir, 'tcx, T> +where + T: GenKill, +{ fn visit_local(&mut self, local: &Local, context: PlaceContext, loc: Location) { if PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) == context { let mut borrowed_locals = self.borrowed_locals.borrow_mut(); borrowed_locals.seek_before(loc); if !borrowed_locals.contains(*local) { - self.sets.kill(*local); + self.trans.kill(*local); } } } diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index de9710452ee..b9d2a167d73 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -49,9 +49,7 @@ //! For generators with state 1 (returned) and state 2 (poisoned) it does nothing. //! Otherwise it drops all the values in scope at the last suspension point. -use crate::dataflow::generic::{Analysis, ResultsCursor}; -use crate::dataflow::{do_dataflow, DataflowResultsCursor, DebugFormatted}; -use crate::dataflow::{DataflowResults, DataflowResultsConsumer, FlowAtLocation}; +use crate::dataflow::generic::{self as dataflow, Analysis}; use crate::dataflow::{MaybeBorrowedLocals, MaybeStorageLive, RequiresStorage}; use crate::transform::no_landing_pads::no_landing_pads; use crate::transform::simplify; @@ -467,7 +465,6 @@ fn locals_live_across_suspend_points( source: MirSource<'tcx>, movable: bool, ) -> LivenessInfo { - let dead_unwinds = BitSet::new_empty(body.basic_blocks().len()); let def_id = source.def_id(); let body_ref: &Body<'_> = &body; @@ -488,22 +485,16 @@ fn locals_live_across_suspend_points( let borrowed_locals_results = MaybeBorrowedLocals::all_borrows().into_engine(tcx, body_ref, def_id).iterate_to_fixpoint(); - let mut borrowed_locals_cursor = ResultsCursor::new(body_ref, &borrowed_locals_results); + let mut borrowed_locals_cursor = + dataflow::ResultsCursor::new(body_ref, &borrowed_locals_results); // Calculate the MIR locals that we actually need to keep storage around // for. - let requires_storage_analysis = RequiresStorage::new(body, &borrowed_locals_results); - let requires_storage_results = do_dataflow( - tcx, - body_ref, - def_id, - &[], - &dead_unwinds, - requires_storage_analysis, - |bd, p| DebugFormatted::new(&bd.body().local_decls[p]), - ); + let requires_storage_results = RequiresStorage::new(body, &borrowed_locals_results) + .into_engine(tcx, body_ref, def_id) + .iterate_to_fixpoint(); let mut requires_storage_cursor = - DataflowResultsCursor::new(&requires_storage_results, body_ref); + dataflow::ResultsCursor::new(body_ref, &requires_storage_results); // Calculate the liveness of MIR locals ignoring borrows. let mut live_locals = liveness::LiveVarSet::new_empty(body.local_decls.len()); @@ -539,7 +530,7 @@ fn locals_live_across_suspend_points( // after a suspension point storage_liveness_map.insert(block, storage_liveness.clone()); - requires_storage_cursor.seek(loc); + requires_storage_cursor.seek_before(loc); let storage_required = requires_storage_cursor.get().clone(); // Locals live are live at this point only if they are used across @@ -609,7 +600,7 @@ fn compute_storage_conflicts( body: &'mir Body<'tcx>, stored_locals: &liveness::LiveVarSet, ignored: &StorageIgnored, - requires_storage: DataflowResults<'tcx, RequiresStorage<'mir, 'tcx>>, + requires_storage: dataflow::Results<'tcx, RequiresStorage<'mir, 'tcx>>, ) -> BitMatrix { assert_eq!(body.local_decls.len(), ignored.0.domain_size()); assert_eq!(body.local_decls.len(), stored_locals.domain_size()); @@ -627,8 +618,10 @@ fn compute_storage_conflicts( stored_locals: &stored_locals, local_conflicts: BitMatrix::from_row_n(&ineligible_locals, body.local_decls.len()), }; - let mut state = FlowAtLocation::new(requires_storage); - visitor.analyze_results(&mut state); + + // FIXME: Do we need to do this in RPO? + requires_storage.visit_in_rpo_with(body, &mut visitor); + let local_conflicts = visitor.local_conflicts; // Compress the matrix using only stored locals (Local -> GeneratorSavedLocal). @@ -657,60 +650,45 @@ fn compute_storage_conflicts( storage_conflicts } -struct StorageConflictVisitor<'body, 'tcx, 's> { - body: &'body Body<'tcx>, +struct StorageConflictVisitor<'mir, 'tcx, 's> { + body: &'mir Body<'tcx>, stored_locals: &'s liveness::LiveVarSet, // FIXME(tmandry): Consider using sparse bitsets here once we have good // benchmarks for generators. local_conflicts: BitMatrix, } -impl<'body, 'tcx, 's> DataflowResultsConsumer<'body, 'tcx> - for StorageConflictVisitor<'body, 'tcx, 's> -{ - type FlowState = FlowAtLocation<'tcx, RequiresStorage<'body, 'tcx>>; +impl dataflow::ResultsVisitor<'mir, 'tcx> for StorageConflictVisitor<'mir, 'tcx, '_> { + type FlowState = BitSet; - fn body(&self) -> &'body Body<'tcx> { - self.body - } - - fn visit_block_entry(&mut self, block: BasicBlock, flow_state: &Self::FlowState) { - // statement_index is only used for logging, so this is fine. - self.apply_state(flow_state, Location { block, statement_index: 0 }); - } - - fn visit_statement_entry( + fn visit_statement( &mut self, + state: &Self::FlowState, + _statement: &'mir Statement<'tcx>, loc: Location, - _stmt: &Statement<'tcx>, - flow_state: &Self::FlowState, ) { - self.apply_state(flow_state, loc); + self.apply_state(state, loc); } - fn visit_terminator_entry( + fn visit_terminator( &mut self, + state: &Self::FlowState, + _terminator: &'mir Terminator<'tcx>, loc: Location, - _term: &Terminator<'tcx>, - flow_state: &Self::FlowState, ) { - self.apply_state(flow_state, loc); + self.apply_state(state, loc); } } impl<'body, 'tcx, 's> StorageConflictVisitor<'body, 'tcx, 's> { - fn apply_state( - &mut self, - flow_state: &FlowAtLocation<'tcx, RequiresStorage<'body, 'tcx>>, - loc: Location, - ) { + fn apply_state(&mut self, flow_state: &BitSet, loc: Location) { // Ignore unreachable blocks. match self.body.basic_blocks()[loc.block].terminator().kind { TerminatorKind::Unreachable => return, _ => (), }; - let mut eligible_storage_live = flow_state.as_dense().clone(); + let mut eligible_storage_live = flow_state.clone(); eligible_storage_live.intersect(&self.stored_locals); for local in eligible_storage_live.iter() {