Repository: bytecodealliance/wasmtime
Pull request #5663 (2023-01-31 00:02:12)

ISLE: reject multi-term rules with explicit priorities
In multi-terms, all matching rules fire. We treat the result as an unordered set of values, so setting rule priorities is meaningless. We want to prohibit relying on the rule match order in this case.

Also, codegen can produce invalid Rust if rules with different priorities both match against a multi-term. We first documented this symptom in #5647. As far as I can figure, prohibiting rule priorities prevents all possible instances of that bug.

At some point in the future we might decide we want to carefully define semantics for multi-term result ordering, at which point we can revisit this.


Patch:
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 48a300e78..6ea2885e8 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -56,6 +56,12 @@ jobs:
       with:
         submodules: true
     - uses: ./.github/actions/install-rust
+      # Note that the usage of this nightly toolchain is temporary until it
+      # rides to stable. After this nightly version becomes stable (Rust 1.69.0)
+      # then this should switch back to using stable by deleting the `with` and
+      # `toolchain` options.
+      with:
+        toolchain: nightly-2023-01-31
     # On one builder produce the source tarball since there's no need to produce
     # it everywhere
     - run: ./ci/build-src-tarball.sh
diff --git a/cranelift/codegen/meta/src/cdsl/instructions.rs b/cranelift/codegen/meta/src/cdsl/instructions.rs
index 11025921c..da5657ae8 100644
--- a/cranelift/codegen/meta/src/cdsl/instructions.rs
+++ b/cranelift/codegen/meta/src/cdsl/instructions.rs
@@ -171,52 +171,59 @@ impl InstructionBuilder {
         self
     }
 
-    #[allow(clippy::wrong_self_convention)]
-    pub fn is_terminator(mut self, val: bool) -> Self {
-        self.is_terminator = val;
+    /// Mark this instruction as a block terminator.
+    pub fn terminates_block(mut self) -> Self {
+        self.is_terminator = true;
         self
     }
 
-    #[allow(clippy::wrong_self_convention)]
-    pub fn is_branch(mut self, val: bool) -> Self {
-        self.is_branch = val;
-        self
+    /// Mark this instruction as a branch instruction. This also implies that the instruction is a
+    /// block terminator.
+    pub fn branches(mut self) -> Self {
+        self.is_branch = true;
+        self.terminates_block()
     }
 
-    #[allow(clippy::wrong_self_convention)]
-    pub fn is_call(mut self, val: bool) -> Self {
-        self.is_call = val;
+    /// Mark this instruction as a call instruction.
+    pub fn call(mut self) -> Self {
+        self.is_call = true;
         self
     }
 
-    #[allow(clippy::wrong_self_convention)]
-    pub fn is_return(mut self, val: bool) -> Self {
-        self.is_return = val;
-        self
+    /// Mark this instruction as a return instruction. This also implies that the instruction is a
+    /// block terminator.
+    pub fn returns(mut self) -> Self {
+        self.is_return = true;
+        self.terminates_block()
     }
 
-    pub fn can_load(mut self, val: bool) -> Self {
-        self.can_load = val;
+    /// Mark this instruction as one that can load from memory.
+    pub fn can_load(mut self) -> Self {
+        self.can_load = true;
         self
     }
 
-    pub fn can_store(mut self, val: bool) -> Self {
-        self.can_store = val;
+    /// Mark this instruction as one that can store to memory.
+    pub fn can_store(mut self) -> Self {
+        self.can_store = true;
         self
     }
 
-    pub fn can_trap(mut self, val: bool) -> Self {
-        self.can_trap = val;
+    /// Mark this instruction as possibly trapping.
+    pub fn can_trap(mut self) -> Self {
+        self.can_trap = true;
         self
     }
 
-    pub fn other_side_effects(mut self, val: bool) -> Self {
-        self.other_side_effects = val;
+    /// Mark this instruction as one that has side-effects.
+    pub fn other_side_effects(mut self) -> Self {
+        self.other_side_effects = true;
         self
     }
 
-    pub fn side_effects_idempotent(mut self, val: bool) -> Self {
-        self.side_effects_idempotent = val;
+    /// Mark this instruction as one whose side-effects may be de-duplicated.
+    pub fn side_effects_idempotent(mut self) -> Self {
+        self.side_effects_idempotent = true;
         self
     }
 
diff --git a/cranelift/codegen/meta/src/shared/instructions.rs b/cranelift/codegen/meta/src/shared/instructions.rs
index cdba18eb4..995b9346a 100755
--- a/cranelift/codegen/meta/src/shared/instructions.rs
+++ b/cranelift/codegen/meta/src/shared/instructions.rs
@@ -34,8 +34,7 @@ fn define_control_flow(
             &formats.jump,
         )
         .operands_in(vec![block_call])
-        .is_terminator(true)
-        .is_branch(true),
+        .branches(),
     );
 
     let ScalarTruthy = &TypeVar::new(
@@ -60,8 +59,7 @@ fn define_control_flow(
                 &formats.brif,
             )
             .operands_in(vec![c, block_then, block_else])
-            .is_terminator(true)
-            .is_branch(true),
+            .branches(),
         );
     }
 
@@ -96,8 +94,7 @@ fn define_control_flow(
                 &formats.branch_table,
             )
             .operands_in(vec![x, label, JT])
-            .is_terminator(true)
-            .is_branch(true),
+            .branches(),
         );
     }
 
@@ -115,9 +112,9 @@ fn define_control_flow(
     "#,
             &formats.nullary,
         )
-        .other_side_effects(true)
-        .can_load(true)
-        .can_store(true),
+        .other_side_effects()
+        .can_load()
+        .can_store(),
     );
 
     {
@@ -131,8 +128,8 @@ fn define_control_flow(
                 &formats.trap,
             )
             .operands_in(vec![code])
-            .can_trap(true)
-            .is_terminator(true),
+            .can_trap()
+            .terminates_block(),
         );
 
         let c = &Operand::new("c", ScalarTruthy).with_doc("Controlling value to test");
@@ -147,7 +144,7 @@ fn define_control_flow(
                 &formats.cond_trap,
             )
             .operands_in(vec![c, code])
-            .can_trap(true),
+            .can_trap(),
         );
 
         ig.push(
@@ -161,7 +158,7 @@ fn define_control_flow(
                 &formats.trap,
             )
             .operands_in(vec![code])
-            .can_trap(true),
+            .can_trap(),
         );
 
         let c = &Operand::new("c", ScalarTruthy).with_doc("Controlling value to test");
@@ -176,7 +173,7 @@ fn define_control_flow(
                 &formats.cond_trap,
             )
             .operands_in(vec![c, code])
-            .can_trap(true),
+            .can_trap(),
         );
 
         ig.push(
@@ -190,7 +187,7 @@ fn define_control_flow(
                 &formats.cond_trap,
             )
             .operands_in(vec![c, code])
-            .can_trap(true),
+            .can_trap(),
         );
     }
 
@@ -208,8 +205,7 @@ fn define_control_flow(
             &formats.multiary,
         )
         .operands_in(vec![rvals])
-        .is_return(true)
-        .is_terminator(true),
+        .returns(),
     );
 
     let FN = &Operand::new("FN", &entities.func_ref)
