diff --git a/cranelift/src/filetest/legalizer.rs b/cranelift/src/filetest/legalizer.rs index 08155289ce..e79aada799 100644 --- a/cranelift/src/filetest/legalizer.rs +++ b/cranelift/src/filetest/legalizer.rs @@ -40,7 +40,7 @@ impl SubTest for TestLegalizer { comp_ctx.func = func.into_owned(); let isa = context.isa.expect("legalizer needs an ISA"); - comp_ctx.flowgraph(); + comp_ctx.compute_cfg(); comp_ctx.legalize(isa).map_err(|e| { pretty_error(&comp_ctx.func, context.isa, e) })?; diff --git a/cranelift/src/filetest/licm.rs b/cranelift/src/filetest/licm.rs index 79b785f8c3..5806776e84 100644 --- a/cranelift/src/filetest/licm.rs +++ b/cranelift/src/filetest/licm.rs @@ -38,6 +38,8 @@ impl SubTest for TestLICM { let mut comp_ctx = cretonne::Context::new(); comp_ctx.func = func.into_owned(); + comp_ctx.flowgraph(); + comp_ctx.compute_loop_analysis(); comp_ctx.licm(); comp_ctx.verify(context.isa).map_err(|e| { pretty_error(&comp_ctx.func, context.isa, Into::into(e)) diff --git a/cranelift/src/filetest/regalloc.rs b/cranelift/src/filetest/regalloc.rs index 3974854416..48160796a8 100644 --- a/cranelift/src/filetest/regalloc.rs +++ b/cranelift/src/filetest/regalloc.rs @@ -44,11 +44,12 @@ impl SubTest for TestRegalloc { let mut comp_ctx = cretonne::Context::new(); comp_ctx.func = func.into_owned(); - comp_ctx.flowgraph(); + comp_ctx.compute_cfg(); // TODO: Should we have an option to skip legalization? comp_ctx.legalize(isa).map_err(|e| { pretty_error(&comp_ctx.func, context.isa, e) })?; + comp_ctx.compute_domtree(); comp_ctx.regalloc(isa).map_err(|e| { pretty_error(&comp_ctx.func, context.isa, e) })?; diff --git a/cranelift/src/filetest/simple_gvn.rs b/cranelift/src/filetest/simple_gvn.rs index 1b04cd67b9..d25618eb1d 100644 --- a/cranelift/src/filetest/simple_gvn.rs +++ b/cranelift/src/filetest/simple_gvn.rs @@ -38,6 +38,7 @@ impl SubTest for TestSimpleGVN { let mut comp_ctx = cretonne::Context::new(); comp_ctx.func = func.into_owned(); + comp_ctx.flowgraph(); comp_ctx.simple_gvn(); comp_ctx.verify(context.isa).map_err(|e| { pretty_error(&comp_ctx.func, context.isa, Into::into(e)) diff --git a/cranelift/src/wasm.rs b/cranelift/src/wasm.rs index 6b22d66e31..73d27dac91 100644 --- a/cranelift/src/wasm.rs +++ b/cranelift/src/wasm.rs @@ -158,6 +158,8 @@ fn handle_module( context.verify(None).map_err(|err| { pretty_verifier_error(&context.func, None, err) })?; + context.flowgraph(); + context.compute_loop_analysis(); context.licm(); context.verify(None).map_err(|err| { pretty_verifier_error(&context.func, None, err) diff --git a/lib/cretonne/src/context.rs b/lib/cretonne/src/context.rs index c7347a1079..398fc33c71 100644 --- a/lib/cretonne/src/context.rs +++ b/lib/cretonne/src/context.rs @@ -63,10 +63,11 @@ impl Context { /// /// Returns the size of the function's code. pub fn compile(&mut self, isa: &TargetIsa) -> Result { - self.cfg.compute(&self.func); self.verify_if(isa)?; + self.compute_cfg(); self.legalize(isa)?; + self.compute_domtree(); self.regalloc(isa)?; self.prologue_epilogue(isa)?; self.relax_branches(isa) @@ -103,16 +104,37 @@ impl Context { /// Run the legalizer for `isa` on the function. pub fn legalize(&mut self, isa: &TargetIsa) -> CtonResult { - // Legalization invalidates the domtree by mutating the CFG. + // Legalization invalidates the domtree and loop_analysis by mutating the CFG. + // TODO: Avoid doing this when legalization doesn't actually mutate the CFG. self.domtree.clear(); + self.loop_analysis.clear(); legalize_function(&mut self.func, &mut self.cfg, isa); self.verify_if(isa) } - /// Recompute the control flow graph and dominator tree. + /// Compute the control flow graph. + pub fn compute_cfg(&mut self) { + self.cfg.compute(&self.func) + } + + /// Compute dominator tree. + pub fn compute_domtree(&mut self) { + self.domtree.compute(&self.func, &self.cfg) + } + + /// Compute the loop analysis. + pub fn compute_loop_analysis(&mut self) { + self.loop_analysis.compute( + &self.func, + &self.cfg, + &self.domtree, + ) + } + + /// Compute the control flow graph and dominator tree. pub fn flowgraph(&mut self) { - self.cfg.compute(&self.func); - self.domtree.compute(&self.func, &self.cfg); + self.compute_cfg(); + self.compute_domtree() } /// Perform simple GVN on the function. diff --git a/lib/cretonne/src/dominator_tree.rs b/lib/cretonne/src/dominator_tree.rs index 143df57db6..faf369fcf6 100644 --- a/lib/cretonne/src/dominator_tree.rs +++ b/lib/cretonne/src/dominator_tree.rs @@ -37,6 +37,8 @@ pub struct DominatorTree { // Scratch memory used by `compute_postorder()`. stack: Vec, + + valid: bool, } /// Methods for querying the dominator tree. @@ -51,6 +53,7 @@ impl DominatorTree { /// Note that this post-order is not updated automatically when the CFG is modified. It is /// computed from scratch and cached by `compute()`. pub fn cfg_postorder(&self) -> &[Ebb] { + debug_assert!(self.is_valid()); &self.postorder } @@ -203,6 +206,7 @@ impl DominatorTree { nodes: EntityMap::new(), postorder: Vec::new(), stack: Vec::new(), + valid: false, } } @@ -215,8 +219,10 @@ impl DominatorTree { /// Reset and compute a CFG post-order and dominator tree. pub fn compute(&mut self, func: &Function, cfg: &ControlFlowGraph) { + debug_assert!(cfg.is_valid()); self.compute_postorder(func, cfg); self.compute_domtree(func, cfg); + self.valid = true; } /// Clear the data structures used to represent the dominator tree. This will leave the tree in @@ -225,6 +231,7 @@ impl DominatorTree { self.nodes.clear(); self.postorder.clear(); assert!(self.stack.is_empty()); + self.valid = false; } /// Check if the dominator tree is in a valid state. @@ -233,15 +240,7 @@ impl DominatorTree { /// `compute()` method has been called since the last `clear()`. It does not check that the /// dominator tree is consistent with the CFG. pub fn is_valid(&self) -> bool { - !self.nodes.is_empty() - } - - /// Conveneince function to call `compute` if `compute` hasn't been called - /// since the last `clear()`. - pub fn ensure(&mut self, func: &Function, cfg: &ControlFlowGraph) { - if !self.is_valid() { - self.compute(func, cfg) - } + self.valid } /// Reset all internal data structures and compute a post-order for `cfg`. @@ -437,6 +436,7 @@ mod test { fn empty() { let func = Function::new(); let cfg = ControlFlowGraph::with_function(&func); + debug_assert!(cfg.is_valid()); let dtree = DominatorTree::with_function(&func, &cfg); assert_eq!(0, dtree.nodes.keys().count()); assert_eq!(dtree.cfg_postorder(), &[]); diff --git a/lib/cretonne/src/flowgraph.rs b/lib/cretonne/src/flowgraph.rs index 1517ae5ee3..d246a1fa3a 100644 --- a/lib/cretonne/src/flowgraph.rs +++ b/lib/cretonne/src/flowgraph.rs @@ -47,6 +47,7 @@ pub struct CFGNode { pub struct ControlFlowGraph { entry_block: Option, data: EntityMap, + valid: bool, } impl ControlFlowGraph { @@ -55,6 +56,7 @@ impl ControlFlowGraph { ControlFlowGraph { entry_block: None, data: EntityMap::new(), + valid: false, } } @@ -76,6 +78,8 @@ impl ControlFlowGraph { for ebb in &func.layout { self.compute_ebb(func, ebb); } + + self.valid = true; } fn compute_ebb(&mut self, func: &Function, ebb: Ebb) { @@ -113,6 +117,7 @@ impl ControlFlowGraph { /// more expensive `compute`, and should be used when we know we don't need to recompute the CFG /// from scratch, but rather that our changes have been restricted to specific EBBs. pub fn recompute_ebb(&mut self, func: &Function, ebb: Ebb) { + debug_assert!(self.is_valid()); self.invalidate_ebb_successors(ebb); self.compute_ebb(func, ebb); } @@ -124,11 +129,13 @@ impl ControlFlowGraph { /// Get the CFG predecessor basic blocks to `ebb`. pub fn get_predecessors(&self, ebb: Ebb) -> &[BasicBlock] { + debug_assert!(self.is_valid()); &self.data[ebb].predecessors } /// Get the CFG successors to `ebb`. pub fn get_successors(&self, ebb: Ebb) -> &[Ebb] { + debug_assert!(self.is_valid()); &self.data[ebb].successors } @@ -138,15 +145,7 @@ impl ControlFlowGraph { /// `compute()` method has been called since the last `clear()`. It does not check that the /// CFG is consistent with the function. pub fn is_valid(&self) -> bool { - self.entry_block.is_some() - } - - /// Conveneince function to call `compute` if `compute` hasn't been called - /// since the last `clear()`. - pub fn ensure(&mut self, func: &Function) { - if !self.is_valid() { - self.compute(func) - } + self.valid } } diff --git a/lib/cretonne/src/legalizer/mod.rs b/lib/cretonne/src/legalizer/mod.rs index 179c0d3884..0367854681 100644 --- a/lib/cretonne/src/legalizer/mod.rs +++ b/lib/cretonne/src/legalizer/mod.rs @@ -33,6 +33,8 @@ use self::heap::expand_heap_addr; /// - Fill out `func.encodings`. /// pub fn legalize_function(func: &mut ir::Function, cfg: &mut ControlFlowGraph, isa: &TargetIsa) { + debug_assert!(cfg.is_valid()); + boundary::legalize_signatures(func, isa); func.encodings.resize(func.dfg.num_insts()); diff --git a/lib/cretonne/src/licm.rs b/lib/cretonne/src/licm.rs index 107f3f7766..58ea3733e9 100644 --- a/lib/cretonne/src/licm.rs +++ b/lib/cretonne/src/licm.rs @@ -16,9 +16,10 @@ pub fn do_licm( domtree: &mut DominatorTree, loop_analysis: &mut LoopAnalysis, ) { - cfg.ensure(func); - domtree.ensure(func, cfg); - loop_analysis.ensure(func, cfg, domtree); + debug_assert!(cfg.is_valid()); + debug_assert!(domtree.is_valid()); + debug_assert!(loop_analysis.is_valid()); + for lp in loop_analysis.loops() { // For each loop that we want to optimize we determine the set of loop-invariant // instructions diff --git a/lib/cretonne/src/loop_analysis.rs b/lib/cretonne/src/loop_analysis.rs index 9d5e466c16..4deedbd568 100644 --- a/lib/cretonne/src/loop_analysis.rs +++ b/lib/cretonne/src/loop_analysis.rs @@ -18,9 +18,9 @@ entity_impl!(Loop, "loop"); /// Loops are referenced by the Loop object, and for each loop you can access its header EBB, /// its eventual parent in the loop tree and all the EBB belonging to the loop. pub struct LoopAnalysis { - valid: bool, loops: PrimaryMap, ebb_loop_map: EntityMap>, + valid: bool, } struct LoopData { @@ -115,12 +115,13 @@ impl LoopAnalysis { self.valid } - /// Conveneince function to call `compute` if `compute` hasn't been called - /// since the last `clear()`. - pub fn ensure(&mut self, func: &Function, cfg: &ControlFlowGraph, domtree: &DominatorTree) { - if !self.is_valid() { - self.compute(func, cfg, domtree) - } + /// Clear all the data structures contanted in the loop analysis. This will leave the + /// analysis in a similar state to a context returned by `new()` except that allocated + /// memory be retained. + pub fn clear(&mut self) { + self.loops.clear(); + self.ebb_loop_map.clear(); + self.valid = false; } // Traverses the CFG in reverse postorder and create a loop object for every EBB having a diff --git a/lib/cretonne/src/regalloc/context.rs b/lib/cretonne/src/regalloc/context.rs index 92be408919..1cb7d3f129 100644 --- a/lib/cretonne/src/regalloc/context.rs +++ b/lib/cretonne/src/regalloc/context.rs @@ -60,8 +60,7 @@ impl Context { cfg: &ControlFlowGraph, domtree: &mut DominatorTree, ) -> CtonResult { - // Ensure that a valid domtree exists. - domtree.ensure(func, cfg); + debug_assert!(domtree.is_valid()); // `Liveness` and `Coloring` are self-clearing. self.virtregs.clear(); diff --git a/lib/cretonne/src/simple_gvn.rs b/lib/cretonne/src/simple_gvn.rs index d3d8391991..40d0ecf527 100644 --- a/lib/cretonne/src/simple_gvn.rs +++ b/lib/cretonne/src/simple_gvn.rs @@ -14,8 +14,8 @@ fn trivially_unsafe_for_gvn(opcode: Opcode) -> bool { /// Perform simple GVN on `func`. /// pub fn do_simple_gvn(func: &mut Function, cfg: &mut ControlFlowGraph, domtree: &mut DominatorTree) { - cfg.ensure(func); - domtree.ensure(func, cfg); + debug_assert!(cfg.is_valid()); + debug_assert!(domtree.is_valid()); let mut visible_values: HashMap<(InstructionData, Type), Inst> = HashMap::new(); diff --git a/lib/cretonne/src/verifier/mod.rs b/lib/cretonne/src/verifier/mod.rs index f3952606ca..118192786b 100644 --- a/lib/cretonne/src/verifier/mod.rs +++ b/lib/cretonne/src/verifier/mod.rs @@ -134,7 +134,9 @@ pub fn verify_context( isa: Option<&TargetIsa>, ) -> Result { let verifier = Verifier::new(func, isa); - verifier.cfg_integrity(cfg)?; + if cfg.is_valid() { + verifier.cfg_integrity(cfg)?; + } if domtree.is_valid() { verifier.domtree_integrity(domtree)?; } diff --git a/lib/wasm/src/func_translator.rs b/lib/wasm/src/func_translator.rs index 3383b0caf1..acbb43b36b 100644 --- a/lib/wasm/src/func_translator.rs +++ b/lib/wasm/src/func_translator.rs @@ -250,7 +250,6 @@ mod tests { trans.translate(&BODY, &mut ctx.func, &mut runtime).unwrap(); dbg!("{}", ctx.func.display(None)); - ctx.flowgraph(); ctx.verify(None).unwrap(); } @@ -284,7 +283,6 @@ mod tests { trans.translate(&BODY, &mut ctx.func, &mut runtime).unwrap(); dbg!("{}", ctx.func.display(None)); - ctx.flowgraph(); ctx.verify(None).unwrap(); } @@ -324,7 +322,6 @@ mod tests { trans.translate(&BODY, &mut ctx.func, &mut runtime).unwrap(); dbg!("{}", ctx.func.display(None)); - ctx.flowgraph(); ctx.verify(None).unwrap(); } }