From fbc6067f97ff7ccde88fa10f37ea48f793fed3f9 Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Thu, 15 Feb 2024 12:48:23 -0800 Subject: [PATCH 1/4] Don't keep extra copies of root block ID We already know the entry block everywhere we use this. --- cranelift/codegen/src/egraph.rs | 2 +- cranelift/codegen/src/egraph/domtree.rs | 9 +-------- cranelift/codegen/src/egraph/elaborate.rs | 3 +-- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/cranelift/codegen/src/egraph.rs b/cranelift/codegen/src/egraph.rs index 32f67028795c..55510d01e23f 100644 --- a/cranelift/codegen/src/egraph.rs +++ b/cranelift/codegen/src/egraph.rs @@ -497,7 +497,7 @@ impl<'a> EgraphPass<'a> { // In domtree preorder, visit blocks. (TODO: factor out an // iterator from this and elaborator.) - let root = self.domtree_children.root(); + let root = cursor.layout().entry_block().unwrap(); enum StackEntry { Visit(Block), Pop, diff --git a/cranelift/codegen/src/egraph/domtree.rs b/cranelift/codegen/src/egraph/domtree.rs index aa76f6781c0f..13413d289374 100644 --- a/cranelift/codegen/src/egraph/domtree.rs +++ b/cranelift/codegen/src/egraph/domtree.rs @@ -9,7 +9,6 @@ use cranelift_entity::{packed_option::PackedOption, SecondaryMap}; /// rather than parent pointers. pub(crate) struct DomTreeWithChildren { nodes: SecondaryMap, - root: Block, } #[derive(Clone, Copy, Debug, Default)] @@ -40,13 +39,7 @@ impl DomTreeWithChildren { nodes[idom].children = block.into(); } - let root = func.layout.entry_block().unwrap(); - - Self { nodes, root } - } - - pub(crate) fn root(&self) -> Block { - self.root + Self { nodes } } pub(crate) fn children<'a>(&'a self, block: Block) -> DomTreeChildIter<'a> { diff --git a/cranelift/codegen/src/egraph/elaborate.rs b/cranelift/codegen/src/egraph/elaborate.rs index f2d02d0f674d..1a92f604da54 100644 --- a/cranelift/codegen/src/egraph/elaborate.rs +++ b/cranelift/codegen/src/egraph/elaborate.rs @@ -763,9 +763,8 @@ impl<'a> Elaborator<'a> { } fn elaborate_domtree(&mut self, domtree: &DomTreeWithChildren) { - let root = domtree.root(); self.block_stack.push(BlockStackEntry::Elaborate { - block: root, + block: self.func.layout.entry_block().unwrap(), idom: None, }); From 9062c99e4fd3c68d67a8cb2f01b988968326fcb1 Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Thu, 15 Feb 2024 12:54:39 -0800 Subject: [PATCH 2/4] Replace DomTreeWithChildren with DominatorTreePreorder These two types had nearly identical interfaces except that the latter also supports constant-time dominance checks. --- cranelift/codegen/src/egraph.rs | 9 ++- cranelift/codegen/src/egraph/domtree.rs | 67 ----------------------- cranelift/codegen/src/egraph/elaborate.rs | 9 ++- 3 files changed, 8 insertions(+), 77 deletions(-) delete mode 100644 cranelift/codegen/src/egraph/domtree.rs diff --git a/cranelift/codegen/src/egraph.rs b/cranelift/codegen/src/egraph.rs index 55510d01e23f..08497980fa1e 100644 --- a/cranelift/codegen/src/egraph.rs +++ b/cranelift/codegen/src/egraph.rs @@ -3,8 +3,7 @@ use crate::alias_analysis::{AliasAnalysis, LastStores}; use crate::ctxhash::{CtxEq, CtxHash, CtxHashMap}; use crate::cursor::{Cursor, CursorPosition, FuncCursor}; -use crate::dominator_tree::DominatorTree; -use crate::egraph::domtree::DomTreeWithChildren; +use crate::dominator_tree::{DominatorTree, DominatorTreePreorder}; use crate::egraph::elaborate::Elaborator; use crate::fx::FxHashSet; use crate::inst_predicates::{is_mergeable_for_egraph, is_pure_for_egraph}; @@ -22,7 +21,6 @@ use smallvec::SmallVec; use std::hash::Hasher; mod cost; -mod domtree; mod elaborate; /// Pass over a Function that does the whole aegraph thing. @@ -53,7 +51,7 @@ pub struct EgraphPass<'a> { /// "Domtree with children": like `domtree`, but with an explicit /// list of children, complementing the parent pointers stored /// in `domtree`. - domtree_children: DomTreeWithChildren, + domtree_children: DominatorTreePreorder, /// Loop analysis results, used for built-in LICM during /// elaboration. loop_analysis: &'a LoopAnalysis, @@ -406,7 +404,8 @@ impl<'a> EgraphPass<'a> { alias_analysis: &'a mut AliasAnalysis<'a>, ) -> Self { let num_values = func.dfg.num_values(); - let domtree_children = DomTreeWithChildren::new(func, domtree); + let mut domtree_children = DominatorTreePreorder::new(); + domtree_children.compute(domtree, &func.layout); Self { func, domtree, diff --git a/cranelift/codegen/src/egraph/domtree.rs b/cranelift/codegen/src/egraph/domtree.rs deleted file mode 100644 index 13413d289374..000000000000 --- a/cranelift/codegen/src/egraph/domtree.rs +++ /dev/null @@ -1,67 +0,0 @@ -//! Extended domtree with various traversal support. - -use crate::dominator_tree::DominatorTree; -use crate::ir::{Block, Function}; -use cranelift_entity::{packed_option::PackedOption, SecondaryMap}; - -#[derive(Clone, Debug)] -/// Like a [`DominatorTree`], but with an explicit list of children, -/// rather than parent pointers. -pub(crate) struct DomTreeWithChildren { - nodes: SecondaryMap, -} - -#[derive(Clone, Copy, Debug, Default)] -struct DomTreeNode { - /// Points to the first child of the node, and implicitly to an entire - /// linked list of the children. - children: PackedOption, - /// Points to the next sibling, if any. - next: PackedOption, -} - -impl DomTreeWithChildren { - pub(crate) fn new(func: &Function, domtree: &DominatorTree) -> DomTreeWithChildren { - let mut nodes: SecondaryMap = - SecondaryMap::with_capacity(func.dfg.num_blocks()); - - for block in func.layout.blocks() { - let Some(idom_inst) = domtree.idom(block) else { - continue; - }; - let idom = func - .layout - .inst_block(idom_inst) - .expect("Dominating instruction should be part of a block"); - - // Insert at the front of nodes[idom].children - nodes[block].next = nodes[idom].children; - nodes[idom].children = block.into(); - } - - Self { nodes } - } - - pub(crate) fn children<'a>(&'a self, block: Block) -> DomTreeChildIter<'a> { - let block = self.nodes[block].children; - DomTreeChildIter { - domtree: self, - block, - } - } -} - -pub(crate) struct DomTreeChildIter<'a> { - domtree: &'a DomTreeWithChildren, - block: PackedOption, -} - -impl<'a> Iterator for DomTreeChildIter<'a> { - type Item = Block; - fn next(&mut self) -> Option { - self.block.expand().map(|block| { - self.block = self.domtree.nodes[block].next; - block - }) - } -} diff --git a/cranelift/codegen/src/egraph/elaborate.rs b/cranelift/codegen/src/egraph/elaborate.rs index 1a92f604da54..9d2a0496daca 100644 --- a/cranelift/codegen/src/egraph/elaborate.rs +++ b/cranelift/codegen/src/egraph/elaborate.rs @@ -2,9 +2,8 @@ //! in CFG nodes. use super::cost::Cost; -use super::domtree::DomTreeWithChildren; use super::Stats; -use crate::dominator_tree::DominatorTree; +use crate::dominator_tree::{DominatorTree, DominatorTreePreorder}; use crate::fx::{FxHashMap, FxHashSet}; use crate::hash_map::Entry as HashEntry; use crate::inst_predicates::is_pure_for_egraph; @@ -19,7 +18,7 @@ use smallvec::{smallvec, SmallVec}; pub(crate) struct Elaborator<'a> { func: &'a mut Function, domtree: &'a DominatorTree, - domtree_children: &'a DomTreeWithChildren, + domtree_children: &'a DominatorTreePreorder, loop_analysis: &'a LoopAnalysis, /// Map from Value that is produced by a pure Inst (and was thus /// not in the side-effecting skeleton) to the value produced by @@ -138,7 +137,7 @@ impl<'a> Elaborator<'a> { pub(crate) fn new( func: &'a mut Function, domtree: &'a DominatorTree, - domtree_children: &'a DomTreeWithChildren, + domtree_children: &'a DominatorTreePreorder, loop_analysis: &'a LoopAnalysis, remat_values: &'a FxHashSet, stats: &'a mut Stats, @@ -762,7 +761,7 @@ impl<'a> Elaborator<'a> { } } - fn elaborate_domtree(&mut self, domtree: &DomTreeWithChildren) { + fn elaborate_domtree(&mut self, domtree: &DominatorTreePreorder) { self.block_stack.push(BlockStackEntry::Elaborate { block: self.func.layout.entry_block().unwrap(), idom: None, From 0a9a9b7ae685a5e79256df36baa96f3c1e33a628 Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Fri, 16 Feb 2024 12:07:51 -0800 Subject: [PATCH 3/4] Use dom-tree preorder for O(1) dominance checks It turns out this was the only thing the egraph pass used the original dominator tree for, so we can stop passing that around. We could also require that the caller of EgraphPass::new construct the DominatorTreePreorder and avoid passing the original tree into the egraph pass at all, but I've chosen to stop refactoring here. We should review all uses of DominatorTree::dominates to see if it makes sense to pass a DominatorTreePreorder around more places. --- cranelift/codegen/src/egraph.rs | 4 ---- cranelift/codegen/src/egraph/elaborate.rs | 13 ++++--------- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/cranelift/codegen/src/egraph.rs b/cranelift/codegen/src/egraph.rs index 08497980fa1e..dcee18878504 100644 --- a/cranelift/codegen/src/egraph.rs +++ b/cranelift/codegen/src/egraph.rs @@ -44,8 +44,6 @@ mod elaborate; pub struct EgraphPass<'a> { /// The function we're operating on. func: &'a mut Function, - /// Dominator tree, used for elaboration pass. - domtree: &'a DominatorTree, /// Alias analysis, used during optimization. alias_analysis: &'a mut AliasAnalysis<'a>, /// "Domtree with children": like `domtree`, but with an explicit @@ -408,7 +406,6 @@ impl<'a> EgraphPass<'a> { domtree_children.compute(domtree, &func.layout); Self { func, - domtree, domtree_children, loop_analysis, alias_analysis, @@ -614,7 +611,6 @@ impl<'a> EgraphPass<'a> { fn elaborate(&mut self) { let mut elaborator = Elaborator::new( self.func, - self.domtree, &self.domtree_children, self.loop_analysis, &mut self.remat_values, diff --git a/cranelift/codegen/src/egraph/elaborate.rs b/cranelift/codegen/src/egraph/elaborate.rs index 9d2a0496daca..1544de48feed 100644 --- a/cranelift/codegen/src/egraph/elaborate.rs +++ b/cranelift/codegen/src/egraph/elaborate.rs @@ -3,7 +3,7 @@ use super::cost::Cost; use super::Stats; -use crate::dominator_tree::{DominatorTree, DominatorTreePreorder}; +use crate::dominator_tree::DominatorTreePreorder; use crate::fx::{FxHashMap, FxHashSet}; use crate::hash_map::Entry as HashEntry; use crate::inst_predicates::is_pure_for_egraph; @@ -17,7 +17,6 @@ use smallvec::{smallvec, SmallVec}; pub(crate) struct Elaborator<'a> { func: &'a mut Function, - domtree: &'a DominatorTree, domtree_children: &'a DominatorTreePreorder, loop_analysis: &'a LoopAnalysis, /// Map from Value that is produced by a pure Inst (and was thus @@ -136,7 +135,6 @@ enum BlockStackEntry { impl<'a> Elaborator<'a> { pub(crate) fn new( func: &'a mut Function, - domtree: &'a DominatorTree, domtree_children: &'a DominatorTreePreorder, loop_analysis: &'a LoopAnalysis, remat_values: &'a FxHashSet, @@ -148,7 +146,6 @@ impl<'a> Elaborator<'a> { value_to_best_value.resize(num_values); Self { func, - domtree, domtree_children, loop_analysis, value_to_elaborated_value: ScopedHashMap::with_capacity(num_values), @@ -556,11 +553,9 @@ impl<'a> Elaborator<'a> { let data = &self.loop_stack[loop_hoist_level]; // `data.hoist_block` should dominate `before`'s block. let before_block = self.func.layout.inst_block(before).unwrap(); - debug_assert!(self.domtree.dominates( - data.hoist_block, - before_block, - &self.func.layout - )); + debug_assert!(self + .domtree_children + .dominates(data.hoist_block, before_block)); // Determine the instruction at which we // insert in `data.hoist_block`. let before = self.func.layout.last_inst(data.hoist_block).unwrap(); From 69fb41b44c2f39d483f3e016f7c2e68988709286 Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Fri, 16 Feb 2024 12:19:02 -0800 Subject: [PATCH 4/4] review comments: Rename/document away from "domtree with children" --- cranelift/codegen/src/egraph.rs | 21 ++++++++++----------- cranelift/codegen/src/egraph/elaborate.rs | 12 +++++------- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/cranelift/codegen/src/egraph.rs b/cranelift/codegen/src/egraph.rs index dcee18878504..fd89743951b5 100644 --- a/cranelift/codegen/src/egraph.rs +++ b/cranelift/codegen/src/egraph.rs @@ -44,12 +44,12 @@ mod elaborate; pub struct EgraphPass<'a> { /// The function we're operating on. func: &'a mut Function, + /// Dominator tree for the CFG, used to visit blocks in pre-order + /// so we see value definitions before their uses, and also used for + /// O(1) dominance checks. + domtree: DominatorTreePreorder, /// Alias analysis, used during optimization. alias_analysis: &'a mut AliasAnalysis<'a>, - /// "Domtree with children": like `domtree`, but with an explicit - /// list of children, complementing the parent pointers stored - /// in `domtree`. - domtree_children: DominatorTreePreorder, /// Loop analysis results, used for built-in LICM during /// elaboration. loop_analysis: &'a LoopAnalysis, @@ -397,16 +397,16 @@ impl<'a> EgraphPass<'a> { /// Create a new EgraphPass. pub fn new( func: &'a mut Function, - domtree: &'a DominatorTree, + raw_domtree: &'a DominatorTree, loop_analysis: &'a LoopAnalysis, alias_analysis: &'a mut AliasAnalysis<'a>, ) -> Self { let num_values = func.dfg.num_values(); - let mut domtree_children = DominatorTreePreorder::new(); - domtree_children.compute(domtree, &func.layout); + let mut domtree = DominatorTreePreorder::new(); + domtree.compute(raw_domtree, &func.layout); Self { func, - domtree_children, + domtree, loop_analysis, alias_analysis, stats: Stats::default(), @@ -505,8 +505,7 @@ impl<'a> EgraphPass<'a> { // We popped this block; push children // immediately, then process this block. block_stack.push(StackEntry::Pop); - block_stack - .extend(self.domtree_children.children(block).map(StackEntry::Visit)); + block_stack.extend(self.domtree.children(block).map(StackEntry::Visit)); effectful_gvn_map.increment_depth(); trace!("Processing block {}", block); @@ -611,7 +610,7 @@ impl<'a> EgraphPass<'a> { fn elaborate(&mut self) { let mut elaborator = Elaborator::new( self.func, - &self.domtree_children, + &self.domtree, self.loop_analysis, &mut self.remat_values, &mut self.stats, diff --git a/cranelift/codegen/src/egraph/elaborate.rs b/cranelift/codegen/src/egraph/elaborate.rs index 1544de48feed..0e3ca285979d 100644 --- a/cranelift/codegen/src/egraph/elaborate.rs +++ b/cranelift/codegen/src/egraph/elaborate.rs @@ -17,7 +17,7 @@ use smallvec::{smallvec, SmallVec}; pub(crate) struct Elaborator<'a> { func: &'a mut Function, - domtree_children: &'a DominatorTreePreorder, + domtree: &'a DominatorTreePreorder, loop_analysis: &'a LoopAnalysis, /// Map from Value that is produced by a pure Inst (and was thus /// not in the side-effecting skeleton) to the value produced by @@ -135,7 +135,7 @@ enum BlockStackEntry { impl<'a> Elaborator<'a> { pub(crate) fn new( func: &'a mut Function, - domtree_children: &'a DominatorTreePreorder, + domtree: &'a DominatorTreePreorder, loop_analysis: &'a LoopAnalysis, remat_values: &'a FxHashSet, stats: &'a mut Stats, @@ -146,7 +146,7 @@ impl<'a> Elaborator<'a> { value_to_best_value.resize(num_values); Self { func, - domtree_children, + domtree, loop_analysis, value_to_elaborated_value: ScopedHashMap::with_capacity(num_values), value_to_best_value, @@ -553,9 +553,7 @@ impl<'a> Elaborator<'a> { let data = &self.loop_stack[loop_hoist_level]; // `data.hoist_block` should dominate `before`'s block. let before_block = self.func.layout.inst_block(before).unwrap(); - debug_assert!(self - .domtree_children - .dominates(data.hoist_block, before_block)); + debug_assert!(self.domtree.dominates(data.hoist_block, before_block)); // Determine the instruction at which we // insert in `data.hoist_block`. let before = self.func.layout.last_inst(data.hoist_block).unwrap(); @@ -801,7 +799,7 @@ impl<'a> Elaborator<'a> { self.stats.elaborate_func += 1; self.stats.elaborate_func_pre_insts += self.func.dfg.num_insts() as u64; self.compute_best_values(); - self.elaborate_domtree(&self.domtree_children); + self.elaborate_domtree(&self.domtree); self.stats.elaborate_func_post_insts += self.func.dfg.num_insts() as u64; } }