Remove Engine::new_gen_kill.

nnethercote · nnethercote · commit e0b83c34c332 · 2024-10-14T16:35:28.000+11:00
This is an alternative to `Engine::new_generic` for gen/kill analyses.
It's supposed to be an optimization, but it has negligible effect.
The commit merges `Engine::new_generic` into `Engine::new`.

This allows the removal of various other things: `GenKillSet`,
`gen_kill_statement_effects_in_block`, `is_cfg_cyclic`.
diff --git a/compiler/rustc_middle/src/mir/basic_blocks.rs b/compiler/rustc_middle/src/mir/basic_blocks.rs
@@ -26,7 +26,6 @@ type SwitchSources = FxHashMap<(BasicBlock, BasicBlock), SmallVec<[Option<u128>;
 struct Cache {
     predecessors: OnceLock<Predecessors>,
     switch_sources: OnceLock<SwitchSources>,
-    is_cyclic: OnceLock<bool>,
     reverse_postorder: OnceLock<Vec<BasicBlock>>,
     dominators: OnceLock<Dominators<BasicBlock>>,
 }
@@ -37,12 +36,6 @@ impl<'tcx> BasicBlocks<'tcx> {
         BasicBlocks { basic_blocks, cache: Cache::default() }
     }
 
-    /// Returns true if control-flow graph contains a cycle reachable from the `START_BLOCK`.
-    #[inline]
-    pub fn is_cfg_cyclic(&self) -> bool {
-        *self.cache.is_cyclic.get_or_init(|| graph::is_cyclic(self))
-    }
-
     pub fn dominators(&self) -> &Dominators<BasicBlock> {
         self.cache.dominators.get_or_init(|| dominators(self))
     }
diff --git a/compiler/rustc_mir_dataflow/src/framework/direction.rs b/compiler/rustc_mir_dataflow/src/framework/direction.rs
@@ -5,7 +5,7 @@ use rustc_middle::mir::{
 };
 
 use super::visitor::{ResultsVisitable, ResultsVisitor};
-use super::{Analysis, Effect, EffectIndex, GenKillAnalysis, GenKillSet, SwitchIntTarget};
+use super::{Analysis, Effect, EffectIndex, SwitchIntTarget};
 
 pub trait Direction {
     const IS_FORWARD: bool;
@@ -29,19 +29,10 @@ pub trait Direction {
         state: &mut A::Domain,
         block: BasicBlock,
         block_data: &'mir mir::BasicBlockData<'tcx>,
-        statement_effect: Option<&dyn Fn(BasicBlock, &mut A::Domain)>,
     ) -> TerminatorEdges<'mir, 'tcx>
     where
         A: Analysis<'tcx>;
 
-    fn gen_kill_statement_effects_in_block<'tcx, A>(
-        analysis: &mut A,
-        trans: &mut GenKillSet<A::Idx>,
-        block: BasicBlock,
-        block_data: &mir::BasicBlockData<'tcx>,
-    ) where
-        A: GenKillAnalysis<'tcx>;
-
     fn visit_results_in_block<'mir, 'tcx, D, R>(
         state: &mut D,
         block: BasicBlock,
@@ -73,7 +64,6 @@ impl Direction for Backward {
         state: &mut A::Domain,
         block: BasicBlock,
         block_data: &'mir mir::BasicBlockData<'tcx>,
-        statement_effect: Option<&dyn Fn(BasicBlock, &mut A::Domain)>,
     ) -> TerminatorEdges<'mir, 'tcx>
     where
         A: Analysis<'tcx>,
@@ -82,31 +72,12 @@ impl Direction for Backward {
         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);
-        if let Some(statement_effect) = statement_effect {
-            statement_effect(block, state)
-        } else {
-            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
-    }
-
-    fn gen_kill_statement_effects_in_block<'tcx, A>(
-        analysis: &mut A,
-        trans: &mut GenKillSet<A::Idx>,
-        block: BasicBlock,
-        block_data: &mir::BasicBlockData<'tcx>,
-    ) where
-        A: GenKillAnalysis<'tcx>,
-    {
         for (statement_index, statement) in block_data.statements.iter().enumerate().rev() {
             let location = Location { block, statement_index };
-            analysis.before_statement_effect(trans, statement, location);
-            analysis.statement_effect(trans, statement, location);
+            analysis.apply_before_statement_effect(state, statement, location);
+            analysis.apply_statement_effect(state, statement, location);
         }
+        edges
     }
 
     fn apply_effects_in_range<'tcx, A>(
@@ -330,42 +301,21 @@ impl Direction for Forward {
         state: &mut A::Domain,
         block: BasicBlock,
         block_data: &'mir mir::BasicBlockData<'tcx>,
-        statement_effect: Option<&dyn Fn(BasicBlock, &mut A::Domain)>,
     ) -> TerminatorEdges<'mir, 'tcx>
     where
         A: Analysis<'tcx>,
     {
-        if let Some(statement_effect) = statement_effect {
-            statement_effect(block, state)
-        } else {
-            for (statement_index, statement) in block_data.statements.iter().enumerate() {
-                let location = Location { block, statement_index };
-                analysis.apply_before_statement_effect(state, statement, location);
-                analysis.apply_statement_effect(state, statement, location);
-            }
+        for (statement_index, statement) in block_data.statements.iter().enumerate() {
+            let location = Location { block, statement_index };
+            analysis.apply_before_statement_effect(state, statement, location);
+            analysis.apply_statement_effect(state, statement, location);
         }
-
         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)
     }
 
-    fn gen_kill_statement_effects_in_block<'tcx, A>(
-        analysis: &mut A,
-        trans: &mut GenKillSet<A::Idx>,
-        block: BasicBlock,
-        block_data: &mir::BasicBlockData<'tcx>,
-    ) where
-        A: GenKillAnalysis<'tcx>,
-    {
-        for (statement_index, statement) in block_data.statements.iter().enumerate() {
-            let location = Location { block, statement_index };
-            analysis.before_statement_effect(trans, statement, location);
-            analysis.statement_effect(trans, statement, location);
-        }
-    }
-
     fn apply_effects_in_range<'tcx, A>(
         analysis: &mut A,
         state: &mut A::Domain,
