Repository: bytecodealliance/wasmtime
Pull request #9075 (2024-08-03 22:16:16)

pulley: sign-extend `xconst` immediates
Change `xconst8`, `xconst16`, `xconst32` and `xconst64` instructions to sign-extend rather than zero-extend their immediates.

Kmeakin: > Would you be up for switching the `xconst*` instructions to sign extending instead of zero extending? I think it probably doesn't makes sense to burn the opcode space on both sign- and zero-extending constant instructions, and sign extending is a little more flexible in general, so I think it would be better to only have sign-extending constant instructions.

Sure

Patch:
diff --git a/pulley/README.md b/pulley/README.md
index a20cde1da..c55377299 100644
--- a/pulley/README.md
+++ b/pulley/README.md
@@ -47,26 +47,24 @@ to change, instructions to appear and disappear, and APIs to be overhauled.
 Here is the disassembly of `f(a, b) = a + b` in Pulley today:
 
 ```
-       0: 11 1f f0 ff ff ff ff ff ff ff   xconst64 x31, 18446744073709551600
-       a: 12 20 20 1f                     xadd32 sp, sp, x31
-       e: 32 20 08 21                     store64_offset8 sp, 8, lr
-      12: 30 20 22                        store64 sp, fp
-      15: 0b 22 20                        xmov fp, sp
-      18: 12 00 00 01                     xadd32 x0, x0, x1
-      1c: 0b 20 22                        xmov sp, fp
-      1f: 25 21 20 08                     load64_offset8 lr, sp, 8
-      23: 22 22 20                        load64 fp, sp
-      26: 0e 1f 10                        xconst8 x31, 16
-      29: 12 20 20 1f                     xadd32 sp, sp, x31
-      2d: 00                              ret
+       0: 0e 1f f0                        xconst8 x31, -16
+       3: 12 20 20 1f                     xadd32 sp, sp, x31
+       7: 29 20 08 21                     store64_offset8 sp, 8, lr
+       b: 27 20 22                        store64 sp, fp
+       e: 0b 22 20                        xmov fp, sp
+      11: 12 00 00 01                     xadd32 x0, x0, x1
+      15: 0b 20 22                        xmov sp, fp
+      18: 25 21 20 08                     load64_offset8 lr, sp, 8
+      1c: 22 22 20                        load64 fp, sp
+      1f: 0e 1f 10                        xconst8 x31, 16
+      22: 12 20 20 1f                     xadd32 sp, sp, x31
+      26: 00                              ret
 ```
 
 Note that there are a number of things that could be improved here:
 
-* We could avoid allocating a deallocating a stack frame because this function's
+* We could avoid allocating and deallocating a stack frame because this function's
   body doesn't use any stack slots.
-* We could sign-extend, rather than zero-extend, constants so that `-16` has a
-  single-byte encoding instead of an eight-byte encoding.
 * We could collapse the whole prologue and epilogue instruction sequences into
   super-instructions, since they are identical (modulo the frame size immediate)
   for all functions.
diff --git a/pulley/src/interp.rs b/pulley/src/interp.rs
index d341aa7ec..ad6ea2427 100644
--- a/pulley/src/interp.rs
+++ b/pulley/src/interp.rs
@@ -788,23 +788,23 @@ impl OpVisitor for InterpreterVisitor<'_> {
         Continuation::Continue
     }
 
