Make passes assert their dependencies consistently. (#156)

* Make passes assert their dependencies consistently.

This avoids ambiguity about whose responsibility it is to run
to compute cfg, domtree, and loop_analysis data.

* Reset the `valid` flag in DominatorTree's `clear()`.

* Remove the redundant assert from DominatorTree::with_function.

* Remove the message strings from obvious asserts.

This avoids having them spill out into multiple lines.

* Refactor calls to `compute` on `Context` objects into helper functions.

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)
         })?;

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))

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)
         })?;

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))

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)

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<CodeOffset, CtonError> {
-        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.compute_cfg();
-        self.domtree.compute(&self.func, &self.cfg);
+        self.compute_domtree()
     }
 
     /// Perform simple GVN on the function.

lib/cretonne/src/dominator_tree.rs

@@ -37,6 +37,8 @@ pub struct DominatorTree {
 
     // Scratch memory used by `compute_postorder()`.
     stack: Vec<Ebb>,
+
+    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()
+        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) {
-        if !self.is_valid() {
-            self.compute(func, cfg)
-        }
     }
 
     /// 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(), &[]);

lib/cretonne/src/flowgraph.rs

@@ -47,6 +47,7 @@ pub struct CFGNode {
 pub struct ControlFlowGraph {
     entry_block: Option<Ebb>,
     data: EntityMap<Ebb, CFGNode>,
+    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()
+        self.valid
-    }
-
-    /// 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)
-        }
     }
 }
 

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());

lib/cretonne/src/licm.rs

@@ -16,9 +16,10 @@ pub fn do_licm(
     domtree: &mut DominatorTree,
     loop_analysis: &mut LoopAnalysis,
 ) {
-    cfg.ensure(func);
+    debug_assert!(cfg.is_valid());
-    domtree.ensure(func, cfg);
+    debug_assert!(domtree.is_valid());
-    loop_analysis.ensure(func, cfg, domtree);
+    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

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<Loop, LoopData>,
     ebb_loop_map: EntityMap<Ebb, PackedOption<Loop>>,
+    valid: bool,
 }
 
 struct LoopData {
@@ -115,12 +115,13 @@ impl LoopAnalysis {
         self.valid
     }
 
-    /// Conveneince function to call `compute` if `compute` hasn't been called
+    /// Clear all the data structures contanted in the loop analysis. This will leave the
-    /// since the last `clear()`.
+    /// analysis in a similar state to a context returned by `new()` except that allocated
-    pub fn ensure(&mut self, func: &Function, cfg: &ControlFlowGraph, domtree: &DominatorTree) {
+    /// memory be retained.
-        if !self.is_valid() {
+    pub fn clear(&mut self) {
-            self.compute(func, cfg, domtree)
+        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

lib/cretonne/src/regalloc/context.rs

@@ -60,8 +60,7 @@ impl Context {
         cfg: &ControlFlowGraph,
         domtree: &mut DominatorTree,
     ) -> CtonResult {
-        // Ensure that a valid domtree exists.
+        debug_assert!(domtree.is_valid());
-        domtree.ensure(func, cfg);
 
         // `Liveness` and `Coloring` are self-clearing.
         self.virtregs.clear();

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);
+    debug_assert!(cfg.is_valid());
-    domtree.ensure(func, cfg);
+    debug_assert!(domtree.is_valid());
 
     let mut visible_values: HashMap<(InstructionData, Type), Inst> = HashMap::new();
 

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)?;
     }

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();
     }
 }