diff --git a/compiler/rustc_mir_dataflow/src/framework/engine.rs b/compiler/rustc_mir_dataflow/src/framework/engine.rs
@@ -5,7 +5,7 @@ use std::path::PathBuf;
 
 use rustc_data_structures::work_queue::WorkQueue;
 use rustc_hir::def_id::DefId;
-use rustc_index::{Idx, IndexVec};
+use rustc_index::IndexVec;
 use rustc_middle::bug;
 use rustc_middle::mir::{self, BasicBlock, create_dump_file, dump_enabled, traversal};
 use rustc_middle::ty::TyCtxt;
@@ -16,13 +16,12 @@ use {rustc_ast as ast, rustc_graphviz as dot};
 
 use super::fmt::DebugWithContext;
 use super::{
-    Analysis, AnalysisDomain, Direction, GenKill, GenKillAnalysis, GenKillSet, JoinSemiLattice,
-    ResultsCursor, ResultsVisitor, graphviz, visit_results,
+    Analysis, AnalysisDomain, Direction, JoinSemiLattice, ResultsCursor, ResultsVisitor, graphviz,
+    visit_results,
 };
 use crate::errors::{
     DuplicateValuesFor, PathMustEndInFilename, RequiresAnArgument, UnknownFormatter,
 };
-use crate::framework::BitSetExt;
 
 type EntrySets<'tcx, A> = IndexVec<BasicBlock, <A as AnalysisDomain<'tcx>>::Domain>;
 
@@ -82,53 +81,6 @@ where
     entry_sets: IndexVec<BasicBlock, A::Domain>,
     pass_name: Option<&'static str>,
     analysis: A,
