Repository: bytecodealliance/wasmtime
Pull request #6013 (2023-03-13 23:26:58)

cranelift: Add narrower and wider constraints to the instruction DSL
In service of merging #5947, add the `narrower` and `wider` constraints to the `TypeVar` type in the instruction DSL.

These two new constraints capture the situation where an instruction requires that its output be either narrower or wider than one of its input arguments. Instructions that fit one of these two patterns include: `uextend`, `sextend`, `ireduce`, `fpromote`, and `fdemote`. Previously these constraints were validated by special case typechecking behavior in the verifier, and with their addition those special cases can now be removed.

The benefit to #5947 is that the constraints are now available to the `function_generator` module when determining which arguments to select for random instruction instantiations. This gets us a bit closer to being able to use the generated opcode list as the source of truth for how we generate arbitrary instructions.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

- [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
- [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
- [ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the [code of
conduct](https://github.com/bytecodealliance/wasmtime/blob/master/CODE_OF_CONDUCT.md).
-->


elliottt: > Could you also diff the generated target/debug/build/cranelift-codegen-*/out/opcodes.rs before and after this change? I'm particularly interested in making sure that OPCODE_CONSTRAINTS isn't different in any interesting way except for the five instructions you've updated to use the new derived types. There are some nice comments in the generated code for the computed value of use_typevar_operand, for example.

`OPCODE_CONSTRAINTS` only differs in that it's now 80 elements long. There are some new constraints that uses `Narrower` and `Wider`, with two previously completely free constraints being replaced with a constrained use of the controlling variable.
elliottt: I realized that the changes to the `is_ctrl_typevar_candidate` were only necessary when we were allowing an instructions return type to include a constrained use of the control type. As we needed to avoid that to ensure that instructions always had a well-defined return type, those changes could be reverted as well.

Patch:
diff --git a/cranelift/codegen/meta/src/cdsl/typevar.rs b/cranelift/codegen/meta/src/cdsl/typevar.rs
index f5875be2e..7b1381f17 100644
--- a/cranelift/codegen/meta/src/cdsl/typevar.rs
+++ b/cranelift/codegen/meta/src/cdsl/typevar.rs
@@ -202,6 +202,32 @@ impl TypeVar {
                     "can't halve a scalar type"
                 );
             }
+            DerivedFunc::Narrower => {
+                assert_eq!(
+                    *ts.lanes.iter().max().unwrap(),
+                    1,
+                    "The `narrower` constraint does not apply to vectors"
+                );
+                assert!(
+                    (!ts.ints.is_empty() || !ts.floats.is_empty())
+                        && ts.refs.is_empty()
+                        && ts.dynamic_lanes.is_empty(),
+                    "The `narrower` constraint only applies to scalar ints or floats"
+                );
+            }
+            DerivedFunc::Wider => {
+                assert_eq!(
+                    *ts.lanes.iter().max().unwrap(),
+                    1,
+                    "The `wider` constraint does not apply to vectors"
+                );
+                assert!(
+                    (!ts.ints.is_empty() || !ts.floats.is_empty())
+                        && ts.refs.is_empty()
+                        && ts.dynamic_lanes.is_empty(),
+                    "The `wider` constraint only applies to scalar ints or floats"
+                );
+            }
             DerivedFunc::LaneOf | DerivedFunc::AsBool | DerivedFunc::DynamicToVector => {
                 /* no particular assertions */
             }
@@ -241,6 +267,16 @@ impl TypeVar {
     pub fn dynamic_to_vector(&self) -> TypeVar {
         self.derived(DerivedFunc::DynamicToVector)
     }
+
+    /// Make a new [TypeVar] that includes all types narrower than self.
+    pub fn narrower(&self) -> TypeVar {
+        self.derived(DerivedFunc::Narrower)
+    }
+
+    /// Make a new [TypeVar] that includes all types wider than self.
+    pub fn wider(&self) -> TypeVar {
+        self.derived(DerivedFunc::Wider)
+    }
 }
 
 impl Into<TypeVar> for &TypeVar {
@@ -302,6 +338,8 @@ pub(crate) enum DerivedFunc {
     SplitLanes,
     MergeLanes,
     DynamicToVector,
+    Narrower,
+    Wider,
 }
 
 impl DerivedFunc {
@@ -314,6 +352,8 @@ impl DerivedFunc {
             DerivedFunc::SplitLanes => "split_lanes",
             DerivedFunc::MergeLanes => "merge_lanes",
             DerivedFunc::DynamicToVector => "dynamic_to_vector",
+            DerivedFunc::Narrower => "narrower",
+            DerivedFunc::Wider => "wider",
         }
     }
 }
@@ -391,6 +431,8 @@ impl TypeSet {
             DerivedFunc::SplitLanes => self.half_width().double_vector(),
             DerivedFunc::MergeLanes => self.double_width().half_vector(),
             DerivedFunc::DynamicToVector => self.dynamic_to_vector(),
+            DerivedFunc::Narrower => self.clone(),
+            DerivedFunc::Wider => self.clone(),
         }
     }
 
diff --git a/cranelift/codegen/meta/src/shared/instructions.rs b/cranelift/codegen/meta/src/shared/instructions.rs
index 4c7dbe5c7..09bb43e21 100755
--- a/cranelift/codegen/meta/src/shared/instructions.rs
+++ b/cranelift/codegen/meta/src/shared/instructions.rs
@@ -3064,12 +3064,6 @@ pub(crate) fn define(
         TypeSetBuilder::new().ints(Interval::All).build(),
     );
 
-    let IntTo = &TypeVar::new(
-        "IntTo",
-        "A smaller integer type",
-        TypeSetBuilder::new().ints(Interval::All).build(),
-    );
-
     ig.push(
         Inst::new(
             "ireduce",
@@ -3081,8 +3075,9 @@ pub(crate) fn define(
         "#,
             &formats.unary,
         )
-        .operands_in(vec![Operand::new("x", Int)])
-        .operands_out(vec![Operand::new("a", IntTo)]),
+        .operands_in(vec![Operand::new("x", &Int.wider())
+            .with_doc("A scalar integer type, wider than the controlling type")])
+        .operands_out(vec![Operand::new("a", Int)]),
     );
 
     let I16or32or64xN = &TypeVar::new(
@@ -3272,17 +3267,10 @@ pub(crate) fn define(
         .operands_out(vec![Operand::new("a", I16x8)]),
     );
 
-    {
-        let IntTo = &TypeVar::new(
-            "IntTo",
-            "A larger integer type with the same number of lanes",
-            TypeSetBuilder::new().ints(Interval::All).build(),
-        );
-
-        ig.push(
-            Inst::new(
-                "uextend",
-                r#"
+    ig.push(
+        Inst::new(
+            "uextend",
+            r#"
         Convert `x` to a larger integer type by zero-extending.
 
         Each lane in `x` is converted to a larger integer type by adding
@@ -3293,16 +3281,18 @@ pub(crate) fn define(
         and each lane must not have fewer bits that the input lanes. If the
         input and output types are the same, this is a no-op.
         "#,
-                &formats.unary,
-            )
-            .operands_in(vec![Operand::new("x", Int)])
-            .operands_out(vec![Operand::new("a", IntTo)]),
-        );
+            &formats.unary,
+        )
+        .operands_in(vec![Operand::new("x", &Int.narrower()).with_doc(
+            "A scalar integer type, narrower than the controlling type",
+        )])
+        .operands_out(vec![Operand::new("a", Int)]),
+    );
 
-        ig.push(
-            Inst::new(
-                "sextend",
-                r#"
+    ig.push(
+        Inst::new(
+            "sextend",
+            r#"
         Convert `x` to a larger integer type by sign-extending.
 
         Each lane in `x` is converted to a larger integer type by replicating
@@ -3313,12 +3303,13 @@ pub(crate) fn define(
         and each lane must not have fewer bits that the input lanes. If the
         input and output types are the same, this is a no-op.
         "#,
-                &formats.unary,
-            )
-            .operands_in(vec![Operand::new("x", Int)])
-            .operands_out(vec![Operand::new("a", IntTo)]),
-        );
-    }
+            &formats.unary,
+        )
+        .operands_in(vec![Operand::new("x", &Int.narrower()).with_doc(
+            "A scalar integer type, narrower than the controlling type",
+        )])
+        .operands_out(vec![Operand::new("a", Int)]),
+    );
 
     let FloatScalar = &TypeVar::new(
         "FloatScalar",
@@ -3326,12 +3317,6 @@ pub(crate) fn define(
         TypeSetBuilder::new().floats(Interval::All).build(),
     );
 
-    let FloatScalarTo = &TypeVar::new(
-        "FloatScalarTo",
-        "A scalar only floating point number",
-        TypeSetBuilder::new().floats(Interval::All).build(),
-    );
-
     ig.push(
         Inst::new(
             "fpromote",
@@ -3349,8 +3334,10 @@ pub(crate) fn define(
         "#,
             &formats.unary,
         )
-        .operands_in(vec![Operand::new("x", FloatScalar)])
-        .operands_out(vec![Operand::new("a", FloatScalarTo)]),
+        .operands_in(vec![Operand::new("x", &FloatScalar.narrower()).with_doc(
+            "A scalar only floating point number, narrower than the controlling type",
+        )])
+        .operands_out(vec![Operand::new("a", FloatScalar)]),
     );
 
     ig.push(
@@ -3370,8 +3357,10 @@ pub(crate) fn define(
         "#,
             &formats.unary,
         )
-        .operands_in(vec![Operand::new("x", FloatScalar)])
-        .operands_out(vec![Operand::new("a", FloatScalarTo)]),
+        .operands_in(vec![Operand::new("x", &FloatScalar.wider()).with_doc(
+            "A scalar only floating point number, wider than the controlling type",
+        )])
+        .operands_out(vec![Operand::new("a", FloatScalar)]),
     );
 
     let F64x2 = &TypeVar::new(
diff --git a/cranelift/codegen/src/bitset.rs b/cranelift/codegen/src/bitset.rs
index c09a58777..d39661ec2 100644
--- a/cranelift/codegen/src/bitset.rs
+++ b/cranelift/codegen/src/bitset.rs
@@ -11,7 +11,7 @@ use core::mem::size_of;
 use core::ops::{Add, BitOr, Shl, Sub};
 
 /// A small bitset built on a single primitive integer type
-#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
 #[cfg_attr(feature = "enable-serde", derive(serde::Serialize, serde::Deserialize))]
 pub struct BitSet<T>(pub T);
 
diff --git a/cranelift/codegen/src/ir/instructions.rs b/cranelift/codegen/src/ir/instructions.rs
index 40507db01..a8facc1f0 100644
--- a/cranelift/codegen/src/ir/instructions.rs
+++ b/cranelift/codegen/src/ir/instructions.rs
@@ -575,12 +575,9 @@ impl OpcodeConstraints {
     /// `ctrl_type`.
     pub fn result_type(self, n: usize, ctrl_type: Type) -> Type {
         debug_assert!(n < self.num_fixed_results(), "Invalid result index");
-        if let ResolvedConstraint::Bound(t) =
-            OPERAND_CONSTRAINTS[self.constraint_offset() + n].resolve(ctrl_type)
-        {
-            t
-        } else {
-            panic!("Result constraints can't be free");
+        match OPERAND_CONSTRAINTS[self.constraint_offset() + n].resolve(ctrl_type) {
+            ResolvedConstraint::Bound(t) => t,
+            ResolvedConstraint::Free(ts) => panic!("Result constraints can't be free: {:?}", ts),
         }
     }
 
@@ -614,7 +611,7 @@ type BitSet8 = BitSet<u8>;
 type BitSet16 = BitSet<u16>;
 
 /// A value type set describes the permitted set of types for a type variable.
-#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
 pub struct ValueTypeSet {
     /// Allowed lane sizes
     pub lanes: BitSet16,
@@ -703,6 +700,12 @@ enum OperandConstraint {
 
     /// This operands is `ctrlType.dynamic_to_vector()`.
     DynamicToVector,
+
+    /// This operand is `ctrlType.narrower()`.
+    Narrower,
+
+    /// This operand is `ctrlType.wider()`.
+    Wider,
 }
 
 impl OperandConstraint {
@@ -766,6 +769,47 @@ impl OperandConstraint {
                     .dynamic_to_vector()
                     .expect("invalid type for dynamic_to_vector"),
             ),
+            Narrower => {
+                let ctrl_type_bits = ctrl_type.log2_lane_bits();
+                let mut tys = ValueTypeSet::default();
+
+                // We're testing scalar values, only.
+                tys.lanes = BitSet::from_range(0, 1);
+
+                if ctrl_type.is_int() {
+                    // The upper bound in from_range is exclusive, so add one here get the closed
+                    // interval of [I8, ctrl_type].
+                    tys.ints = BitSet::from_range(3, ctrl_type_bits as u8 + 1);
+                } else if ctrl_type.is_float() {
+                    // The upper bound in from_range is exclusive, so add one here get the closed
+                    // interval of [F32, ctrl_type].
+                    tys.floats = BitSet::from_range(5, ctrl_type_bits as u8 + 1);
+                } else {
+                    panic!("The Narrower constraint only operates on floats or ints");
+                }
+                ResolvedConstraint::Free(tys)
+            }
+            Wider => {
+                let ctrl_type_bits = ctrl_type.log2_lane_bits();
+                let mut tys = ValueTypeSet::default();
+
+                // We're testing scalar values, only.
+                tys.lanes = BitSet::from_range(0, 1);
+
+                if ctrl_type.is_int() {
+                    // The upper bound should include all types wider than `ctrl_type`, so we use
+                    // `2^8` as the upper bound to define the closed range `[ctrl_type, I128]`.
+                    tys.ints = BitSet::from_range(ctrl_type_bits as u8, 8);
+                } else if ctrl_type.is_float() {
+                    // The upper bound should include all float types wider than `ctrl_type`, so we
+                    // use `2^7` as the upper bound to define the closed range `[ctrl_type, F64]`.
+                    tys.floats = BitSet::from_range(ctrl_type_bits as u8, 7);
+                } else {
+                    panic!("The Wider constraint only operates on floats or ints");
+                }
+
+                ResolvedConstraint::Free(tys)
+            }
         }
     }
 }
diff --git a/cranelift/codegen/src/verifier/mod.rs b/cranelift/codegen/src/verifier/mod.rs
index cd8d76771..016abdbf7 100644
--- a/cranelift/codegen/src/verifier/mod.rs
+++ b/cranelift/codegen/src/verifier/mod.rs
@@ -1191,7 +1191,7 @@ impl<'a> Verifier<'a> {
         let _ = self.typecheck_fixed_args(inst, ctrl_type, errors);
         let _ = self.typecheck_variable_args(inst, errors);
         let _ = self.typecheck_return(inst, errors);
-        let _ = self.typecheck_special(inst, ctrl_type, errors);
+        let _ = self.typecheck_special(inst, errors);
 
         Ok(())
     }
@@ -1478,63 +1478,8 @@ impl<'a> Verifier<'a> {
 
     // Check special-purpose type constraints that can't be expressed in the normal opcode
     // constraints.
-    fn typecheck_special(
-        &self,
-        inst: Inst,
-        ctrl_type: Type,
-        errors: &mut VerifierErrors,
-    ) -> VerifierStepResult<()> {
+    fn typecheck_special(&self, inst: Inst, errors: &mut VerifierErrors) -> VerifierStepResult<()> {
         match self.func.dfg.insts[inst] {
-            ir::InstructionData::Unary { opcode, arg } => {
-                let arg_type = self.func.dfg.value_type(arg);
-                match opcode {
-                    Opcode::Uextend | Opcode::Sextend | Opcode::Fpromote => {
-                        if arg_type.lane_count() != ctrl_type.lane_count() {
-                            return errors.nonfatal((
-                                inst,
-                                self.context(inst),
-                                format!(
-                                    "input {} and output {} must have same number of lanes",
-                                    arg_type, ctrl_type,
-                                ),
-                            ));
-                        }
-                        if arg_type.lane_bits() >= ctrl_type.lane_bits() {
-                            return errors.nonfatal((
-                                inst,
-                                self.context(inst),
-                                format!(
-                                    "input {} must be smaller than output {}",
-                                    arg_type, ctrl_type,
-                                ),
-                            ));
-                        }
-                    }
-                    Opcode::Ireduce | Opcode::Fdemote => {
-                        if arg_type.lane_count() != ctrl_type.lane_count() {
-                            return errors.nonfatal((
-                                inst,
-                                self.context(inst),
-                                format!(
-                                    "input {} and output {} must have same number of lanes",
-                                    arg_type, ctrl_type,
-                                ),
-                            ));
-                        }
-                        if arg_type.lane_bits() <= ctrl_type.lane_bits() {
-                            return errors.nonfatal((
-                                inst,
-                                self.context(inst),
-                                format!(
-                                    "input {} must be larger than output {}",
-                                    arg_type, ctrl_type,
-                                ),
-                            ));
-                        }
-                    }
-                    _ => {}
-                }
-            }
             ir::InstructionData::TableAddr { table, arg, .. } => {
                 let index_type = self.func.dfg.value_type(arg);
                 let table_index_type = self.func.tables[table].index_type;


Test patch:
diff --git a/cranelift/filetests/filetests/verifier/type_check.clif b/cranelift/filetests/filetests/verifier/type_check.clif
index ea5ed07ed..68de2338e 100644
--- a/cranelift/filetests/filetests/verifier/type_check.clif
+++ b/cranelift/filetests/filetests/verifier/type_check.clif
@@ -113,14 +113,28 @@ block2(v3: f32, v4: i8):
 function %bad_extend() {
 block0:
     v0 = iconst.i32 10
-    v1 = uextend.i16 v0 ; error: input i32 must be smaller than output i16
+    v1 = uextend.i16 v0 ; error: arg 0 (v0) with type i32 failed to satisfy type set
     return
 }
 
 function %bad_reduce() {
 block0:
     v0 = iconst.i32 10
-    v1 = ireduce.i64 v0 ; error: input i32 must be larger than output i64
+    v1 = ireduce.i64 v0 ; error: arg 0 (v0) with type i32 failed to satisfy type set
+    return
+}
+
+function %bad_fdemote() {
+block0:
+    v0 = f32const 0xf.f
+    v1 = fdemote.f64 v0 ; error: arg 0 (v0) with type f32 failed to satisfy type set
+    return
+}
+
+function %bad_fpromote() {
+block0:
+    v0 = f64const 0xf.f
+    v1 = fpromote.f32 v0 ; error: arg 0 (v0) with type f64 failed to satisfy type set
     return
 }