Cranelift: Stop sign-extending Imm64 immediates in builder methods (bytecodealliance#9046)

* Cranelift: Stop sign-extending `Imm64` immediates in builder methods

Originally, we sign-extended `Imm64` immediates from their controlling type
var's width to `i64` in the builder methods for `BinaryImm64` and a couple other
instruction formats: bytecodealliance#1687

At some point, we decided that the unused bits in the `Imm64` when the
controlling type var's width is less than 64 should be zero. And we started
asserting that in various places, for example:
https://github.com/bytecodealliance/wasmtime/blob/87817f38a128caa76eaa6a3c3c8ceac81a329a3e/cranelift/codegen/src/machinst/lower.rs#L1237-L1243

However, we never removed or updated the sign-extension code in the builder
methods. This commit switches the builder methods over to masking the `Imm64` to
just the valid bits, effectively doing a zero extend from the controlling type
var's width instead of a sign extend.

To avoid too much churn in tests, I made `display_inst` print the
sign-extended-from-the-controlling-type-variable's-width value of `Imm64`
immediates. I also made the `Display` implementation for `Imm64` always print in
decimal form with a `-` for negative numbers, rather than the hex it would
previously print for large negative numbers, so that `iconst.i32 0x8000_0000`
doesn't display as `iconst.i32 0xffff_ffff_8000_0000`, which is otherwise pretty
confusing.

* Rename condition

* return new immediates instead of mutating

* Update some test expectations

Author: fitzgen

cranelift/codegen/meta/src/gen_inst.rs

@@ -973,7 +973,7 @@ fn gen_format_constructor(format: &InstructionFormat, fmt: &mut Formatter) {
         args.join(", ")
     );
 
-    let imms_need_sign_extension = format
+    let imms_need_masking = format
         .imm_fields
         .iter()
         .any(|f| f.kind.rust_type == "ir::immediates::Imm64");
@@ -986,7 +986,7 @@ fn gen_format_constructor(format: &InstructionFormat, fmt: &mut Formatter) {
         fmtln!(
             fmt,
             "let{} data = ir::InstructionData::{} {{",
-            if imms_need_sign_extension { " mut" } else { "" },
+            if imms_need_masking { " mut" } else { "" },
             format.name
         );
         fmt.indent(|fmt| {
@@ -995,8 +995,8 @@ fn gen_format_constructor(format: &InstructionFormat, fmt: &mut Formatter) {
         });
         fmtln!(fmt, "};");
 
-        if imms_need_sign_extension {
-            fmtln!(fmt, "data.sign_extend_immediates(ctrl_typevar);");
+        if imms_need_masking {
+            fmtln!(fmt, "data.mask_immediates(ctrl_typevar);");
         }
 
         // Assert that this opcode belongs to this format

cranelift/codegen/src/ir/immediates.rs

@@ -74,19 +74,38 @@ impl Imm64 {
         self.0
     }
 
+    /// Mask this immediate to the given power-of-two bit width.
+    #[must_use]
+    pub(crate) fn mask_to_width(&self, bit_width: u32) -> Self {
+        debug_assert!(bit_width.is_power_of_two());
+
+        if bit_width >= 64 {
+            return *self;
+        }
+
+        let bit_width = i64::from(bit_width);
+        let mask = (1 << bit_width) - 1;
+        let masked = self.0 & mask;
+        Imm64(masked)
+    }
+
     /// Sign extend this immediate as if it were a signed integer of the given
     /// power-of-two width.
-    pub fn sign_extend_from_width(&mut self, bit_width: u32) {
-        debug_assert!(bit_width.is_power_of_two());
+    #[must_use]
+    pub(crate) fn sign_extend_from_width(&self, bit_width: u32) -> Self {
+        debug_assert!(
+            bit_width.is_power_of_two(),
+            "{bit_width} is not a power of two"
+        );
 
         if bit_width >= 64 {
-            return;
+            return *self;
         }
 
         let bit_width = i64::from(bit_width);
         let delta = 64 - bit_width;
         let sign_extended = (self.0 << delta) >> delta;
-        *self = Imm64(sign_extended);
+        Imm64(sign_extended)
     }
 }
 
@@ -111,8 +130,8 @@ impl From<i64> for Imm64 {
 impl Display for Imm64 {
     fn fmt(&self, f: &mut Formatter) -> fmt::Result {
         let x = self.0;
-        if -10_000 < x && x < 10_000 {
-            // Use decimal for small numbers.
+        if x < 10_000 {
+            // Use decimal for small and negative numbers.
             write!(f, "{}", x)
         } else {
             write_hex(x as u64, f)
@@ -1093,7 +1112,7 @@ mod tests {
         assert_eq!(Imm64(9999).to_string(), "9999");
         assert_eq!(Imm64(10000).to_string(), "0x2710");
         assert_eq!(Imm64(-9999).to_string(), "-9999");
-        assert_eq!(Imm64(-10000).to_string(), "0xffff_ffff_ffff_d8f0");
+        assert_eq!(Imm64(-10000).to_string(), "-10000");
         assert_eq!(Imm64(0xffff).to_string(), "0xffff");
         assert_eq!(Imm64(0x10000).to_string(), "0x0001_0000");
     }
@@ -1113,6 +1132,7 @@ mod tests {
     }
 
     // Verify that `text` can be parsed as a `T` into a value that displays as `want`.
+    #[track_caller]
     fn parse_ok<T: FromStr + Display>(text: &str, want: &str)
     where
         <T as FromStr>::Err: Display,
@@ -1146,11 +1166,11 @@ mod tests {
 
         // Probe limits.
         parse_ok::<Imm64>("0xffffffff_ffffffff", "-1");
-        parse_ok::<Imm64>("0x80000000_00000000", "0x8000_0000_0000_0000");
-        parse_ok::<Imm64>("-0x80000000_00000000", "0x8000_0000_0000_0000");
+        parse_ok::<Imm64>("0x80000000_00000000", "-9223372036854775808");
+        parse_ok::<Imm64>("-0x80000000_00000000", "-9223372036854775808");
         parse_err::<Imm64>("-0x80000000_00000001", "Negative number too small");
         parse_ok::<Imm64>("18446744073709551615", "-1");
-        parse_ok::<Imm64>("-9223372036854775808", "0x8000_0000_0000_0000");
+        parse_ok::<Imm64>("-9223372036854775808", "-9223372036854775808");
         // Overflow both the `checked_add` and `checked_mul`.
         parse_err::<Imm64>("18446744073709551616", "Too large decimal number");
         parse_err::<Imm64>("184467440737095516100", "Too large decimal number");

cranelift/codegen/src/ir/instructions.rs

@@ -450,21 +450,24 @@ impl InstructionData {
     }
 
     #[inline]
-    pub(crate) fn sign_extend_immediates(&mut self, ctrl_typevar: Type) {
+    pub(crate) fn mask_immediates(&mut self, ctrl_typevar: Type) {
         if ctrl_typevar.is_invalid() {
             return;
         }
 
         let bit_width = ctrl_typevar.bits();
 
         match self {
+            Self::UnaryImm { opcode: _, imm } => {
+                *imm = imm.mask_to_width(bit_width);
+            }
             Self::BinaryImm64 {
                 opcode,
                 arg: _,
                 imm,
             } => {
                 if *opcode == Opcode::SdivImm || *opcode == Opcode::SremImm {
-                    imm.sign_extend_from_width(bit_width);
+                    *imm = imm.mask_to_width(bit_width);
                 }
             }
             Self::IntCompareImm {
@@ -475,7 +478,7 @@ impl InstructionData {
             } => {
                 debug_assert_eq!(*opcode, Opcode::IcmpImm);
                 if cond.unsigned() != *cond {
-                    imm.sign_extend_from_width(bit_width);
+                    *imm = imm.mask_to_width(bit_width);
                 }
             }
             _ => {}

cranelift/codegen/src/isle_prelude.rs

@@ -193,9 +193,8 @@ macro_rules! isle_common_prelude_methods {
         }
 
         #[inline]
-        fn i64_sextend_imm64(&mut self, ty: Type, mut x: Imm64) -> i64 {
-            x.sign_extend_from_width(ty.bits());
-            x.bits()
+        fn i64_sextend_imm64(&mut self, ty: Type, x: Imm64) -> i64 {
+            x.sign_extend_from_width(ty.bits()).bits()
         }
 
         #[inline]

cranelift/codegen/src/write.rs

@@ -390,13 +390,20 @@ pub fn write_operands(w: &mut dyn Write, dfg: &DataFlowGraph, inst: Inst) -> fmt
     let pool = &dfg.value_lists;
     let jump_tables = &dfg.jump_tables;
     use crate::ir::instructions::InstructionData::*;
+    let ctrl_ty = dfg.ctrl_typevar(inst);
     match dfg.insts[inst] {
         AtomicRmw { op, args, .. } => write!(w, " {} {}, {}", op, args[0], args[1]),
         AtomicCas { args, .. } => write!(w, " {}, {}, {}", args[0], args[1], args[2]),
         LoadNoOffset { flags, arg, .. } => write!(w, "{} {}", flags, arg),
         StoreNoOffset { flags, args, .. } => write!(w, "{} {}, {}", flags, args[0], args[1]),
         Unary { arg, .. } => write!(w, " {}", arg),
-        UnaryImm { imm, .. } => write!(w, " {}", imm),
+        UnaryImm { imm, .. } => write!(w, " {}", {
+            let mut imm = imm;
+            if ctrl_ty.bits() != 0 {
+                imm = imm.sign_extend_from_width(ctrl_ty.bits());
+            }
+            imm
+        }),
         UnaryIeee16 { imm, .. } => write!(w, " {}", imm),
         UnaryIeee32 { imm, .. } => write!(w, " {}", imm),
         UnaryIeee64 { imm, .. } => write!(w, " {}", imm),
@@ -406,7 +413,13 @@ pub fn write_operands(w: &mut dyn Write, dfg: &DataFlowGraph, inst: Inst) -> fmt
         } => write!(w, " {}", constant_handle),
         Binary { args, .. } => write!(w, " {}, {}", args[0], args[1]),
         BinaryImm8 { arg, imm, .. } => write!(w, " {}, {}", arg, imm),
-        BinaryImm64 { arg, imm, .. } => write!(w, " {}, {}", arg, imm),
+        BinaryImm64 { arg, imm, .. } => write!(w, " {}, {}", arg, {
+            let mut imm = imm;
+            if ctrl_ty.bits() != 0 {
+                imm = imm.sign_extend_from_width(ctrl_ty.bits());
+            }
+            imm
+        }),
         Ternary { args, .. } => write!(w, " {}, {}, {}", args[0], args[1], args[2]),
         MultiAry { ref args, .. } => {
             if args.is_empty() {
@@ -424,7 +437,13 @@ pub fn write_operands(w: &mut dyn Write, dfg: &DataFlowGraph, inst: Inst) -> fmt
             write!(w, " {}, {}, {}", args[0], args[1], data)
         }
         IntCompare { cond, args, .. } => write!(w, " {} {}, {}", cond, args[0], args[1]),
-        IntCompareImm { cond, arg, imm, .. } => write!(w, " {} {}, {}", cond, arg, imm),
+        IntCompareImm { cond, arg, imm, .. } => write!(w, " {} {}, {}", cond, arg, {
+            let mut imm = imm;
+            if ctrl_ty.bits() != 0 {
+                imm = imm.sign_extend_from_width(ctrl_ty.bits());
+            }
+            imm
+        }),
         IntAddTrap { args, code, .. } => write!(w, " {}, {}, {}", args[0], args[1], code),
         FloatCompare { cond, args, .. } => write!(w, " {} {}, {}", cond, args[0], args[1]),
         Jump { destination, .. } => {
@@ -495,7 +514,13 @@ pub fn write_operands(w: &mut dyn Write, dfg: &DataFlowGraph, inst: Inst) -> fmt
     for arg in dfg.inst_values(inst) {
         if let ValueDef::Result(src, _) = dfg.value_def(arg) {
             let imm = match dfg.insts[src] {
-                UnaryImm { imm, .. } => imm.to_string(),
+                UnaryImm { imm, .. } => {
+                    let mut imm = imm;
+                    if dfg.ctrl_typevar(src).bits() != 0 {
+                        imm = imm.sign_extend_from_width(dfg.ctrl_typevar(src).bits());
+                    }
+                    imm.to_string()
+                }
                 UnaryIeee16 { imm, .. } => imm.to_string(),
                 UnaryIeee32 { imm, .. } => imm.to_string(),
                 UnaryIeee64 { imm, .. } => imm.to_string(),

cranelift/filetests/filetests/egraph/cprop.clif

@@ -19,7 +19,7 @@ block0:
     return v2
 }
 
-; check: v3 = iconst.i16 0xfffe
+; check: v3 = iconst.i16 -2
 ; nextln: return v3
 
 function %ishl() -> i8 {
@@ -96,7 +96,7 @@ block0:
     return v2
 }
 
-; check: v3 = iconst.i8 252
+; check: v3 = iconst.i8 -4
 ; check: return v3
 
 function %sshr_i8_i16() -> i8 {
@@ -107,7 +107,7 @@ block0:
     return v2
 }
 
-; check: v3 = iconst.i8 252
+; check: v3 = iconst.i8 -4
 ; check: return v3
 
 function %sshr_i16_i8() -> i16 {
@@ -118,7 +118,7 @@ block0:
     return v2
 }
 
-; check: v3 = iconst.i16 0xfffc
+; check: v3 = iconst.i16 -4
 ; check: return v3
 
 function %icmp_eq_i32() -> i8 {
@@ -238,7 +238,7 @@ block0:
     return v2
 }
 
-; check: v3 = iconst.i8 246
+; check: v3 = iconst.i8 -10
 ; nextln: return v3
 
 function %sextend_iconst() -> i32 {
@@ -248,7 +248,7 @@ block0:
     return v2
 }
 
-; check: v3 = iconst.i32 0xffff_fff6
+; check: v3 = iconst.i32 -10
 ; nextln: return v3
 
 function %uextend_iconst() -> i32 {
@@ -310,7 +310,7 @@ block0:
     return v1
 }
 
-; check: v2 = iconst.i64 0xf0de_bc9a_7856_3412
+; check: v2 = iconst.i64 -1090226688147180526
 ; nextln: return v2
 
 function %f16_fmin() -> f16 {