-
-    /// Cached, cumulative transfer functions for each block.
-    //
-    // FIXME(ecstaticmorse): This boxed `Fn` trait object is invoked inside a tight loop for
-    // gen/kill problems on cyclic CFGs. This is not ideal, but it doesn't seem to degrade
-    // performance in practice. I've tried a few ways to avoid this, but they have downsides. See
-    // the message for the commit that added this FIXME for more information.
-    apply_statement_trans_for_block: Option<Box<dyn Fn(BasicBlock, &mut A::Domain)>>,
-}
-
-impl<'mir, 'tcx, A, D, T> Engine<'mir, 'tcx, A>
-where
-    A: GenKillAnalysis<'tcx, Idx = T, Domain = D>,
-    D: Clone + JoinSemiLattice + GenKill<T> + BitSetExt<T>,
-    T: Idx,
-{
-    /// Creates a new `Engine` to solve a gen-kill dataflow problem.
-    pub fn new_gen_kill(tcx: TyCtxt<'tcx>, body: &'mir mir::Body<'tcx>, mut analysis: A) -> Self {
-        // If there are no back-edges in the control-flow graph, we only ever need to apply the
-        // transfer function for each block exactly once (assuming that we process blocks in RPO).
-        //
-        // In this case, there's no need to compute the block transfer functions ahead of time.
-        if !body.basic_blocks.is_cfg_cyclic() {
-            return Self::new(tcx, body, analysis, None);
-        }
-
-        // Otherwise, compute and store the cumulative transfer function for each block.
-
-        let identity = GenKillSet::identity(analysis.domain_size(body));
-        let mut trans_for_block = IndexVec::from_elem(identity, &body.basic_blocks);
-
-        for (block, block_data) in body.basic_blocks.iter_enumerated() {
-            let trans = &mut trans_for_block[block];
-            A::Direction::gen_kill_statement_effects_in_block(
-                &mut analysis,
-                trans,
-                block,
-                block_data,
-            );
-        }
-
-        let apply_trans = Box::new(move |bb: BasicBlock, state: &mut A::Domain| {
-            trans_for_block[bb].apply(state);
-        });
-
-        Self::new(tcx, body, analysis, Some(apply_trans as Box<_>))
-    }
 }
 
 impl<'mir, 'tcx, A, D> Engine<'mir, 'tcx, A>
@@ -138,19 +90,7 @@ where
 {
     /// Creates a new `Engine` to solve a dataflow problem with an arbitrary transfer
     /// function.
-    ///
-    /// Gen-kill problems should use `new_gen_kill`, which will coalesce transfer functions for
-    /// better performance.
-    pub fn new_generic(tcx: TyCtxt<'tcx>, body: &'mir mir::Body<'tcx>, analysis: A) -> Self {
-        Self::new(tcx, body, analysis, None)
-    }
-
-    fn new(
-        tcx: TyCtxt<'tcx>,
-        body: &'mir mir::Body<'tcx>,
-        analysis: A,
-        apply_statement_trans_for_block: Option<Box<dyn Fn(BasicBlock, &mut A::Domain)>>,
-    ) -> Self {
+    pub(crate) fn new(tcx: TyCtxt<'tcx>, body: &'mir mir::Body<'tcx>, analysis: A) -> Self {
         let mut entry_sets =
             IndexVec::from_fn_n(|_| analysis.bottom_value(body), body.basic_blocks.len());
         analysis.initialize_start_block(body, &mut entry_sets[mir::START_BLOCK]);
@@ -160,7 +100,7 @@ where
             bug!("`initialize_start_block` is not yet supported for backward dataflow analyses");
         }
 
-        Engine { analysis, tcx, body, pass_name: None, entry_sets, apply_statement_trans_for_block }
+        Engine { analysis, tcx, body, pass_name: None, entry_sets }
     }
 
     /// Adds an identifier to the graphviz output for this particular run of a dataflow analysis.
@@ -177,14 +117,7 @@ where
     where
         A::Domain: DebugWithContext<A>,
     {
-        let Engine {
-            mut analysis,
-            body,
-            mut entry_sets,
-            tcx,
-            apply_statement_trans_for_block,
-            pass_name,
-        } = self;
+        let Engine { mut analysis, body, mut entry_sets, tcx, pass_name } = self;
 
         let mut dirty_queue: WorkQueue<BasicBlock> = WorkQueue::with_none(body.basic_blocks.len());
 
@@ -213,13 +146,8 @@ where
             state.clone_from(&entry_sets[bb]);
 
             // Apply the block transfer function, using the cached one if it exists.
-            let edges = A::Direction::apply_effects_in_block(
-                &mut analysis,
-                &mut state,
-                bb,
-                bb_data,
-                apply_statement_trans_for_block.as_deref(),
-            );
+            let edges =
+                A::Direction::apply_effects_in_block(&mut analysis, &mut state, bb, bb_data);
 
             A::Direction::join_state_into_successors_of(
                 &mut analysis,
diff --git a/compiler/rustc_mir_dataflow/src/framework/mod.rs b/compiler/rustc_mir_dataflow/src/framework/mod.rs
@@ -242,7 +242,7 @@ pub trait Analysis<'tcx>: AnalysisDomain<'tcx> {
     where
         Self: Sized,
     {
-        Engine::new_generic(tcx, body, self)
+        Engine::new(tcx, body, self)
     }
 }
 
@@ -376,19 +376,6 @@ where
     ) {
         self.switch_int_edge_effects(block, discr, edge_effects);
     }
-
-    /* Extension methods */
-    #[inline]
-    fn into_engine<'mir>(
-        self,
-        tcx: TyCtxt<'tcx>,
-        body: &'mir mir::Body<'tcx>,
-    ) -> Engine<'mir, 'tcx, Self>
-    where
-        Self: Sized,
-    {
-        Engine::new_gen_kill(tcx, body, self)
-    }
 }
 
 /// The legal operations for a transfer function in a gen/kill problem.
