From 373020e5cf62dc91be83a17a49cd0e6f3696982c Mon Sep 17 00:00:00 2001 From: badumbatish Date: Sat, 7 Sep 2024 12:16:44 -0700 Subject: [PATCH] Attempt to fix #7954 Changes to be committed: modified: cranelift/codegen/src/alias_analysis.rs: Use dominator tree modified: cranelift/codegen/src/context.rs: Use dominator tree modified: cranelift/codegen/src/dominator_tree.rs: Fix dominates and computes and modify logic of domination --- cranelift/codegen/src/alias_analysis.rs | 8 +++--- cranelift/codegen/src/context.rs | 15 ++++++++--- cranelift/codegen/src/dominator_tree.rs | 36 ++++++++++++++++++++++++- 3 files changed, 50 insertions(+), 9 deletions(-) diff --git a/cranelift/codegen/src/alias_analysis.rs b/cranelift/codegen/src/alias_analysis.rs index 6a6e9f2748ba..ae95849b73e9 100644 --- a/cranelift/codegen/src/alias_analysis.rs +++ b/cranelift/codegen/src/alias_analysis.rs @@ -63,7 +63,7 @@ use crate::{ cursor::{Cursor, FuncCursor}, - dominator_tree::DominatorTree, + dominator_tree::DominatorTreePreorder, inst_predicates::{ has_memory_fence_semantics, inst_addr_offset_type, inst_store_data, visit_block_succs, }, @@ -176,7 +176,7 @@ struct MemoryLoc { /// An alias-analysis pass. pub struct AliasAnalysis<'a> { /// The domtree for the function. - domtree: &'a DominatorTree, + domtree: &'a DominatorTreePreorder, /// Input state to a basic block. block_input: FxHashMap, @@ -191,7 +191,7 @@ pub struct AliasAnalysis<'a> { impl<'a> AliasAnalysis<'a> { /// Perform an alias analysis pass. - pub fn new(func: &Function, domtree: &'a DominatorTree) -> AliasAnalysis<'a> { + pub fn new(func: &Function, domtree: &'a DominatorTreePreorder) -> AliasAnalysis<'a> { trace!("alias analysis: input is:\n{:?}", func); let mut analysis = AliasAnalysis { domtree, @@ -333,7 +333,7 @@ impl<'a> AliasAnalysis<'a> { value.index(), def_inst.index() ); - if self.domtree.dominates(def_inst, inst, &func.layout) { + if self.domtree.dominates_inst(def_inst, inst, &func.layout) { trace!( " -> dominates; value equiv from v{} to v{} inserted", load_result.index(), diff --git a/cranelift/codegen/src/context.rs b/cranelift/codegen/src/context.rs index c36459ec587f..b8679596eff2 100644 --- a/cranelift/codegen/src/context.rs +++ b/cranelift/codegen/src/context.rs @@ -10,7 +10,7 @@ //! single ISA instance. use crate::alias_analysis::AliasAnalysis; -use crate::dominator_tree::DominatorTree; +use crate::dominator_tree::{DominatorTree, DominatorTreePreorder}; use crate::egraph::EgraphPass; use crate::flowgraph::ControlFlowGraph; use crate::ir::Function; @@ -46,6 +46,9 @@ pub struct Context { /// Dominator tree for `func`. pub domtree: DominatorTree, + /// Domniator tree preorder, be on look out to merge with domtree, although unlikely + pub domtree_preorder: DominatorTreePreorder, + /// Loop analysis of `func`. pub loop_analysis: LoopAnalysis, @@ -74,6 +77,7 @@ impl Context { func, cfg: ControlFlowGraph::new(), domtree: DominatorTree::new(), + domtree_preorder: DominatorTreePreorder::new(), loop_analysis: LoopAnalysis::new(), compiled_code: None, want_disasm: false, @@ -300,7 +304,9 @@ impl Context { /// Compute dominator tree. pub fn compute_domtree(&mut self) { - self.domtree.compute(&self.func, &self.cfg) + self.domtree.compute(&self.func, &self.cfg); + self.domtree_preorder + .compute(&self.domtree, &self.func.layout); } /// Compute the loop analysis. @@ -330,7 +336,7 @@ impl Context { /// by a store instruction to the same instruction (so-called /// "store-to-load forwarding"). pub fn replace_redundant_loads(&mut self) -> CodegenResult<()> { - let mut analysis = AliasAnalysis::new(&self.func, &self.domtree); + let mut analysis = AliasAnalysis::new(&self.func, &self.domtree_preorder); analysis.compute_and_update_aliases(&mut self.func); Ok(()) } @@ -362,7 +368,8 @@ impl Context { ); let fisa = fisa.into(); self.compute_loop_analysis(); - let mut alias_analysis = AliasAnalysis::new(&self.func, &self.domtree); + let mut alias_analysis = AliasAnalysis::new(&self.func, &self.domtree_preorder); + let mut pass = EgraphPass::new( &mut self.func, &self.domtree, diff --git a/cranelift/codegen/src/dominator_tree.rs b/cranelift/codegen/src/dominator_tree.rs index 12288362b48b..fbd8d3fc2159 100644 --- a/cranelift/codegen/src/dominator_tree.rs +++ b/cranelift/codegen/src/dominator_tree.rs @@ -482,7 +482,41 @@ impl DominatorTreePreorder { pub fn dominates(&self, a: Block, b: Block) -> bool { let na = &self.nodes[a]; let nb = &self.nodes[b]; - na.pre_number <= nb.pre_number && na.pre_max >= nb.pre_max + // debug_assert!(na.pre_number != 0); + // debug_assert!(nb.pre_number != 0); + if a == b { + // check first if they're the same block + // if they are then they dominate themselves + true + } else if nb.pre_number == 0 { + // if the supposedly dominated is not reachable (pre_number = 0), is it + // really being dominated though ? + false + } else { + na.pre_number <= nb.pre_number && na.pre_max >= nb.pre_max + } + } + + /// Checks if one instruction dominates another. + /// + /// Instruction a dominates instruction b if either of these conditions are true: + /// - a and b are in the same block, and a is not after b. + /// - The block containing a dominates the block containing b. + /// + /// This helper function is placed next to `fn dominates` for spatial locality of usages + pub fn dominates_inst(&self, a: Inst, b: Inst, layout: &Layout) -> bool { + match (layout.inst_block(a), layout.inst_block(b)) { + (Some(block_a), Some(block_b)) => { + if block_a == block_b { + // Case 1 + layout.pp_cmp(a, b) != Ordering::Greater + } else { + // Case 2 + self.dominates(block_a, block_b) + } + } + _ => false, + } } /// Compare two blocks according to the dominator pre-order.