Always require a Flags reference for verifying functions.

Add a settings::FlagsOrIsa struct which represents a flags reference and
optionally the ISA it belongs to. Use this for passing flags/isa
information to the verifier.

The verify_function() and verify_context() functions are now generic so
they accept either a &Flags or a &TargetISa argument.

Fix the return_at_end verifier tests which no longer require an ISA
specified. The signle "set return_at_end" flag setting now makes it to
the verifier even when no ISA is present to carry it.

cranelift/filetests/verifier/return_at_end.cton

@@ -1,10 +1,6 @@
 test verifier
 set return_at_end
 
-; The verifier doesn't have an API for verifying with flags without also
-; setting an ISA.
-isa riscv
-
 function %ok(i32) {
 ebb0(v0: i32):
     brnz v0, ebb1

cranelift/src/filetest/licm.rs

@@ -41,7 +41,7 @@ impl SubTest for TestLICM {
         comp_ctx.flowgraph();
         comp_ctx.compute_loop_analysis();
         comp_ctx.licm();
-        comp_ctx.verify(context.isa).map_err(|e| {
+        comp_ctx.verify(context.flags_or_isa()).map_err(|e| {
             pretty_error(&comp_ctx.func, context.isa, Into::into(e))
         })?;
 

cranelift/src/filetest/runone.rs

@@ -119,9 +119,11 @@ fn run_one_test<'a>(
 
     // Should we run the verifier before this test?
     if !context.verified && test.needs_verifier() {
-        verify_function(&func, isa).map_err(|e| {
+        verify_function(&func, context.flags_or_isa()).map_err(
-            pretty_verifier_error(&func, isa, e)
+            |e| {
-        })?;
+                pretty_verifier_error(&func, isa, e)
+            },
+        )?;
         context.verified = true;
     }
 

cranelift/src/filetest/simple_gvn.rs

@@ -40,7 +40,7 @@ impl SubTest for TestSimpleGVN {
 
         comp_ctx.flowgraph();
         comp_ctx.simple_gvn();
-        comp_ctx.verify(context.isa).map_err(|e| {
+        comp_ctx.verify(context.flags_or_isa()).map_err(|e| {
             pretty_error(&comp_ctx.func, context.isa, Into::into(e))
         })?;
 

cranelift/src/filetest/subtest.rs

@@ -4,7 +4,7 @@ use std::result;
 use std::borrow::Cow;
 use cretonne::ir::Function;
 use cretonne::isa::TargetIsa;
-use cretonne::settings::Flags;
+use cretonne::settings::{Flags, FlagsOrIsa};
 use cton_reader::{Details, Comment};
 use filecheck::{self, CheckerBuilder, Checker, Value as FCValue};
 
@@ -29,6 +29,16 @@ pub struct Context<'a> {
     pub isa: Option<&'a TargetIsa>,
 }
 
+impl<'a> Context<'a> {
+    /// Get a `FlagsOrIsa` object for passing to the verifier.
+    pub fn flags_or_isa(&self) -> FlagsOrIsa<'a> {
+        FlagsOrIsa {
+            flags: self.flags,
+            isa: self.isa,
+        }
+    }
+}
+
 /// Common interface for implementations of test commands.
 ///
 /// Each `.cton` test file may contain multiple test commands, each represented by a `SubTest`

cranelift/src/filetest/verifier.rs

@@ -51,7 +51,7 @@ impl SubTest for TestVerifier {
             }
         }
 
-        match verify_function(func, context.isa) {
+        match verify_function(func, context.flags_or_isa()) {
             Ok(_) => {
                 match expected {
                     None => Ok(()),

cranelift/src/wasm.rs

@@ -61,7 +61,6 @@ pub fn run(
         &mut flag_builder,
         &Location { line_number: 0 },
     ).map_err(|err| err.to_string())?;
-    let flags = settings::Flags::new(&flag_builder);
 
     let mut words = flag_isa.trim().split_whitespace();
     // Look for `isa foo`.
@@ -84,7 +83,6 @@ pub fn run(
             flag_check,
             path.to_path_buf(),
             name,
-            &flags,
             &*isa,
         )?;
     }
@@ -97,7 +95,6 @@ fn handle_module(
     flag_check: bool,
     path: PathBuf,
     name: String,
-    flags: &settings::Flags,
     isa: &TargetIsa,
 ) -> Result<(), String> {
     let mut terminal = term::stdout().unwrap();
@@ -144,7 +141,7 @@ fn handle_module(
             }
         }
     };
-    let mut dummy_runtime = DummyRuntime::with_flags(flags.clone());
+    let mut dummy_runtime = DummyRuntime::with_flags(isa.flags().clone());
     let translation = {
         let runtime: &mut WasmRuntime = &mut dummy_runtime;
         translate_module(&data, runtime)?
@@ -157,8 +154,8 @@ fn handle_module(
         vprint!(flag_verbose, "Checking...   ");
         terminal.reset().unwrap();
         for func in &translation.functions {
-            verifier::verify_function(func, Some(isa)).map_err(|err| {
+            verifier::verify_function(func, isa).map_err(|err| {
-                pretty_verifier_error(func, Some(isa), err)
+                pretty_verifier_error(func, None, err)
             })?;
         }
         terminal.fg(term::color::GREEN).unwrap();
@@ -172,18 +169,18 @@ fn handle_module(
         for func in &translation.functions {
             let mut context = Context::new();
             context.func = func.clone();
-            context.verify(Some(isa)).map_err(|err| {
+            context.verify(isa).map_err(|err| {
-                pretty_verifier_error(&context.func, Some(isa), err)
+                pretty_verifier_error(&context.func, None, err)
             })?;
             context.flowgraph();
             context.compute_loop_analysis();
             context.licm();
-            context.verify(Some(isa)).map_err(|err| {
+            context.verify(isa).map_err(|err| {
-                pretty_verifier_error(&context.func, Some(isa), err)
+                pretty_verifier_error(&context.func, None, err)
             })?;
             context.simple_gvn();
-            context.verify(Some(isa)).map_err(|err| {
+            context.verify(isa).map_err(|err| {
-                pretty_verifier_error(&context.func, Some(isa), err)
+                pretty_verifier_error(&context.func, None, err)
             })?;
         }
         terminal.fg(term::color::GREEN).unwrap();

lib/cretonne/src/context.rs

@@ -18,6 +18,7 @@ use isa::TargetIsa;
 use legalize_function;
 use regalloc;
 use result::{CtonError, CtonResult};
+use settings::FlagsOrIsa;
 use verifier;
 use simple_gvn::do_simple_gvn;
 use licm::do_licm;
@@ -86,17 +87,14 @@ impl Context {
     /// Run the verifier on the function.
     ///
     /// Also check that the dominator tree and control flow graph are consistent with the function.
-    ///
+    pub fn verify<'a, FOI: Into<FlagsOrIsa<'a>>>(&self, fisa: FOI) -> verifier::Result {
-    /// The `isa` argument is currently unused, but the verifier will soon be able to also
+        verifier::verify_context(&self.func, &self.cfg, &self.domtree, fisa)
-    /// check ISA-dependent constraints.
-    pub fn verify(&self, isa: Option<&TargetIsa>) -> verifier::Result {
-        verifier::verify_context(&self.func, &self.cfg, &self.domtree, isa)
     }
 
     /// Run the verifier only if the `enable_verifier` setting is true.
     pub fn verify_if(&self, isa: &TargetIsa) -> CtonResult {
         if isa.flags().enable_verifier() {
-            self.verify(Some(isa)).map_err(Into::into)
+            self.verify(isa).map_err(Into::into)
         } else {
             Ok(())
         }

lib/cretonne/src/dominator_tree.rs

@@ -427,9 +427,10 @@ impl DominatorTree {
 mod test {
     use cursor::{Cursor, FuncCursor};
     use flowgraph::ControlFlowGraph;
-    use ir::{Function, InstBuilder, types};
-    use super::*;
     use ir::types::*;
+    use ir::{Function, InstBuilder, types};
+    use settings;
+    use super::*;
     use verifier::verify_context;
 
     #[test]
@@ -608,6 +609,8 @@ mod test {
         dt.recompute_split_ebb(ebb3, ebb4, middle_jump_inst);
 
         cfg.compute(cur.func);
-        verify_context(cur.func, &cfg, &dt, None).unwrap();
+
+        let flags = settings::Flags::new(&settings::builder());
+        verify_context(cur.func, &cfg, &dt, &flags).unwrap();
     }
 }

lib/cretonne/src/regalloc/context.rs

@@ -87,7 +87,7 @@ impl Context {
         );
 
         if isa.flags().enable_verifier() {
-            verify_context(func, cfg, domtree, Some(isa))?;
+            verify_context(func, cfg, domtree, isa)?;
             verify_liveness(isa, func, cfg, &self.liveness)?;
             verify_cssa(func, cfg, domtree, &self.liveness, &self.virtregs)?;
         }
@@ -105,7 +105,7 @@ impl Context {
         );
 
         if isa.flags().enable_verifier() {
-            verify_context(func, cfg, domtree, Some(isa))?;
+            verify_context(func, cfg, domtree, isa)?;
             verify_liveness(isa, func, cfg, &self.liveness)?;
             verify_cssa(func, cfg, domtree, &self.liveness, &self.virtregs)?;
         }
@@ -121,7 +121,7 @@ impl Context {
         );
 
         if isa.flags().enable_verifier() {
-            verify_context(func, cfg, domtree, Some(isa))?;
+            verify_context(func, cfg, domtree, isa)?;
             verify_liveness(isa, func, cfg, &self.liveness)?;
             verify_cssa(func, cfg, domtree, &self.liveness, &self.virtregs)?;
         }
@@ -136,7 +136,7 @@ impl Context {
         );
 
         if isa.flags().enable_verifier() {
-            verify_context(func, cfg, domtree, Some(isa))?;
+            verify_context(func, cfg, domtree, isa)?;
             verify_liveness(isa, func, cfg, &self.liveness)?;
             verify_cssa(func, cfg, domtree, &self.liveness, &self.virtregs)?;
         }

lib/cretonne/src/settings.rs

@@ -20,11 +20,11 @@
 //! assert_eq!(f.opt_level(), settings::OptLevel::Fastest);
 //! ```
 
+use constant_hash::{probe, simple_hash};
+use isa::TargetIsa;
 use std::fmt;
 use std::result;
 
-use constant_hash::{probe, simple_hash};
-
 /// A string-based configurator for settings groups.
 ///
 /// The `Configurable` protocol allows settings to be modified by name before a finished `Flags`
@@ -303,6 +303,34 @@ pub mod detail {
 // with an impl for all of the settings defined in `meta/cretonne/settings.py`.
 include!(concat!(env!("OUT_DIR"), "/settings.rs"));
 
+/// Wrapper containing flags and optionally a `TargetIsa` trait object.
+///
+/// A few passes need to access the flags but only optionally a target ISA. The `FlagsOrIsa`
+/// wrapper can be used to pass either, and extract the flags so they are always accessible.
+#[derive(Clone, Copy)]
+pub struct FlagsOrIsa<'a> {
+    /// Flags are always present.
+    pub flags: &'a Flags,
+
+    /// The ISA may not be present.
+    pub isa: Option<&'a TargetIsa>,
+}
+
+impl<'a> From<&'a Flags> for FlagsOrIsa<'a> {
+    fn from(flags: &'a Flags) -> FlagsOrIsa {
+        FlagsOrIsa { flags, isa: None }
+    }
+}
+
+impl<'a> From<&'a TargetIsa> for FlagsOrIsa<'a> {
+    fn from(isa: &'a TargetIsa) -> FlagsOrIsa {
+        FlagsOrIsa {
+            flags: isa.flags(),
+            isa: Some(isa),
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::{builder, Flags};

lib/cretonne/src/verifier/mod.rs

@@ -66,6 +66,7 @@ use ir::instructions::{InstructionFormat, BranchInfo, ResolvedConstraint, CallIn
 use ir::{types, Function, ValueDef, Ebb, Inst, SigRef, FuncRef, ValueList, JumpTable, StackSlot,
          StackSlotKind, GlobalVar, Value, Type, Opcode, ValueLoc, ArgumentLoc};
 use isa::TargetIsa;
+use settings::{Flags, FlagsOrIsa};
 use std::error as std_error;
 use std::fmt::{self, Display, Formatter};
 use std::result;
@@ -121,19 +122,19 @@ impl std_error::Error for Error {
 pub type Result = result::Result<(), Error>;
 
 /// Verify `func`.
-pub fn verify_function(func: &Function, isa: Option<&TargetIsa>) -> Result {
+pub fn verify_function<'a, FOI: Into<FlagsOrIsa<'a>>>(func: &Function, fisa: FOI) -> Result {
-    Verifier::new(func, isa).run()
+    Verifier::new(func, fisa.into()).run()
 }
 
 /// Verify `func` after checking the integrity of associated context data structures `cfg` and
 /// `domtree`.
-pub fn verify_context(
+pub fn verify_context<'a, FOI: Into<FlagsOrIsa<'a>>>(
     func: &Function,
     cfg: &ControlFlowGraph,
     domtree: &DominatorTree,
-    isa: Option<&TargetIsa>,
+    fisa: FOI,
 ) -> Result {
-    let verifier = Verifier::new(func, isa);
+    let verifier = Verifier::new(func, fisa.into());
     if cfg.is_valid() {
         verifier.cfg_integrity(cfg)?;
     }
@@ -147,18 +148,20 @@ struct Verifier<'a> {
     func: &'a Function,
     cfg: ControlFlowGraph,
     domtree: DominatorTree,
+    flags: &'a Flags,
     isa: Option<&'a TargetIsa>,
 }
 
 impl<'a> Verifier<'a> {
-    pub fn new(func: &'a Function, isa: Option<&'a TargetIsa>) -> Verifier<'a> {
+    pub fn new(func: &'a Function, fisa: FlagsOrIsa<'a>) -> Verifier<'a> {
         let cfg = ControlFlowGraph::with_function(func);
         let domtree = DominatorTree::with_function(func, &cfg);
         Verifier {
             func,
             cfg,
             domtree,
-            isa,
+            flags: fisa.flags,
+            isa: fisa.isa,
         }
     }
 
@@ -971,7 +974,7 @@ impl<'a> Verifier<'a> {
             }
         }
 
-        if self.isa.map(|isa| isa.flags().return_at_end()) == Some(true) {
+        if self.flags.return_at_end() {
             self.verify_return_at_end()?;
         }
 
@@ -984,6 +987,7 @@ mod tests {
     use super::{Verifier, Error};
     use ir::Function;
     use ir::instructions::{InstructionData, Opcode};
+    use settings;
 
     macro_rules! assert_err_with_msg {
         ($e:expr, $msg:expr) => (
@@ -1001,7 +1005,8 @@ mod tests {
     #[test]
     fn empty() {
         let func = Function::new();
-        let verifier = Verifier::new(&func, None);
+        let flags = &settings::Flags::new(&settings::builder());
+        let verifier = Verifier::new(&func, flags.into());
         assert_eq!(verifier.run(), Ok(()));
     }
 
@@ -1014,7 +1019,8 @@ mod tests {
             InstructionData::Nullary { opcode: Opcode::Jump },
         );
         func.layout.append_inst(nullary_with_bad_opcode, ebb0);
-        let verifier = Verifier::new(&func, None);
+        let flags = &settings::Flags::new(&settings::builder());
+        let verifier = Verifier::new(&func, flags.into());
         assert_err_with_msg!(verifier.run(), "instruction format");
     }
 }

lib/frontend/src/frontend.rs

@@ -631,6 +631,7 @@ mod tests {
     use cretonne::ir::types::*;
     use frontend::{ILBuilder, FunctionBuilder};
     use cretonne::verifier::verify_function;
+    use cretonne::settings;
 
     use std::u32;
 
@@ -727,7 +728,8 @@ mod tests {
             builder.seal_block(block1);
         }
 
-        let res = verify_function(&func, None);
+        let flags = settings::Flags::new(&settings::builder());
+        let res = verify_function(&func, &flags);
         // println!("{}", func.display(None));
         match res {
             Ok(_) => {}

lib/frontend/src/lib.rs

@@ -38,6 +38,7 @@
 //! use cretonne::entity::EntityRef;
 //! use cretonne::ir::{FunctionName, CallConv, Function, Signature, ArgumentType, InstBuilder};
 //! use cretonne::ir::types::*;
+//! use cretonne::settings;
 //! use cton_frontend::{ILBuilder, FunctionBuilder};
 //! use cretonne::verifier::verify_function;
 //! use std::u32;
@@ -133,7 +134,8 @@
 //!         builder.seal_block(block1);
 //!     }
 //!
-//!     let res = verify_function(&func, None);
+//!     let flags = settings::Flags::new(&settings::builder());
+//!     let res = verify_function(&func, &flags);
 //!     println!("{}", func.display(None));
 //!     match res {
 //!         Ok(_) => {}

lib/frontend/src/ssa.rs

@@ -589,6 +589,7 @@ mod tests {
     use cretonne::ir::types::*;
     use cretonne::verify_function;
     use cretonne::ir::instructions::BranchInfo;
+    use cretonne::settings;
     use ssa::SSABuilder;
     use std::u32;
 
@@ -1194,7 +1195,8 @@ mod tests {
             cur.goto_bottom(ebb1);
             func.dfg.ins(cur).return_(&[])
         };
-        match verify_function(&func, None) {
+        let flags = settings::Flags::new(&settings::builder());
+        match verify_function(&func, &flags) {
             Ok(()) => {}
             Err(err) => panic!(err.message),
         }

lib/wasm/src/func_translator.rs

@@ -218,7 +218,7 @@ fn parse_function_body<FE: FuncEnvironment + ?Sized>(
 mod tests {
     use cretonne::{ir, Context};
     use cretonne::ir::types::I32;
-    use runtime::DummyRuntime;
+    use runtime::{DummyRuntime, FuncEnvironment};
     use super::FuncTranslator;
 
     #[test]
@@ -250,7 +250,7 @@ mod tests {
 
         trans.translate(&BODY, &mut ctx.func, &mut runtime).unwrap();
         dbg!("{}", ctx.func.display(None));
-        ctx.verify(None).unwrap();
+        ctx.verify(runtime.flags()).unwrap();
     }
 
     #[test]
@@ -283,7 +283,7 @@ mod tests {
 
         trans.translate(&BODY, &mut ctx.func, &mut runtime).unwrap();
         dbg!("{}", ctx.func.display(None));
-        ctx.verify(None).unwrap();
+        ctx.verify(runtime.flags()).unwrap();
     }
 
     #[test]
@@ -322,6 +322,6 @@ mod tests {
 
         trans.translate(&BODY, &mut ctx.func, &mut runtime).unwrap();
         dbg!("{}", ctx.func.display(None));
-        ctx.verify(None).unwrap();
+        ctx.verify(runtime.flags()).unwrap();
     }
 }

lib/wasm/tests/testsuite.rs

@@ -4,7 +4,6 @@ extern crate tempdir;
 
 use cton_wasm::{translate_module, DummyRuntime, WasmRuntime};
 use std::path::PathBuf;
-use std::borrow::Borrow;
 use std::fs::File;
 use std::error::Error;
 use std::io;
@@ -15,8 +14,8 @@ use std::process::Command;
 use std::fs;
 use cretonne::ir;
 use cretonne::ir::entities::AnyEntity;
-use cretonne::isa::{self, TargetIsa};
+use cretonne::isa::TargetIsa;
-use cretonne::settings::{self, Configurable};
+use cretonne::settings::{self, Configurable, Flags};
 use cretonne::verifier;
 use tempdir::TempDir;
 
@@ -27,9 +26,10 @@ fn testsuite() {
         .map(|r| r.unwrap())
         .collect();
     paths.sort_by_key(|dir| dir.path());
+    let flags = Flags::new(&settings::builder());
     for path in paths {
         let path = path.path();
-        handle_module(path, None);
+        handle_module(path, &flags);
     }
 }
 
@@ -37,19 +37,8 @@ fn testsuite() {
 fn return_at_end() {
     let mut flag_builder = settings::builder();
     flag_builder.enable("return_at_end").unwrap();
-    let flags = settings::Flags::new(&flag_builder);
+    let flags = Flags::new(&flag_builder);
-    // We don't care about the target itself here, so just pick one arbitrarily.
+    handle_module(PathBuf::from("../../wasmtests/return_at_end.wat"), &flags);
-    let isa = match isa::lookup("riscv") {
-        Err(_) => {
-            println!("riscv target not found; disabled test return_at_end.wat");
-            return;
-        }
-        Ok(isa_builder) => isa_builder.finish(flags),
-    };
-    handle_module(
-        PathBuf::from("../../wasmtests/return_at_end.wat"),
-        Some(isa.borrow()),
-    );
 }
 
 fn read_wasm_file(path: PathBuf) -> Result<Vec<u8>, io::Error> {
@@ -60,7 +49,7 @@ fn read_wasm_file(path: PathBuf) -> Result<Vec<u8>, io::Error> {
     Ok(buf)
 }
 
-fn handle_module(path: PathBuf, isa: Option<&TargetIsa>) {
+fn handle_module(path: PathBuf, flags: &Flags) {
     let data = match path.extension() {
         None => {
             panic!("the file extension is not wasm or wat");
@@ -105,17 +94,14 @@ fn handle_module(path: PathBuf, isa: Option<&TargetIsa>) {
             }
         }
     };
-    let mut dummy_runtime = match isa {
+    let mut dummy_runtime = DummyRuntime::with_flags(flags.clone());
-        Some(isa) => DummyRuntime::with_flags(isa.flags().clone()),
-        None => DummyRuntime::default(),
-    };
     let translation = {
         let runtime: &mut WasmRuntime = &mut dummy_runtime;
         translate_module(&data, runtime).unwrap()
     };
     for func in &translation.functions {
-        verifier::verify_function(func, isa)
+        verifier::verify_function(func, flags)
-            .map_err(|err| panic!(pretty_verifier_error(func, isa, err)))
+            .map_err(|err| panic!(pretty_verifier_error(func, None, err)))
             .unwrap();
     }
 }