@@ -422,44 +409,6 @@ pub trait GenKill<T> {
     }
 }
 
-/// Stores a transfer function for a gen/kill problem.
-///
-/// Calling `gen_`/`kill` on a `GenKillSet` will "build up" a transfer function so that it can be
-/// applied multiple times efficiently. When there are multiple calls to `gen_` and/or `kill` for
-/// the same element, the most recent one takes precedence.
-#[derive(Clone)]
-pub struct GenKillSet<T> {
-    gen_: HybridBitSet<T>,
-    kill: HybridBitSet<T>,
-}
-
-impl<T: Idx> GenKillSet<T> {
-    /// Creates a new transfer function that will leave the dataflow state unchanged.
-    pub fn identity(universe: usize) -> Self {
-        GenKillSet {
-            gen_: HybridBitSet::new_empty(universe),
-            kill: HybridBitSet::new_empty(universe),
-        }
-    }
-
-    pub fn apply(&self, state: &mut impl BitSetExt<T>) {
-        state.union(&self.gen_);
-        state.subtract(&self.kill);
-    }
-}
-
-impl<T: Idx> GenKill<T> for GenKillSet<T> {
-    fn gen_(&mut self, elem: T) {
-        self.gen_.insert(elem);
-        self.kill.remove(elem);
-    }
-
-    fn kill(&mut self, elem: T) {
-        self.kill.insert(elem);
-        self.gen_.remove(elem);
-    }
-}
-
 impl<T: Idx> GenKill<T> for BitSet<T> {
     fn gen_(&mut self, elem: T) {
         self.insert(elem);

Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,6 @@ type SwitchSources = FxHashMap<(BasicBlock, BasicBlock), SmallVec<[Option<u128>;`
`26`	`26`	`struct Cache {`
`27`	`27`	`predecessors: OnceLock<Predecessors>,`
`28`	`28`	`switch_sources: OnceLock<SwitchSources>,`
`29`		`- is_cyclic: OnceLock<bool>,`
`30`	`29`	`reverse_postorder: OnceLock<Vec<BasicBlock>>,`
`31`	`30`	`dominators: OnceLock<Dominators<BasicBlock>>,`
`32`	`31`	`}`
`@@ -37,12 +36,6 @@ impl<'tcx> BasicBlocks<'tcx> {`
`37`	`36`	`BasicBlocks { basic_blocks, cache: Cache::default() }`
`38`	`37`	`}`
`39`	`38`
`40`		- /// Returns true if control-flow graph contains a cycle reachable from the `START_BLOCK`.
`41`		`- #[inline]`
`42`		`- pub fn is_cfg_cyclic(&self) -> bool {`
`43`		`- *self.cache.is_cyclic.get_or_init(\|\| graph::is_cyclic(self))`
`44`		`- }`
`45`		`-`
`46`	`39`	`pub fn dominators(&self) -> &Dominators<BasicBlock> {`
`47`	`40`	`self.cache.dominators.get_or_init(\|\| dominators(self))`
`48`	`41`	`}`