From 9d0d6b5a1b909355c0d1906f11bfa6bae49482e2 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Tue, 12 Sep 2017 15:38:30 -0700 Subject: [PATCH] Move `verify` calls out of Context. Also, move flowgraph() calls out of filetest and into the passes that need them so that filetest doesn't have embedded knowledge of these dependencies. This resolves a TODO about the way Context was running the verifier, and it makes the Context functions and the filetest runners more transparent. This also fixes simple_gvn to use the existing dominator tree rather than computing its own. --- cranelift/src/filetest/licm.rs | 6 +++--- cranelift/src/filetest/simple_gvn.rs | 6 +++--- cranelift/src/wasm.rs | 17 ++++------------- lib/cretonne/src/context.rs | 12 ++++-------- lib/cretonne/src/dominator_tree.rs | 3 +-- lib/cretonne/src/flowgraph.rs | 17 +++++++++++++++++ lib/cretonne/src/licm.rs | 1 + lib/cretonne/src/simple_gvn.rs | 7 ++++--- 8 files changed, 37 insertions(+), 32 deletions(-) diff --git a/cranelift/src/filetest/licm.rs b/cranelift/src/filetest/licm.rs index db817530a9..79b785f8c3 100644 --- a/cranelift/src/filetest/licm.rs +++ b/cranelift/src/filetest/licm.rs @@ -38,9 +38,9 @@ impl SubTest for TestLICM { let mut comp_ctx = cretonne::Context::new(); comp_ctx.func = func.into_owned(); - comp_ctx.flowgraph(); - comp_ctx.licm().map_err(|e| { - pretty_error(&comp_ctx.func, context.isa, e) + comp_ctx.licm(); + comp_ctx.verify(context.isa).map_err(|e| { + pretty_error(&comp_ctx.func, context.isa, Into::into(e)) })?; let mut text = String::new(); diff --git a/cranelift/src/filetest/simple_gvn.rs b/cranelift/src/filetest/simple_gvn.rs index e8eb377b5e..1b04cd67b9 100644 --- a/cranelift/src/filetest/simple_gvn.rs +++ b/cranelift/src/filetest/simple_gvn.rs @@ -38,9 +38,9 @@ impl SubTest for TestSimpleGVN { let mut comp_ctx = cretonne::Context::new(); comp_ctx.func = func.into_owned(); - comp_ctx.flowgraph(); - comp_ctx.simple_gvn().map_err(|e| { - pretty_error(&comp_ctx.func, context.isa, e) + comp_ctx.simple_gvn(); + comp_ctx.verify(context.isa).map_err(|e| { + pretty_error(&comp_ctx.func, context.isa, Into::into(e)) })?; let mut text = String::new(); diff --git a/cranelift/src/wasm.rs b/cranelift/src/wasm.rs index e0b4d17772..350a15d51a 100644 --- a/cranelift/src/wasm.rs +++ b/cranelift/src/wasm.rs @@ -10,7 +10,6 @@ use cretonne::loop_analysis::LoopAnalysis; use cretonne::flowgraph::ControlFlowGraph; use cretonne::dominator_tree::DominatorTree; use cretonne::Context; -use cretonne::result::CtonError; use cretonne::verifier; use cretonne::settings::{self, Configurable}; use std::fs::File; @@ -171,18 +170,10 @@ fn handle_module( context.loop_analysis = loop_analysis; verifier::verify_context(&context.func, &context.cfg, &context.domtree, None) .map_err(|err| pretty_verifier_error(&context.func, None, err))?; - context.licm().map_err(|error| match error { - CtonError::Verifier(err) => pretty_verifier_error(&context.func, None, err), - CtonError::InvalidInput | - CtonError::ImplLimitExceeded | - CtonError::CodeTooLarge => String::from(error.description()), - })?; - context.simple_gvn().map_err(|error| match error { - CtonError::Verifier(err) => pretty_verifier_error(&context.func, None, err), - CtonError::InvalidInput | - CtonError::ImplLimitExceeded | - CtonError::CodeTooLarge => String::from(error.description()), - })?; + context.licm(); + verifier::verify_context(&context.func, &context.cfg, &context.domtree, None) + .map_err(|err| pretty_verifier_error(&context.func, None, err))?; + context.simple_gvn(); verifier::verify_context(&context.func, &context.cfg, &context.domtree, 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 45914d8069..c7347a1079 100644 --- a/lib/cretonne/src/context.rs +++ b/lib/cretonne/src/context.rs @@ -116,22 +116,18 @@ impl Context { } /// Perform simple GVN on the function. - pub fn simple_gvn(&mut self) -> CtonResult { - do_simple_gvn(&mut self.func, &mut self.cfg); - // TODO: Factor things such that we can get a Flags and test - // enable_verifier(). - self.verify(None).map_err(Into::into) + pub fn simple_gvn(&mut self) { + do_simple_gvn(&mut self.func, &mut self.cfg, &mut self.domtree) } /// Perform LICM on the function. - pub fn licm(&mut self) -> CtonResult { + pub fn licm(&mut self) { do_licm( &mut self.func, &mut self.cfg, &mut self.domtree, &mut self.loop_analysis, - ); - self.verify(None).map_err(Into::into) + ) } /// Run the register allocator. diff --git a/lib/cretonne/src/dominator_tree.rs b/lib/cretonne/src/dominator_tree.rs index 18fd9e91b2..143df57db6 100644 --- a/lib/cretonne/src/dominator_tree.rs +++ b/lib/cretonne/src/dominator_tree.rs @@ -231,8 +231,7 @@ impl DominatorTree { /// /// Note that this doesn't perform any kind of validity checks. It simply checks if the /// `compute()` method has been called since the last `clear()`. It does not check that the - /// dominator tree is consistent - /// with the CFG> + /// dominator tree is consistent with the CFG. pub fn is_valid(&self) -> bool { !self.nodes.is_empty() } diff --git a/lib/cretonne/src/flowgraph.rs b/lib/cretonne/src/flowgraph.rs index 504d90fb9d..1517ae5ee3 100644 --- a/lib/cretonne/src/flowgraph.rs +++ b/lib/cretonne/src/flowgraph.rs @@ -131,6 +131,23 @@ impl ControlFlowGraph { pub fn get_successors(&self, ebb: Ebb) -> &[Ebb] { &self.data[ebb].successors } + + /// Check if the CFG is in a valid state. + /// + /// Note that this doesn't perform any kind of validity checks. It simply checks if the + /// `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) + } + } } #[cfg(test)] diff --git a/lib/cretonne/src/licm.rs b/lib/cretonne/src/licm.rs index 41aa498c5c..225cd4d524 100644 --- a/lib/cretonne/src/licm.rs +++ b/lib/cretonne/src/licm.rs @@ -16,6 +16,7 @@ pub fn do_licm( domtree: &mut DominatorTree, loop_analysis: &mut LoopAnalysis, ) { + cfg.ensure(func); domtree.ensure(func, cfg); loop_analysis.compute(func, cfg, domtree); for lp in loop_analysis.loops() { diff --git a/lib/cretonne/src/simple_gvn.rs b/lib/cretonne/src/simple_gvn.rs index 308d23e083..d3d8391991 100644 --- a/lib/cretonne/src/simple_gvn.rs +++ b/lib/cretonne/src/simple_gvn.rs @@ -13,10 +13,11 @@ fn trivially_unsafe_for_gvn(opcode: Opcode) -> bool { /// Perform simple GVN on `func`. /// -pub fn do_simple_gvn(func: &mut Function, cfg: &mut ControlFlowGraph) { - let mut visible_values: HashMap<(InstructionData, Type), Inst> = HashMap::new(); +pub fn do_simple_gvn(func: &mut Function, cfg: &mut ControlFlowGraph, domtree: &mut DominatorTree) { + cfg.ensure(func); + domtree.ensure(func, cfg); - let domtree = DominatorTree::with_function(func, cfg); + let mut visible_values: HashMap<(InstructionData, Type), Inst> = HashMap::new(); // Visit EBBs in a reverse post-order. let mut pos = Cursor::new(&mut func.layout);