-    fn xconst8(&mut self, dst: XReg, imm: u8) -> Self::Return {
-        self.state.x_mut(dst).set_u64(u64::from(imm));
+    fn xconst8(&mut self, dst: XReg, imm: i8) -> Self::Return {
+        self.state.x_mut(dst).set_i64(i64::from(imm));
         Continuation::Continue
     }
 
-    fn xconst16(&mut self, dst: XReg, imm: u16) -> Self::Return {
-        self.state.x_mut(dst).set_u64(u64::from(imm));
+    fn xconst16(&mut self, dst: XReg, imm: i16) -> Self::Return {
+        self.state.x_mut(dst).set_i64(i64::from(imm));
         Continuation::Continue
     }
 
-    fn xconst32(&mut self, dst: XReg, imm: u32) -> Self::Return {
-        self.state.x_mut(dst).set_u64(u64::from(imm));
+    fn xconst32(&mut self, dst: XReg, imm: i32) -> Self::Return {
+        self.state.x_mut(dst).set_i64(i64::from(imm));
         Continuation::Continue
     }
 
-    fn xconst64(&mut self, dst: XReg, imm: u64) -> Self::Return {
-        self.state.x_mut(dst).set_u64(imm);
+    fn xconst64(&mut self, dst: XReg, imm: i64) -> Self::Return {
+        self.state.x_mut(dst).set_i64(imm);
         Continuation::Continue
     }
 
diff --git a/pulley/src/lib.rs b/pulley/src/lib.rs
index b0a62dc1c..30485f643 100644
--- a/pulley/src/lib.rs
+++ b/pulley/src/lib.rs
@@ -57,14 +57,14 @@ macro_rules! for_each_op {
             /// Move between `v` registers.
             vmov = Vmov { dst: VReg, src: VReg };
 
-            /// Set `dst = zero_extend(imm8)`.
-            xconst8 = Xconst8 { dst: XReg, imm: u8 };
-            /// Set `dst = zero_extend(imm16)`.
-            xconst16 = Xconst16 { dst: XReg, imm: u16 };
-            /// Set `dst = zero_extend(imm32)`.
-            xconst32 = Xconst32 { dst: XReg, imm: u32 };
+            /// Set `dst = sign_extend(imm8)`.
+            xconst8 = Xconst8 { dst: XReg, imm: i8 };
+            /// Set `dst = sign_extend(imm16)`.
+            xconst16 = Xconst16 { dst: XReg, imm: i16 };
+            /// Set `dst = sign_extend(imm32)`.
+            xconst32 = Xconst32 { dst: XReg, imm: i32 };
             /// Set `dst = imm64`.
-            xconst64 = Xconst64 { dst: XReg, imm: u64 };
+            xconst64 = Xconst64 { dst: XReg, imm: i64 };
 
             /// 32-bit wrapping addition: `low32(dst) = low32(src1) + low32(src2)`.
             ///
diff --git a/pulley/src/regs.rs b/pulley/src/regs.rs
index a7339f699..c8260f7ff 100644
--- a/pulley/src/regs.rs
+++ b/pulley/src/regs.rs
@@ -156,12 +156,12 @@ impl XReg {
 
 impl fmt::Display for XReg {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            x if *x == Self::SP => write!(f, "sp"),
-            x if *x == Self::LR => write!(f, "lr"),
-            x if *x == Self::FP => write!(f, "fp"),
-            x if *x == Self::SPILL_TMP_0 => write!(f, "spilltmp0"),
-            x if *x == Self::SPILL_TMP_1 => write!(f, "spilltmp1"),
+        match *self {
+            Self::SP => write!(f, "sp"),
+            Self::LR => write!(f, "lr"),
+            Self::FP => write!(f, "fp"),
+            Self::SPILL_TMP_0 => write!(f, "spilltmp0"),
+            Self::SPILL_TMP_1 => write!(f, "spilltmp1"),
             Self(x) => write!(f, "x{x}"),
         }
     }


Test patch:
diff --git a/pulley/tests/all/disas.rs b/pulley/tests/all/disas.rs
index b91ecf224..ca0f1e5bb 100644
--- a/pulley/tests/all/disas.rs
+++ b/pulley/tests/all/disas.rs
@@ -33,9 +33,9 @@ fn simple() {
     assert_disas(
         &[
             // Prologue.
-            Op::Xconst64(Xconst64 {
+            Op::Xconst8(Xconst8 {
                 dst: x31,
-                imm: -16i64 as u64,
+                imm: -16i8,
             }),
             Op::Xadd32(Xadd32 {
                 dst: XReg::SP,
@@ -84,18 +84,18 @@ fn simple() {
             Op::Ret(Ret {}),
         ],
         r#"
-       0: 11 1f f0 ff ff ff ff ff ff ff   xconst64 x31, 18446744073709551600
-       a: 12 20 20 1f                     xadd32 sp, sp, x31
-       e: 29 20 08 21                     store64_offset8 sp, 8, lr
-      12: 27 20 22                        store64 sp, fp
-      15: 0b 22 20                        xmov fp, sp
-      18: 12 00 00 01                     xadd32 x0, x0, x1
-      1c: 0b 20 22                        xmov sp, fp
-      1f: 25 21 20 08                     load64_offset8 lr, sp, 8
-      23: 22 22 20                        load64 fp, sp
-      26: 0e 1f 10                        xconst8 x31, 16
-      29: 12 20 20 1f                     xadd32 sp, sp, x31
-      2d: 00                              ret
+       0: 0e 1f f0                        xconst8 x31, -16
+       3: 12 20 20 1f                     xadd32 sp, sp, x31
+       7: 29 20 08 21                     store64_offset8 sp, 8, lr
+       b: 27 20 22                        store64 sp, fp
+       e: 0b 22 20                        xmov fp, sp
+      11: 12 00 00 01                     xadd32 x0, x0, x1
+      15: 0b 20 22                        xmov sp, fp
+      18: 25 21 20 08                     load64_offset8 lr, sp, 8
+      1c: 22 22 20                        load64 fp, sp
+      1f: 0e 1f 10                        xconst8 x31, 16
+      22: 12 20 20 1f                     xadd32 sp, sp, x31
+      26: 00                              ret
         "#,
     );
 }
diff --git a/pulley/tests/all/interp.rs b/pulley/tests/all/interp.rs
index 57946f395..c9295ac6f 100644
--- a/pulley/tests/all/interp.rs
+++ b/pulley/tests/all/interp.rs
@@ -72,7 +72,7 @@ fn f(f: u8) -> FReg {
 
 #[test]
 fn xconst8() {
-    for (expected, imm) in [(42u64, 42u8), (u64::from(u8::MAX), u8::MAX)] {
+    for (expected, imm) in [(42u64, 42i8), (u64::MAX, -1i8)] {
         unsafe {
             assert_one(
                 [(x(0), 0x1234567812345678u64)],
@@ -86,7 +86,7 @@ fn xconst8() {
 
 #[test]
 fn xconst16() {
-    for (expected, imm) in [(42u64, 42u16), (u64::from(u16::MAX), u16::MAX)] {
+    for (expected, imm) in [(42u64, 42i16), (u64::MAX, -1i16)] {
         unsafe {
             assert_one(
                 [(x(0), 0x1234567812345678u64)],
@@ -100,7 +100,7 @@ fn xconst16() {
 
 #[test]
 fn xconst32() {
-    for (expected, imm) in [(42u64, 42u32), (u64::from(u32::MAX), u32::MAX)] {
+    for (expected, imm) in [(42u64, 42i32), (u64::MAX, -1i32)] {
         unsafe {
             assert_one(
                 [(x(0), 0x1234567812345678u64)],
@@ -114,7 +114,7 @@ fn xconst32() {
 
 #[test]
 fn xconst64() {
-    for (expected, imm) in [(42u64, 42u64), (u64::MAX, u64::MAX)] {
+    for (expected, imm) in [(42u64, 42i64), (u64::MAX, -1i64)] {
         unsafe {
             assert_one(
                 [(x(0), 0x1234567812345678u64)],