@@ -229,7 +225,7 @@ fn define_control_flow(
         )
         .operands_in(vec![FN, args])
         .operands_out(vec![rvals])
-        .is_call(true),
+        .call(),
     );
 
     let SIG = &Operand::new("SIG", &entities.sig_ref).with_doc("function signature");
@@ -254,7 +250,7 @@ fn define_control_flow(
         )
         .operands_in(vec![SIG, callee, args])
         .operands_out(vec![rvals])
-        .is_call(true),
+        .call(),
     );
 
     let FN = &Operand::new("FN", &entities.func_ref)
@@ -675,7 +671,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     ig.push(
@@ -690,7 +686,7 @@ pub(crate) fn define(
             &formats.store,
         )
         .operands_in(vec![MemFlags, x, p, Offset])
-        .can_store(true),
+        .can_store(),
     );
 
     let iExt8 = &TypeVar::new(
@@ -713,7 +709,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     ig.push(
@@ -728,7 +724,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     ig.push(
@@ -742,7 +738,7 @@ pub(crate) fn define(
             &formats.store,
         )
         .operands_in(vec![MemFlags, x, p, Offset])
-        .can_store(true),
+        .can_store(),
     );
 
     let iExt16 = &TypeVar::new(
@@ -765,7 +761,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     ig.push(
@@ -780,7 +776,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     ig.push(
@@ -794,7 +790,7 @@ pub(crate) fn define(
             &formats.store,
         )
         .operands_in(vec![MemFlags, x, p, Offset])
-        .can_store(true),
+        .can_store(),
     );
 
     let iExt32 = &TypeVar::new(
@@ -817,7 +813,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     ig.push(
@@ -832,7 +828,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     ig.push(
@@ -846,7 +842,7 @@ pub(crate) fn define(
             &formats.store,
         )
         .operands_in(vec![MemFlags, x, p, Offset])
-        .can_store(true),
+        .can_store(),
     );
 
     let I16x8 = &TypeVar::new(
@@ -871,7 +867,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     ig.push(
@@ -885,7 +881,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     let I32x4 = &TypeVar::new(
@@ -910,7 +906,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     ig.push(
@@ -924,7 +920,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     let I64x2 = &TypeVar::new(
@@ -949,7 +945,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     ig.push(
@@ -963,7 +959,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     let x = &Operand::new("x", Mem).with_doc("Value to be stored");
@@ -988,7 +984,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![SS, Offset])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     ig.push(
@@ -1007,7 +1003,7 @@ pub(crate) fn define(
             &formats.stack_store,
         )
         .operands_in(vec![x, SS, Offset])
-        .can_store(true),
+        .can_store(),
     );
 
     ig.push(
@@ -1039,7 +1035,7 @@ pub(crate) fn define(
         )
         .operands_in(vec![DSS])
         .operands_out(vec![a])
-        .can_load(true),
+        .can_load(),
     );
 
     ig.push(
@@ -1054,7 +1050,7 @@ pub(crate) fn define(
             &formats.dynamic_stack_store,
         )
         .operands_in(vec![x, DSS])
-        .can_store(true),
+        .can_store(),
     );
 
     let GV = &Operand::new("GV", &entities.global_value);
@@ -1123,7 +1119,7 @@ pub(crate) fn define(
             &formats.nullary,
         )
         .operands_out(vec![addr])
-        .other_side_effects(true),
+        .other_side_effects(),
     );
 
     ig.push(
@@ -1135,7 +1131,7 @@ pub(crate) fn define(
             &formats.unary,
         )
         .operands_in(vec![addr])
-        .other_side_effects(true),
+        .other_side_effects(),
     );
 
     ig.push(
@@ -1379,10 +1375,10 @@ pub(crate) fn define(
         )
         .operands_in(vec![c, x, y])
         .operands_out(vec![a])
-        .other_side_effects(true)
+        .other_side_effects()
         // We can de-duplicate spectre selects since the side effect is
         // idempotent.
-        .side_effects_idempotent(true),
+        .side_effects_idempotent(),
     );
 
     let c = &Operand::new("c", Any).with_doc("Controlling value to test");
@@ -1676,8 +1672,8 @@ pub(crate) fn define(
             )
             .operands_in(vec![x, y])
             .operands_out(vec![a])
-            .can_trap(true)
-            .side_effects_idempotent(true),
+            .can_trap()
+            .side_effects_idempotent(),
         );
 
         ig.push(
@@ -1695,8 +1691,8 @@ pub(crate) fn define(
             )
             .operands_in(vec![x, y])
             .operands_out(vec![a])
-            .can_trap(true)
-            .side_effects_idempotent(true),
+            .can_trap()
+            .side_effects_idempotent(),
         );
 
         ig.push(
@@ -1711,8 +1707,8 @@ pub(crate) fn define(
             )
             .operands_in(vec![x, y])
             .operands_out(vec![a])
-            .can_trap(true)
-            .side_effects_idempotent(true),
+            .can_trap()
+            .side_effects_idempotent(),
         );
 
         ig.push(
@@ -1727,8 +1723,8 @@ pub(crate) fn define(
             )
             .operands_in(vec![x, y])
             .operands_out(vec![a])
-            .can_trap(true)
-            .side_effects_idempotent(true),
+            .can_trap()
+            .side_effects_idempotent(),
         );
     }
 
@@ -1955,8 +1951,8 @@ pub(crate) fn define(
             )
             .operands_in(vec![x, y, code])
             .operands_out(vec![a])
-            .can_trap(true)
-            .side_effects_idempotent(true),
+            .can_trap()
+            .side_effects_idempotent(),
         );
     }
 
@@ -3318,8 +3314,8 @@ pub(crate) fn define(
         )
         .operands_in(vec![x])
         .operands_out(vec![a])
-        .can_trap(true)
-        .side_effects_idempotent(true),
+        .can_trap()
+        .side_effects_idempotent(),
     );
 
     ig.push(
@@ -3337,8 +3333,8 @@ pub(crate) fn define(
         )
         .operands_in(vec![x])
         .operands_out(vec![a])
-        .can_trap(true)
-        .side_effects_idempotent(true),
+        .can_trap()
+        .side_effects_idempotent(),
     );
 
     let x = &Operand::new("x", Float);
@@ -3514,9 +3510,9 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, AtomicRmwOp, p, x])
         .operands_out(vec![a])
-        .can_load(true)
-        .can_store(true)
-        .other_side_effects(true),
+        .can_load()
+        .can_store()
+        .other_side_effects(),
     );
 
     ig.push(
@@ -3536,9 +3532,9 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p, e, x])
         .operands_out(vec![a])
-        .can_load(true)
-        .can_store(true)
-        .other_side_effects(true),
+        .can_load()
+        .can_store()
+        .other_side_effects(),
     );
 
     ig.push(
@@ -3556,8 +3552,8 @@ pub(crate) fn define(
         )
         .operands_in(vec![MemFlags, p])
         .operands_out(vec![a])
-        .can_load(true)
-        .other_side_effects(true),
+        .can_load()
+        .other_side_effects(),
     );
 
     ig.push(
@@ -3574,8 +3570,8 @@ pub(crate) fn define(
             &formats.store_no_offset,
         )
         .operands_in(vec![MemFlags, x, p])
-        .can_store(true)
-        .other_side_effects(true),
+        .can_store()
+        .other_side_effects(),
     );
 
     ig.push(
@@ -3588,7 +3584,7 @@ pub(crate) fn define(
         "#,
             &formats.nullary,
         )
-        .other_side_effects(true),
+        .other_side_effects(),
     );
 
     let TxN = &TypeVar::new(
diff --git a/cranelift/codegen/src/souper_harvest.rs b/cranelift/codegen/src/souper_harvest.rs
index a690fa726..d818eee9f 100644
--- a/cranelift/codegen/src/souper_harvest.rs
+++ b/cranelift/codegen/src/souper_harvest.rs
@@ -387,7 +387,8 @@ fn harvest_candidate_lhs(
                         let a = arg(allocs, 0);
 
                         // While Cranelift allows any width condition for
-                        // `select`, Souper requires an `i1`.
+                        // `select` and checks it against `0`, Souper requires
+                        // an `i1`. So insert a `ne %x, 0` as needed.
                         let a = match a {
                             ast::Operand::Value(id) => match lhs.get_value(id).r#type {
                                 Some(ast::Type { width: 1 }) => a,
@@ -395,7 +396,14 @@ fn harvest_candidate_lhs(
                                     .assignment(
                                         None,
                                         Some(ast::Type { width: 1 }),
-                                        ast::Instruction::Trunc { a },
+                                        ast::Instruction::Ne {
+                                            a,
+                                            b: ast::Constant {
+                                                value: 0,
+                                                r#type: None,
+                                            }
+                                            .into(),
+                                        },
                                         vec![],
                                     )
                                     .into(),
diff --git a/cranelift/isle/isle/isle_examples/fail/multi_prio.isle b/cranelift/isle/isle/isle_examples/fail/multi_prio.isle
new file mode 100644
index 000000000..30ce9c7a5
--- /dev/null
+++ b/cranelift/isle/isle/isle_examples/fail/multi_prio.isle
@@ -0,0 +1,4 @@
+(type u32 (primitive u32))
+
+(decl multi A (u32) u32)
+(rule 0 (A x) x)
diff --git a/cranelift/isle/isle/isle_examples/link/multi_extractor.isle b/cranelift/isle/isle/isle_examples/link/multi_extractor.isle
index 5878dd065..2630d8682 100644
--- a/cranelift/isle/isle/isle_examples/link/multi_extractor.isle
+++ b/cranelift/isle/isle/isle_examples/link/multi_extractor.isle
@@ -7,7 +7,7 @@
 
 (decl multi Rule (u32) u32)
 
-(rule 1 (Rule (E1 a idx))
+(rule (Rule (E1 a idx))
       (if-let (A.B) a)
       idx)
 (rule (Rule _)
diff --git a/cranelift/isle/isle/src/sema.rs b/cranelift/isle/isle/src/sema.rs
index 95481f106..ba9906f85 100644
--- a/cranelift/isle/isle/src/sema.rs
+++ b/cranelift/isle/isle/src/sema.rs
@@ -1744,6 +1744,18 @@ impl TermEnv {
 
                     bindings.exit_scope();
 
+                    let prio = if let Some(prio) = rule.prio {
+                        if flags.multi {
+                            tyenv.report_error(
+                                pos,
+                                "Cannot set rule priorities in multi-terms".to_string(),
+                            );
+                        }
+                        prio
+                    } else {
+                        0
+                    };
+
                     let rid = RuleId(self.rules.len());
                     self.rules.push(Rule {
                         id: rid,
@@ -1752,7 +1764,7 @@ impl TermEnv {
                         iflets,
                         rhs,
                         vars: bindings.seen,
-                        prio: rule.prio.unwrap_or(0),
+                        prio,
                         pos,
                     });
                 }


Test patch:
diff --git a/cranelift/isle/isle/tests/run_tests.rs b/cranelift/isle/isle/tests/run_tests.rs
index 9adf51c32..1b347b945 100644
--- a/cranelift/isle/isle/tests/run_tests.rs
+++ b/cranelift/isle/isle/tests/run_tests.rs
@@ -15,8 +15,12 @@ pub fn run_pass(filename: &str) {
 }
 
 pub fn run_fail(filename: &str) {
-    if build(filename).is_ok() {
-        panic!("test {} passed unexpectedly", filename);
+    match build(filename) {
+        Ok(_) => panic!("test {} passed unexpectedly", filename),
+        Err(err) => {
+            // Log the actual errors for use with `cargo test -- --nocapture`
+            println!("failed, as expected:\n{:?}", err);
+        }
     }
 }