Verify instruction encodings

filetests/isa/riscv/verify-encoding.cton

@@ -0,0 +1,21 @@
+test verifier
+isa riscv
+
+function RV32I(i32 link [%x1]) -> i32 link [%x1] {
+    fn0 = function foo()
+
+ebb0(v9999: i32):
+    ; iconst.i32 needs legalizing, so it should throw a
+    [R#0,-]         v1 = iconst.i32 1 ; error: Instruction failed to re-encode
+    return v9999
+}
+
+function RV32I(i32 link [%x1]) -> i32 link [%x1] {
+    fn0 = function foo()
+
+ebb0(v9999: i32):
+    v1 = iconst.i32 1
+    v2 = iconst.i32 2
+    [R#0,-]         v3 = iadd v1, v2 ; error: Instruction re-encoding
+    return v9999
+}

lib/cretonne/src/context.rs

@@ -53,8 +53,8 @@ impl Context {
     ///
     /// The `TargetIsa` argument is currently unused, but the verifier will soon be able to also
     /// check ISA-dependent constraints.
-    pub fn verify<'a, ISA: Into<Option<&'a TargetIsa>>>(&self, _isa: ISA) -> verifier::Result {
-        verifier::verify_context(&self.func, &self.cfg, &self.domtree)
+    pub fn verify<'a, ISA: Into<Option<&'a TargetIsa>>>(&self, isa: ISA) -> verifier::Result {
+        verifier::verify_context(&self.func, &self.cfg, &self.domtree, isa.into())
     }
 
     /// Run the verifier only if the `enable_verifier` setting is true.

lib/cretonne/src/isa/encoding.rs

@@ -60,7 +60,7 @@ impl fmt::Display for Encoding {
 }
 
 /// Temporary object that holds enough context to properly display an encoding.
-/// This is meant to be created by `TargetIsa::display_enc()`.
+/// This is meant to be created by `EncInfo::display()`.
 pub struct DisplayEncoding {
     pub encoding: Encoding,
     pub recipe_names: &'static [&'static str],

lib/cretonne/src/legalizer/mod.rs

@@ -91,6 +91,7 @@ pub fn legalize_function(func: &mut Function, cfg: &mut ControlFlowGraph, isa: &
                     // unsound. Should we attempt to detect that?
                     if changed {
                         pos.set_position(prev_pos);
+                        continue;
                     }
                 }
             }
@@ -99,6 +100,7 @@ pub fn legalize_function(func: &mut Function, cfg: &mut ControlFlowGraph, isa: &
             prev_pos = pos.position();
         }
     }
+    func.encodings.resize(func.dfg.num_insts());
 }
 
 // Include legalization patterns that were generated by `gen_legalizer.py` from the `XForms` in

lib/cretonne/src/regalloc/context.rs

@@ -63,7 +63,7 @@ impl Context {
             .run(isa, func, domtree, &mut self.liveness, &mut self.tracker);
 
         if isa.flags().enable_verifier() {
-            verify_context(func, cfg, domtree)?;
+            verify_context(func, cfg, domtree, Some(isa))?;
             verify_liveness(isa, func, cfg, &self.liveness)?;
         }
         Ok(())

lib/cretonne/src/verifier/mod.rs

@@ -58,6 +58,7 @@ use ir::entities::AnyEntity;
 use ir::instructions::{InstructionFormat, BranchInfo, ResolvedConstraint, CallInfo};
 use ir::{types, Function, ValueDef, Ebb, Inst, SigRef, FuncRef, ValueList, JumpTable, StackSlot,
          Value, Type};
+use isa::TargetIsa;
 use std::error as std_error;
 use std::fmt::{self, Display, Formatter};
 use std::result;
@@ -109,14 +110,18 @@ impl std_error::Error for Error {
 pub type Result = result::Result<(), Error>;
 
 /// Verify `func`.
-pub fn verify_function(func: &Function) -> Result {
-    Verifier::new(func).run()
+pub fn verify_function(func: &Function, isa: Option<&TargetIsa>) -> Result {
+    Verifier::new(func, isa).run()
 }
 
 /// Verify `func` after checking the integrity of associated context data structures `cfg` and
 /// `domtree`.
-pub fn verify_context(func: &Function, cfg: &ControlFlowGraph, domtree: &DominatorTree) -> Result {
-    let verifier = Verifier::new(func);
+pub fn verify_context(func: &Function,
+                      cfg: &ControlFlowGraph,
+                      domtree: &DominatorTree,
+                      isa: Option<&TargetIsa>)
+                      -> Result {
+    let verifier = Verifier::new(func, isa);
     verifier.cfg_integrity(cfg)?;
     verifier.domtree_integrity(domtree)?;
     verifier.run()
@@ -126,16 +131,18 @@ struct Verifier<'a> {
     func: &'a Function,
     cfg: ControlFlowGraph,
     domtree: DominatorTree,
+    isa: Option<&'a TargetIsa>,
 }
 
 impl<'a> Verifier<'a> {
-    pub fn new(func: &'a Function) -> Verifier {
+    pub fn new(func: &'a Function, isa: Option<&'a TargetIsa>) -> Verifier<'a> {
         let cfg = ControlFlowGraph::with_function(func);
         let domtree = DominatorTree::with_function(func, &cfg);
         Verifier {
             func: func,
             cfg: cfg,
             domtree: domtree,
+            isa: isa,
         }
     }
 
@@ -657,15 +664,53 @@ impl<'a> Verifier<'a> {
         Ok(())
     }
 
+    /// If the verifier has been set up with an ISA, make sure that the recorded encoding for the
+    /// instruction (if any) matches how the ISA would encode it.
+    fn verify_encoding(&self, inst: Inst) -> Result {
+        if let Some(isa) = self.isa {
+            let encoding = self.func
+                .encodings
+                .get(inst)
+                .cloned()
+                .unwrap_or_default();
+            if encoding.is_legal() {
+                let verify_encoding =
+                    isa.encode(&self.func.dfg,
+                               &self.func.dfg[inst],
+                               self.func.dfg.ctrl_typevar(inst));
+                match verify_encoding {
+                    Ok(verify_encoding) => {
+                        if verify_encoding != encoding {
+                            return err!(inst,
+                                        "Instruction re-encoding {} doesn't match {}",
+                                        isa.encoding_info().display(verify_encoding),
+                                        isa.encoding_info().display(encoding));
+                        }
+                    }
+                    Err(e) => {
+                        return err!(inst,
+                                    "Instruction failed to re-encode {}: {:?}",
+                                    isa.encoding_info().display(encoding),
+                                    e)
+                    }
+                }
+            }
+        }
+
+        Ok(())
+    }
+
     pub fn run(&self) -> Result {
         self.typecheck_entry_block_arguments()?;
         for ebb in self.func.layout.ebbs() {
             for inst in self.func.layout.ebb_insts(ebb) {
                 self.ebb_integrity(ebb, inst)?;
                 self.instruction_integrity(inst)?;
                 self.typecheck(inst)?;
+                self.verify_encoding(inst)?;
             }
         }
+
         Ok(())
     }
 }
@@ -692,7 +737,7 @@ mod tests {
     #[test]
     fn empty() {
         let func = Function::new();
-        let verifier = Verifier::new(&func);
+        let verifier = Verifier::new(&func, None);
         assert_eq!(verifier.run(), Ok(()));
     }
 
@@ -705,7 +750,7 @@ mod tests {
             func.dfg
                 .make_inst(InstructionData::Nullary { opcode: Opcode::Jump });
         func.layout.append_inst(nullary_with_bad_opcode, ebb0);
-        let verifier = Verifier::new(&func);
+        let verifier = Verifier::new(&func, None);
         assert_err_with_msg!(verifier.run(), "instruction format");
     }
 }

src/filetest/runone.rs

@@ -116,7 +116,7 @@ fn run_one_test<'a>(tuple: (&'a SubTest, &'a Flags, Option<&'a TargetIsa>),
 
     // Should we run the verifier before this test?
     if !context.verified && test.needs_verifier() {
-        verify_function(&func).map_err(|e| pretty_verifier_error(&func, e))?;
+        verify_function(&func, isa).map_err(|e| pretty_verifier_error(&func, e))?;
         context.verified = true;
     }
 

src/filetest/verifier.rs

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