From 0066acf753ced0730cb4a2337ed083dd7e4d9a0d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 18 Nov 2024 15:37:55 +1100 Subject: [PATCH 1/5] Merge `apply_effects_in_block` and `join_state_into_successors_of`. They are always called in succession, so it's simpler if they are merged into a single function. --- .../src/framework/direction.rs | 285 ++++++++---------- .../rustc_mir_dataflow/src/framework/mod.rs | 9 +- 2 files changed, 132 insertions(+), 162 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/framework/direction.rs b/compiler/rustc_mir_dataflow/src/framework/direction.rs index 9a5cf9d4e84ff..84b22ed68c6dc 100644 --- a/compiler/rustc_mir_dataflow/src/framework/direction.rs +++ b/compiler/rustc_mir_dataflow/src/framework/direction.rs @@ -12,6 +12,16 @@ pub trait Direction { const IS_BACKWARD: bool = !Self::IS_FORWARD; + fn apply_effects_in_block<'mir, 'tcx, A>( + analysis: &mut A, + body: &mir::Body<'tcx>, + state: &mut A::Domain, + block: BasicBlock, + block_data: &'mir mir::BasicBlockData<'tcx>, + propagate: impl FnMut(BasicBlock, &A::Domain), + ) where + A: Analysis<'tcx>; + /// Applies all effects between the given `EffectIndex`s. /// /// `effects.start()` must precede or equal `effects.end()` in this direction. @@ -24,15 +34,6 @@ pub trait Direction { ) where A: Analysis<'tcx>; - fn apply_effects_in_block<'mir, 'tcx, A>( - analysis: &mut A, - state: &mut A::Domain, - block: BasicBlock, - block_data: &'mir mir::BasicBlockData<'tcx>, - ) -> TerminatorEdges<'mir, 'tcx> - where - A: Analysis<'tcx>; - fn visit_results_in_block<'mir, 'tcx, A>( state: &mut A::Domain, block: BasicBlock, @@ -41,16 +42,6 @@ pub trait Direction { vis: &mut impl ResultsVisitor<'mir, 'tcx, A>, ) where A: Analysis<'tcx>; - - fn join_state_into_successors_of<'tcx, A>( - analysis: &mut A, - body: &mir::Body<'tcx>, - exit_state: &mut A::Domain, - block: BasicBlock, - edges: TerminatorEdges<'_, 'tcx>, - propagate: impl FnMut(BasicBlock, &A::Domain), - ) where - A: Analysis<'tcx>; } /// Dataflow that runs from the exit of a block (terminator), to its entry (the first statement). @@ -61,23 +52,84 @@ impl Direction for Backward { fn apply_effects_in_block<'mir, 'tcx, A>( analysis: &mut A, + body: &mir::Body<'tcx>, state: &mut A::Domain, block: BasicBlock, block_data: &'mir mir::BasicBlockData<'tcx>, - ) -> TerminatorEdges<'mir, 'tcx> - where + mut propagate: impl FnMut(BasicBlock, &A::Domain), + ) where A: Analysis<'tcx>, { let terminator = block_data.terminator(); let location = Location { block, statement_index: block_data.statements.len() }; analysis.apply_before_terminator_effect(state, terminator, location); - let edges = analysis.apply_terminator_effect(state, terminator, location); + analysis.apply_terminator_effect(state, terminator, location); for (statement_index, statement) in block_data.statements.iter().enumerate().rev() { let location = Location { block, statement_index }; analysis.apply_before_statement_effect(state, statement, location); analysis.apply_statement_effect(state, statement, location); } - edges + + let exit_state = state; + for pred in body.basic_blocks.predecessors()[block].iter().copied() { + match body[pred].terminator().kind { + // Apply terminator-specific edge effects. + // + // FIXME(ecstaticmorse): Avoid cloning the exit state unconditionally. + mir::TerminatorKind::Call { destination, target: Some(dest), .. } + if dest == block => + { + let mut tmp = exit_state.clone(); + analysis.apply_call_return_effect( + &mut tmp, + pred, + CallReturnPlaces::Call(destination), + ); + propagate(pred, &tmp); + } + + mir::TerminatorKind::InlineAsm { ref targets, ref operands, .. } + if targets.contains(&block) => + { + let mut tmp = exit_state.clone(); + analysis.apply_call_return_effect( + &mut tmp, + pred, + CallReturnPlaces::InlineAsm(operands), + ); + propagate(pred, &tmp); + } + + mir::TerminatorKind::Yield { resume, resume_arg, .. } if resume == block => { + let mut tmp = exit_state.clone(); + analysis.apply_call_return_effect( + &mut tmp, + resume, + CallReturnPlaces::Yield(resume_arg), + ); + propagate(pred, &tmp); + } + + mir::TerminatorKind::SwitchInt { targets: _, ref discr } => { + let mut applier = BackwardSwitchIntEdgeEffectsApplier { + body, + pred, + exit_state, + block, + propagate: &mut propagate, + effects_applied: false, + }; + + analysis.apply_switch_int_edge_effects(pred, discr, &mut applier); + + if !applier.effects_applied { + propagate(pred, exit_state) + } + } + + _ => propagate(pred, exit_state), + } + } } fn apply_effects_in_range<'tcx, A>( @@ -188,82 +240,13 @@ impl Direction for Backward { vis.visit_block_start(state); } - - fn join_state_into_successors_of<'tcx, A>( - analysis: &mut A, - body: &mir::Body<'tcx>, - exit_state: &mut A::Domain, - bb: BasicBlock, - _edges: TerminatorEdges<'_, 'tcx>, - mut propagate: impl FnMut(BasicBlock, &A::Domain), - ) where - A: Analysis<'tcx>, - { - for pred in body.basic_blocks.predecessors()[bb].iter().copied() { - match body[pred].terminator().kind { - // Apply terminator-specific edge effects. - // - // FIXME(ecstaticmorse): Avoid cloning the exit state unconditionally. - mir::TerminatorKind::Call { destination, target: Some(dest), .. } if dest == bb => { - let mut tmp = exit_state.clone(); - analysis.apply_call_return_effect( - &mut tmp, - pred, - CallReturnPlaces::Call(destination), - ); - propagate(pred, &tmp); - } - - mir::TerminatorKind::InlineAsm { ref targets, ref operands, .. } - if targets.contains(&bb) => - { - let mut tmp = exit_state.clone(); - analysis.apply_call_return_effect( - &mut tmp, - pred, - CallReturnPlaces::InlineAsm(operands), - ); - propagate(pred, &tmp); - } - - mir::TerminatorKind::Yield { resume, resume_arg, .. } if resume == bb => { - let mut tmp = exit_state.clone(); - analysis.apply_call_return_effect( - &mut tmp, - resume, - CallReturnPlaces::Yield(resume_arg), - ); - propagate(pred, &tmp); - } - - mir::TerminatorKind::SwitchInt { targets: _, ref discr } => { - let mut applier = BackwardSwitchIntEdgeEffectsApplier { - body, - pred, - exit_state, - bb, - propagate: &mut propagate, - effects_applied: false, - }; - - analysis.apply_switch_int_edge_effects(pred, discr, &mut applier); - - if !applier.effects_applied { - propagate(pred, exit_state) - } - } - - _ => propagate(pred, exit_state), - } - } - } } struct BackwardSwitchIntEdgeEffectsApplier<'mir, 'tcx, D, F> { body: &'mir mir::Body<'tcx>, pred: BasicBlock, exit_state: &'mir mut D, - bb: BasicBlock, + block: BasicBlock, propagate: &'mir mut F, effects_applied: bool, } @@ -276,8 +259,8 @@ where fn apply(&mut self, mut apply_edge_effect: impl FnMut(&mut D, SwitchIntTarget)) { assert!(!self.effects_applied); - let values = &self.body.basic_blocks.switch_sources()[&(self.bb, self.pred)]; - let targets = values.iter().map(|&value| SwitchIntTarget { value, target: self.bb }); + let values = &self.body.basic_blocks.switch_sources()[&(self.block, self.pred)]; + let targets = values.iter().map(|&value| SwitchIntTarget { value, target: self.block }); let mut tmp = None; for target in targets { @@ -298,11 +281,12 @@ impl Direction for Forward { fn apply_effects_in_block<'mir, 'tcx, A>( analysis: &mut A, + _body: &mir::Body<'tcx>, state: &mut A::Domain, block: BasicBlock, block_data: &'mir mir::BasicBlockData<'tcx>, - ) -> TerminatorEdges<'mir, 'tcx> - where + mut propagate: impl FnMut(BasicBlock, &A::Domain), + ) where A: Analysis<'tcx>, { for (statement_index, statement) in block_data.statements.iter().enumerate() { @@ -313,7 +297,53 @@ impl Direction for Forward { let terminator = block_data.terminator(); let location = Location { block, statement_index: block_data.statements.len() }; analysis.apply_before_terminator_effect(state, terminator, location); - analysis.apply_terminator_effect(state, terminator, location) + let edges = analysis.apply_terminator_effect(state, terminator, location); + + let exit_state = state; + match edges { + TerminatorEdges::None => {} + TerminatorEdges::Single(target) => propagate(target, exit_state), + TerminatorEdges::Double(target, unwind) => { + propagate(target, exit_state); + propagate(unwind, exit_state); + } + TerminatorEdges::AssignOnReturn { return_, cleanup, place } => { + // This must be done *first*, otherwise the unwind path will see the assignments. + if let Some(cleanup) = cleanup { + propagate(cleanup, exit_state); + } + + if !return_.is_empty() { + analysis.apply_call_return_effect(exit_state, block, place); + for &target in return_ { + propagate(target, exit_state); + } + } + } + TerminatorEdges::SwitchInt { targets, discr } => { + let mut applier = ForwardSwitchIntEdgeEffectsApplier { + exit_state, + targets, + propagate, + effects_applied: false, + }; + + analysis.apply_switch_int_edge_effects(block, discr, &mut applier); + + let ForwardSwitchIntEdgeEffectsApplier { + exit_state, + mut propagate, + effects_applied, + .. + } = applier; + + if !effects_applied { + for target in targets.all_targets() { + propagate(*target, exit_state); + } + } + } + } } fn apply_effects_in_range<'tcx, A>( @@ -351,7 +381,8 @@ impl Direction for Forward { let statement = &block_data.statements[from.statement_index]; analysis.apply_statement_effect(state, statement, location); - // If we only needed to apply the after effect of the statement at `idx`, we are done. + // If we only needed to apply the after effect of the statement at `idx`, we are + // done. if from == to { return; } @@ -419,62 +450,6 @@ impl Direction for Forward { vis.visit_block_end(state); } - - fn join_state_into_successors_of<'tcx, A>( - analysis: &mut A, - _body: &mir::Body<'tcx>, - exit_state: &mut A::Domain, - bb: BasicBlock, - edges: TerminatorEdges<'_, 'tcx>, - mut propagate: impl FnMut(BasicBlock, &A::Domain), - ) where - A: Analysis<'tcx>, - { - match edges { - TerminatorEdges::None => {} - TerminatorEdges::Single(target) => propagate(target, exit_state), - TerminatorEdges::Double(target, unwind) => { - propagate(target, exit_state); - propagate(unwind, exit_state); - } - TerminatorEdges::AssignOnReturn { return_, cleanup, place } => { - // This must be done *first*, otherwise the unwind path will see the assignments. - if let Some(cleanup) = cleanup { - propagate(cleanup, exit_state); - } - - if !return_.is_empty() { - analysis.apply_call_return_effect(exit_state, bb, place); - for &target in return_ { - propagate(target, exit_state); - } - } - } - TerminatorEdges::SwitchInt { targets, discr } => { - let mut applier = ForwardSwitchIntEdgeEffectsApplier { - exit_state, - targets, - propagate, - effects_applied: false, - }; - - analysis.apply_switch_int_edge_effects(bb, discr, &mut applier); - - let ForwardSwitchIntEdgeEffectsApplier { - exit_state, - mut propagate, - effects_applied, - .. - } = applier; - - if !effects_applied { - for target in targets.all_targets() { - propagate(*target, exit_state); - } - } - } - } - } } struct ForwardSwitchIntEdgeEffectsApplier<'mir, D, F> { diff --git a/compiler/rustc_mir_dataflow/src/framework/mod.rs b/compiler/rustc_mir_dataflow/src/framework/mod.rs index 244dfe26ad362..7c3bcebcfe252 100644 --- a/compiler/rustc_mir_dataflow/src/framework/mod.rs +++ b/compiler/rustc_mir_dataflow/src/framework/mod.rs @@ -278,22 +278,17 @@ pub trait Analysis<'tcx> { // every iteration. let mut state = self.bottom_value(body); while let Some(bb) = dirty_queue.pop() { - let bb_data = &body[bb]; - // Set the state to the entry state of the block. // This is equivalent to `state = entry_sets[bb].clone()`, // but it saves an allocation, thus improving compile times. state.clone_from(&entry_sets[bb]); - // Apply the block transfer function, using the cached one if it exists. - let edges = Self::Direction::apply_effects_in_block(&mut self, &mut state, bb, bb_data); - - Self::Direction::join_state_into_successors_of( + Self::Direction::apply_effects_in_block( &mut self, body, &mut state, bb, - edges, + &body[bb], |target: BasicBlock, state: &Self::Domain| { let set_changed = entry_sets[target].join(state); if set_changed { From 7e704afc2d48df0f266ac592d0806c6770a3d08c Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 18 Nov 2024 15:43:13 +1100 Subject: [PATCH 2/5] Add some useful comments. Describing some things that took me a long time to understand. --- .../rustc_mir_dataflow/src/framework/cursor.rs | 14 +++++++++----- .../rustc_mir_dataflow/src/framework/direction.rs | 9 ++++++--- compiler/rustc_mir_dataflow/src/framework/mod.rs | 5 +++-- .../rustc_mir_dataflow/src/framework/results.rs | 4 +++- .../rustc_mir_dataflow/src/framework/visitor.rs | 4 +++- 5 files changed, 24 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/framework/cursor.rs b/compiler/rustc_mir_dataflow/src/framework/cursor.rs index 5ebb343f4e195..fbd9376917a14 100644 --- a/compiler/rustc_mir_dataflow/src/framework/cursor.rs +++ b/compiler/rustc_mir_dataflow/src/framework/cursor.rs @@ -8,12 +8,16 @@ use rustc_middle::mir::{self, BasicBlock, Location}; use super::{Analysis, Direction, Effect, EffectIndex, Results}; -/// Allows random access inspection of the results of a dataflow analysis. +/// Allows random access inspection of the results of a dataflow analysis. Use this when you want +/// to inspect domain values only in certain locations; use `ResultsVisitor` if you want to inspect +/// domain values in many or all locations. /// -/// This cursor only has linear performance within a basic block when its statements are visited in -/// the same order as the `DIRECTION` of the analysis. In the worst case—when statements are -/// visited in *reverse* order—performance will be quadratic in the number of statements in the -/// block. The order in which basic blocks are inspected has no impact on performance. +/// Because `Results` only has domain values for the entry of each basic block, these inspections +/// involve some amount of domain value recomputations. This cursor only has linear performance +/// within a basic block when its statements are visited in the same order as the `DIRECTION` of +/// the analysis. In the worst case—when statements are visited in *reverse* order—performance will +/// be quadratic in the number of statements in the block. The order in which basic blocks are +/// inspected has no impact on performance. pub struct ResultsCursor<'mir, 'tcx, A> where A: Analysis<'tcx>, diff --git a/compiler/rustc_mir_dataflow/src/framework/direction.rs b/compiler/rustc_mir_dataflow/src/framework/direction.rs index 84b22ed68c6dc..566a6b09b2b15 100644 --- a/compiler/rustc_mir_dataflow/src/framework/direction.rs +++ b/compiler/rustc_mir_dataflow/src/framework/direction.rs @@ -9,9 +9,9 @@ use super::{Analysis, Effect, EffectIndex, Results, SwitchIntTarget}; pub trait Direction { const IS_FORWARD: bool; - const IS_BACKWARD: bool = !Self::IS_FORWARD; + /// Called by `iterate_to_fixpoint` during initial analysis computation. fn apply_effects_in_block<'mir, 'tcx, A>( analysis: &mut A, body: &mir::Body<'tcx>, @@ -22,7 +22,8 @@ pub trait Direction { ) where A: Analysis<'tcx>; - /// Applies all effects between the given `EffectIndex`s. + /// Called by `ResultsCursor` to recompute the domain value for a location + /// in a basic block. Applies all effects between the given `EffectIndex`s. /// /// `effects.start()` must precede or equal `effects.end()` in this direction. fn apply_effects_in_range<'tcx, A>( @@ -34,6 +35,9 @@ pub trait Direction { ) where A: Analysis<'tcx>; + /// Called by `ResultsVisitor` to recompute the analysis domain values for + /// all locations in a basic block (starting from the entry value stored + /// in `Results`) and to visit them with `vis`. fn visit_results_in_block<'mir, 'tcx, A>( state: &mut A::Domain, block: BasicBlock, @@ -222,7 +226,6 @@ impl Direction for Backward { vis.visit_block_end(state); - // Terminator let loc = Location { block, statement_index: block_data.statements.len() }; let term = block_data.terminator(); results.analysis.apply_before_terminator_effect(state, term, loc); diff --git a/compiler/rustc_mir_dataflow/src/framework/mod.rs b/compiler/rustc_mir_dataflow/src/framework/mod.rs index 7c3bcebcfe252..f9e7ba743fc2f 100644 --- a/compiler/rustc_mir_dataflow/src/framework/mod.rs +++ b/compiler/rustc_mir_dataflow/src/framework/mod.rs @@ -8,8 +8,9 @@ //! The `impls` module contains several examples of dataflow analyses. //! //! Then call `iterate_to_fixpoint` on your type that impls `Analysis` to get a `Results`. From -//! there, you can use a `ResultsCursor` to inspect the fixpoint solution to your dataflow problem, -//! or implement the `ResultsVisitor` interface and use `visit_results`. The following example uses +//! there, you can use a `ResultsCursor` to inspect the fixpoint solution to your dataflow problem +//! (good for inspecting a small number of locations), or implement the `ResultsVisitor` interface +//! and use `visit_results` (good for inspecting many or all locations). The following example uses //! the `ResultsCursor` approach. //! //! ```ignore (cross-crate-imports) diff --git a/compiler/rustc_mir_dataflow/src/framework/results.rs b/compiler/rustc_mir_dataflow/src/framework/results.rs index ff6cafbfbaee6..7c775ae7f4a56 100644 --- a/compiler/rustc_mir_dataflow/src/framework/results.rs +++ b/compiler/rustc_mir_dataflow/src/framework/results.rs @@ -20,7 +20,9 @@ use crate::errors::{ pub type EntrySets<'tcx, A> = IndexVec>::Domain>; -/// A dataflow analysis that has converged to fixpoint. +/// A dataflow analysis that has converged to fixpoint. It only holds the domain values at the +/// entry of each basic block. Domain values in other parts of the block are recomputed on the fly +/// by visitors (i.e. `ResultsCursor`, or `ResultsVisitor` impls). #[derive(Clone)] pub struct Results<'tcx, A> where diff --git a/compiler/rustc_mir_dataflow/src/framework/visitor.rs b/compiler/rustc_mir_dataflow/src/framework/visitor.rs index 5c7539eed4d6a..bde41974d4727 100644 --- a/compiler/rustc_mir_dataflow/src/framework/visitor.rs +++ b/compiler/rustc_mir_dataflow/src/framework/visitor.rs @@ -26,7 +26,9 @@ pub fn visit_results<'mir, 'tcx, A>( } } -/// A visitor over the results of an `Analysis`. +/// A visitor over the results of an `Analysis`. Use this when you want to inspect domain values in +/// many or all locations; use `ResultsCursor` if you want to inspect domain values only in certain +/// locations. pub trait ResultsVisitor<'mir, 'tcx, A> where A: Analysis<'tcx>, From dae019dc9dd99ca092da03929406423d0527cfbb Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 18 Nov 2024 15:46:56 +1100 Subject: [PATCH 3/5] Remove `self` param for `MaybeBorrowedLocals::transfer_function`. It is unnecessary. --- compiler/rustc_mir_dataflow/src/impls/borrowed_locals.rs | 6 +++--- compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs | 5 +---- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/borrowed_locals.rs b/compiler/rustc_mir_dataflow/src/impls/borrowed_locals.rs index 859019fd1f6ee..cec654cac7251 100644 --- a/compiler/rustc_mir_dataflow/src/impls/borrowed_locals.rs +++ b/compiler/rustc_mir_dataflow/src/impls/borrowed_locals.rs @@ -15,7 +15,7 @@ use crate::{Analysis, GenKill}; pub struct MaybeBorrowedLocals; impl MaybeBorrowedLocals { - pub(super) fn transfer_function<'a, T>(&'a self, trans: &'a mut T) -> TransferFunction<'a, T> { + pub(super) fn transfer_function<'a, T>(trans: &'a mut T) -> TransferFunction<'a, T> { TransferFunction { trans } } } @@ -39,7 +39,7 @@ impl<'tcx> Analysis<'tcx> for MaybeBorrowedLocals { statement: &Statement<'tcx>, location: Location, ) { - self.transfer_function(trans).visit_statement(statement, location); + Self::transfer_function(trans).visit_statement(statement, location); } fn apply_terminator_effect<'mir>( @@ -48,7 +48,7 @@ impl<'tcx> Analysis<'tcx> for MaybeBorrowedLocals { terminator: &'mir Terminator<'tcx>, location: Location, ) -> TerminatorEdges<'mir, 'tcx> { - self.transfer_function(trans).visit_terminator(terminator, location); + Self::transfer_function(trans).visit_terminator(terminator, location); terminator.edges() } } diff --git a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs index 576289e228ad0..30992f38480f5 100644 --- a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs +++ b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs @@ -180,10 +180,7 @@ impl<'tcx> Analysis<'tcx> for MaybeRequiresStorage<'_, 'tcx> { loc: Location, ) { // If a place is borrowed in a terminator, it needs storage for that terminator. - self.borrowed_locals - .mut_analysis() - .transfer_function(trans) - .visit_terminator(terminator, loc); + MaybeBorrowedLocals::transfer_function(trans).visit_terminator(terminator, loc); match &terminator.kind { TerminatorKind::Call { destination, .. } => { From 1914dbe69465132a545fbc4cd0400a02a56a8a77 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 18 Nov 2024 15:54:50 +1100 Subject: [PATCH 4/5] Tweak `MaybeBorrowedLocals::transfer_function` usage. In `MaybeRequiresStorage::apply_before_statement_effect`, call `transfer_function` directly, as is already done in `MaybeRequiresStorage::apply_before_terminator_effect`. This makes it clear that the operation doesn't rely on the `MaybeBorrowedLocals` results. --- compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs index 30992f38480f5..a6a4f849720fc 100644 --- a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs +++ b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs @@ -135,7 +135,7 @@ impl<'tcx> Analysis<'tcx> for MaybeRequiresStorage<'_, 'tcx> { loc: Location, ) { // If a place is borrowed in a statement, it needs storage for that statement. - self.borrowed_locals.mut_analysis().apply_statement_effect(trans, stmt, loc); + MaybeBorrowedLocals::transfer_function(trans).visit_statement(stmt, loc); match &stmt.kind { StatementKind::StorageDead(l) => trans.kill(*l), From be7c6a3b43a8d200ba29f5a768e32287bb95aec2 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 18 Nov 2024 16:04:03 +1100 Subject: [PATCH 5/5] Make it possible for `ResultsCursor` to borrow a `Results`. `ResultsCursor` currently owns its `Results`. But sometimes the `Results` is needed again afterwards. So there is `ResultsCursor::into_results` for extracting the `Results`, which leads to some awkwardness. This commit adds `ResultsHandle`, a `Cow`-like type that can either borrow or own a a `Results`. `ResultsCursor` now uses it. This is good because some `ResultsCursor`s really want to own their `Results`, while others just want to borrow it. We end with with a few more lines of code, but get some nice cleanups. - `ResultsCursor::into_results` and `Formatter::into_results` are removed. - `write_graphviz_results` now just borrows a `Results`, instead of the awkward "take ownership of a `Results` and then return it unchanged" pattern. This reinstates the cursor flexibility that was lost in #118230 -- which removed the old `ResultsRefCursor` and `ResultsCloneCursor` types -- but in a much simpler way. Hooray! --- .../src/framework/cursor.rs | 51 ++++++++++++++++--- .../src/framework/graphviz.rs | 8 +-- .../rustc_mir_dataflow/src/framework/mod.rs | 7 ++- .../src/framework/results.rs | 35 ++++++++++--- compiler/rustc_mir_transform/src/coroutine.rs | 11 ++-- 5 files changed, 81 insertions(+), 31 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/framework/cursor.rs b/compiler/rustc_mir_dataflow/src/framework/cursor.rs index fbd9376917a14..11cf8c3e89848 100644 --- a/compiler/rustc_mir_dataflow/src/framework/cursor.rs +++ b/compiler/rustc_mir_dataflow/src/framework/cursor.rs @@ -1,6 +1,7 @@ //! Random access inspection of the results of a dataflow analysis. use std::cmp::Ordering; +use std::ops::{Deref, DerefMut}; #[cfg(debug_assertions)] use rustc_index::bit_set::BitSet; @@ -8,6 +9,47 @@ use rustc_middle::mir::{self, BasicBlock, Location}; use super::{Analysis, Direction, Effect, EffectIndex, Results}; +/// Some `ResultsCursor`s want to own a `Results`, and some want to borrow a `Results`, either +/// mutable or immutably. This type allows all of the above. It's similar to `Cow`. +pub enum ResultsHandle<'a, 'tcx, A> +where + A: Analysis<'tcx>, +{ + Borrowed(&'a Results<'tcx, A>), + BorrowedMut(&'a mut Results<'tcx, A>), + Owned(Results<'tcx, A>), +} + +impl<'tcx, A> Deref for ResultsHandle<'_, 'tcx, A> +where + A: Analysis<'tcx>, +{ + type Target = Results<'tcx, A>; + + fn deref(&self) -> &Results<'tcx, A> { + match self { + ResultsHandle::Borrowed(borrowed) => borrowed, + ResultsHandle::BorrowedMut(borrowed) => borrowed, + ResultsHandle::Owned(owned) => owned, + } + } +} + +impl<'tcx, A> DerefMut for ResultsHandle<'_, 'tcx, A> +where + A: Analysis<'tcx>, +{ + fn deref_mut(&mut self) -> &mut Results<'tcx, A> { + match self { + ResultsHandle::Borrowed(_borrowed) => { + panic!("tried to deref_mut a `ResultsHandle::Borrowed") + } + ResultsHandle::BorrowedMut(borrowed) => borrowed, + ResultsHandle::Owned(owned) => owned, + } + } +} + /// Allows random access inspection of the results of a dataflow analysis. Use this when you want /// to inspect domain values only in certain locations; use `ResultsVisitor` if you want to inspect /// domain values in many or all locations. @@ -23,7 +65,7 @@ where A: Analysis<'tcx>, { body: &'mir mir::Body<'tcx>, - results: Results<'tcx, A>, + results: ResultsHandle<'mir, 'tcx, A>, state: A::Domain, pos: CursorPosition, @@ -51,13 +93,8 @@ where self.body } - /// Unwraps this cursor, returning the underlying `Results`. - pub fn into_results(self) -> Results<'tcx, A> { - self.results - } - /// Returns a new cursor that can inspect `results`. - pub fn new(body: &'mir mir::Body<'tcx>, results: Results<'tcx, A>) -> Self { + pub fn new(body: &'mir mir::Body<'tcx>, results: ResultsHandle<'mir, 'tcx, A>) -> Self { let bottom_value = results.analysis.bottom_value(body); ResultsCursor { body, diff --git a/compiler/rustc_mir_dataflow/src/framework/graphviz.rs b/compiler/rustc_mir_dataflow/src/framework/graphviz.rs index 98a4f58cb5dc3..6e4994af8b4e4 100644 --- a/compiler/rustc_mir_dataflow/src/framework/graphviz.rs +++ b/compiler/rustc_mir_dataflow/src/framework/graphviz.rs @@ -47,20 +47,16 @@ where { pub(crate) fn new( body: &'mir Body<'tcx>, - results: Results<'tcx, A>, + results: &'mir Results<'tcx, A>, style: OutputStyle, ) -> Self { let reachable = mir::traversal::reachable_as_bitset(body); - Formatter { cursor: results.into_results_cursor(body).into(), style, reachable } + Formatter { cursor: results.as_results_cursor(body).into(), style, reachable } } fn body(&self) -> &'mir Body<'tcx> { self.cursor.borrow().body() } - - pub(crate) fn into_results(self) -> Results<'tcx, A> { - self.cursor.into_inner().into_results() - } } /// A pair of a basic block and an index into that basic blocks `successors`. diff --git a/compiler/rustc_mir_dataflow/src/framework/mod.rs b/compiler/rustc_mir_dataflow/src/framework/mod.rs index f9e7ba743fc2f..88bab250781c6 100644 --- a/compiler/rustc_mir_dataflow/src/framework/mod.rs +++ b/compiler/rustc_mir_dataflow/src/framework/mod.rs @@ -302,14 +302,13 @@ pub trait Analysis<'tcx> { let results = Results { analysis: self, entry_sets }; if tcx.sess.opts.unstable_opts.dump_mir_dataflow { - let (res, results) = write_graphviz_results(tcx, body, results, pass_name); + let res = write_graphviz_results(tcx, body, &results, pass_name); if let Err(e) = res { error!("Failed to write graphviz dataflow results: {}", e); } - results - } else { - results } + + results } } diff --git a/compiler/rustc_mir_dataflow/src/framework/results.rs b/compiler/rustc_mir_dataflow/src/framework/results.rs index 7c775ae7f4a56..8493a7aa44bb1 100644 --- a/compiler/rustc_mir_dataflow/src/framework/results.rs +++ b/compiler/rustc_mir_dataflow/src/framework/results.rs @@ -17,6 +17,7 @@ use super::{Analysis, ResultsCursor, ResultsVisitor, graphviz, visit_results}; use crate::errors::{ DuplicateValuesFor, PathMustEndInFilename, RequiresAnArgument, UnknownFormatter, }; +use crate::framework::cursor::ResultsHandle; pub type EntrySets<'tcx, A> = IndexVec>::Domain>; @@ -36,12 +37,30 @@ impl<'tcx, A> Results<'tcx, A> where A: Analysis<'tcx>, { - /// Creates a `ResultsCursor` that can inspect these `Results`. + /// Creates a `ResultsCursor` that can inspect these `Results`. Immutably borrows the `Results`, + /// which is appropriate when the `Results` is used outside the cursor. + pub fn as_results_cursor<'mir>( + &'mir self, + body: &'mir mir::Body<'tcx>, + ) -> ResultsCursor<'mir, 'tcx, A> { + ResultsCursor::new(body, ResultsHandle::Borrowed(self)) + } + + /// Creates a `ResultsCursor` that can mutate these `Results`. Mutably borrows the `Results`, + /// which is appropriate when the `Results` is used outside the cursor. + pub fn as_results_cursor_mut<'mir>( + &'mir mut self, + body: &'mir mir::Body<'tcx>, + ) -> ResultsCursor<'mir, 'tcx, A> { + ResultsCursor::new(body, ResultsHandle::BorrowedMut(self)) + } + + /// Creates a `ResultsCursor` that takes ownership of the `Results`. pub fn into_results_cursor<'mir>( self, body: &'mir mir::Body<'tcx>, ) -> ResultsCursor<'mir, 'tcx, A> { - ResultsCursor::new(body, self) + ResultsCursor::new(body, ResultsHandle::Owned(self)) } /// Gets the dataflow state for the given block. @@ -76,9 +95,9 @@ where pub(super) fn write_graphviz_results<'tcx, A>( tcx: TyCtxt<'tcx>, body: &mir::Body<'tcx>, - results: Results<'tcx, A>, + results: &Results<'tcx, A>, pass_name: Option<&'static str>, -) -> (std::io::Result<()>, Results<'tcx, A>) +) -> std::io::Result<()> where A: Analysis<'tcx>, A::Domain: DebugWithContext, @@ -89,7 +108,7 @@ where let def_id = body.source.def_id(); let Ok(attrs) = RustcMirAttrs::parse(tcx, def_id) else { // Invalid `rustc_mir` attrs are reported in `RustcMirAttrs::parse` - return (Ok(()), results); + return Ok(()); }; let file = try { @@ -106,12 +125,12 @@ where create_dump_file(tcx, "dot", false, A::NAME, &pass_name.unwrap_or("-----"), body)? } - _ => return (Ok(()), results), + _ => return Ok(()), } }; let mut file = match file { Ok(f) => f, - Err(e) => return (Err(e), results), + Err(e) => return Err(e), }; let style = match attrs.formatter { @@ -134,7 +153,7 @@ where file.write_all(&buf)?; }; - (lhs, graphviz.into_results()) + lhs } #[derive(Default)] diff --git a/compiler/rustc_mir_transform/src/coroutine.rs b/compiler/rustc_mir_transform/src/coroutine.rs index 8295a806d7125..ae5506b05e74c 100644 --- a/compiler/rustc_mir_transform/src/coroutine.rs +++ b/compiler/rustc_mir_transform/src/coroutine.rs @@ -676,12 +676,11 @@ fn locals_live_across_suspend_points<'tcx>( let mut borrowed_locals_cursor = borrowed_locals_results.clone().into_results_cursor(body); - // Calculate the MIR locals that we actually need to keep storage around - // for. - let mut requires_storage_cursor = + // Calculate the MIR locals that we need to keep storage around for. + let mut requires_storage_results = MaybeRequiresStorage::new(borrowed_locals_results.into_results_cursor(body)) - .iterate_to_fixpoint(tcx, body, None) - .into_results_cursor(body); + .iterate_to_fixpoint(tcx, body, None); + let mut requires_storage_cursor = requires_storage_results.as_results_cursor_mut(body); // Calculate the liveness of MIR locals ignoring borrows. let mut liveness = @@ -754,7 +753,7 @@ fn locals_live_across_suspend_points<'tcx>( body, &saved_locals, always_live_locals.clone(), - requires_storage_cursor.into_results(), + requires_storage_results, ); LivenessInfo {