Do value-extensions at ABI boundaries only when ABI requires it.

There has been some confusion over the meaning of the "sign-extend"
(`sext`) and "zero-extend" (`uext`) attributes on parameters and return
values in signatures. According to the three implemented backends, these
attributes indicate that a value narrower than a full register should
always be extended in the way specified. However, they are much more
useful if they mean "extend in this way if the ABI requires extending":
only the ABI backend knows whether or not a particular ABI (e.g., x64
SysV vs. x64 Baldrdash) requires extensions, while only the frontend
(CLIF generator) knows whether or not a value is signed, so the two have
to work in concert.

This is the result of some very helpful discussion in #2354 (thanks to
@uweigand for raising the issue and @bjorn3 for helping to reason about
it).

This change respects the extension attributes in the above way, rather
than unconditionally extending, to avoid potential performance
degradation as we introduce more extension attributes on signatures.

cranelift/codegen/src/ir/extfunc.rs

@@ -256,8 +256,19 @@ impl fmt::Display for AbiParam {
 
 /// Function argument extension options.
 ///
-/// On some architectures, small integer function arguments are extended to the width of a
+/// On some architectures, small integer function arguments and/or return values are extended to
-/// general-purpose register.
+/// the width of a general-purpose register.
+///
+/// This attribute specifies how an argument or return value should be extended *if the platform
+/// and ABI require it*. Because the frontend (CLIF generator) does not know anything about the
+/// particulars of the target's ABI, and the CLIF should be platform-independent, these attributes
+/// specify *how* to extend (according to the signedness of the original program) rather than
+/// *whether* to extend.
+///
+/// For example, on x86-64, the SystemV ABI does not require extensions of narrow values, so these
+/// `ArgumentExtension` attributes are ignored; but in the Baldrdash (SpiderMonkey) ABI on the same
+/// platform, all narrow values *are* extended, so these attributes may lead to extra
+/// zero/sign-extend instructions in the generated machine code.
 #[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)]
 #[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
 pub enum ArgumentExtension {

cranelift/codegen/src/isa/aarch64/abi.rs

@@ -736,6 +736,19 @@ impl ABIMachineSpec for AArch64MachineDeps {
         }
         caller_saved
     }
+
+    fn get_ext_mode(
+        call_conv: isa::CallConv,
+        specified: ir::ArgumentExtension,
+    ) -> ir::ArgumentExtension {
+        if call_conv.extends_baldrdash() {
+            // Baldrdash (SpiderMonkey) always extends args and return values to the full register.
+            specified
+        } else {
+            // No other supported ABI on AArch64 does so.
+            ir::ArgumentExtension::None
+        }
+    }
 }
 
 /// Is this type supposed to be seen on this machine? E.g. references of the

cranelift/codegen/src/isa/arm32/abi.rs

@@ -451,6 +451,13 @@ impl ABIMachineSpec for Arm32MachineDeps {
         }
         caller_saved
     }
+
+    fn get_ext_mode(
+        _call_conv: isa::CallConv,
+        specified: ir::ArgumentExtension,
+    ) -> ir::ArgumentExtension {
+        specified
+    }
 }
 
 fn is_callee_save(r: RealReg) -> bool {

cranelift/codegen/src/isa/x64/abi.rs

@@ -597,6 +597,19 @@ impl ABIMachineSpec for X64ABIMachineSpec {
 
         caller_saved
     }
+
+    fn get_ext_mode(
+        call_conv: isa::CallConv,
+        specified: ir::ArgumentExtension,
+    ) -> ir::ArgumentExtension {
+        if call_conv.extends_baldrdash() {
+            // Baldrdash (SpiderMonkey) always extends args and return values to the full register.
+            specified
+        } else {
+            // No other supported ABI on x64 does so.
+            ir::ArgumentExtension::None
+        }
+    }
 }
 
 impl From<StackAMode> for SyntheticAmode {

cranelift/codegen/src/isa/x86/abi.rs

@@ -144,8 +144,13 @@ impl ArgAssigner for Args {
             return ValueConversion::VectorSplit.into();
         }
 
-        // Small integers are extended to the size of a pointer register.
+        // Small integers are extended to the size of a pointer register, but
-        if ty.is_int() && ty.bits() < u16::from(self.pointer_bits) {
+        // only in ABIs that require this. The Baldrdash (SpiderMonkey) ABI
+        // does, but our other supported ABIs on x86 do not.
+        if ty.is_int()
+            && ty.bits() < u16::from(self.pointer_bits)
+            && self.call_conv.extends_baldrdash()
+        {
             match arg.extension {
                 ArgumentExtension::None => {}
                 ArgumentExtension::Uext => return ValueConversion::Uext(self.pointer_type).into(),

cranelift/codegen/src/machinst/abi_impl.rs

@@ -370,6 +370,16 @@ pub trait ABIMachineSpec {
     /// Get all caller-save registers, that is, registers that we expect
     /// not to be saved across a call to a callee with the given ABI.
     fn get_regs_clobbered_by_call(call_conv_of_callee: isa::CallConv) -> Vec<Writable<Reg>>;
+
+    /// Get the needed extension mode, given the mode attached to the argument
+    /// in the signature and the calling convention. The input (the attribute in
+    /// the signature) specifies what extension type should be done *if* the ABI
+    /// requires extension to the full register; this method's return value
+    /// indicates whether the extension actually *will* be done.
+    fn get_ext_mode(
+        call_conv: isa::CallConv,
+        specified: ir::ArgumentExtension,
+    ) -> ir::ArgumentExtension;
 }
 
 /// ABI information shared between body (callee) and caller.
@@ -771,6 +781,7 @@ impl<M: ABIMachineSpec> ABICallee for ABICalleeImpl<M> {
             &ABIArg::Reg(r, ty, ext, ..) => {
                 let from_bits = ty_bits(ty) as u8;
                 let dest_reg = Writable::from_reg(r.to_reg());
+                let ext = M::get_ext_mode(self.sig.call_conv, ext);
                 match (ext, from_bits) {
                     (ArgumentExtension::Uext, n) | (ArgumentExtension::Sext, n)
                         if n < word_bits =>
@@ -794,6 +805,7 @@ impl<M: ABIMachineSpec> ABICallee for ABICalleeImpl<M> {
                 // backends (aarch64 and x64) enforce a 128MB limit.
                 let off = i32::try_from(off)
                     .expect("Argument stack offset greater than 2GB; should hit impl limit first");
+                let ext = M::get_ext_mode(self.sig.call_conv, ext);
                 // Trash the from_reg; it should be its last use.
                 match (ext, from_bits) {
                     (ArgumentExtension::Uext, n) | (ArgumentExtension::Sext, n)
@@ -1218,9 +1230,9 @@ impl<M: ABIMachineSpec> ABICaller for ABICallerImpl<M> {
         let word_rc = M::word_reg_class();
         let word_bits = M::word_bits() as usize;
         match &self.sig.args[idx] {
-            &ABIArg::Reg(reg, ty, ext, _)
+            &ABIArg::Reg(reg, ty, ext, _) => {
-                if ext != ir::ArgumentExtension::None && ty_bits(ty) < word_bits =>
+                let ext = M::get_ext_mode(self.sig.call_conv, ext);
-            {
+                if ext != ir::ArgumentExtension::None && ty_bits(ty) < word_bits {
                     assert_eq!(word_rc, reg.get_class());
                     let signed = match ext {
                         ir::ArgumentExtension::Uext => false,
@@ -1234,11 +1246,12 @@ impl<M: ABIMachineSpec> ABICaller for ABICallerImpl<M> {
                         ty_bits(ty) as u8,
                         word_bits as u8,
                     ));
-            }
+                } else {
-            &ABIArg::Reg(reg, ty, _, _) => {
                     ctx.emit(M::gen_move(Writable::from_reg(reg.to_reg()), from_reg, ty));
                 }
+            }
             &ABIArg::Stack(off, mut ty, ext, _) => {
+                let ext = M::get_ext_mode(self.sig.call_conv, ext);
                 if ext != ir::ArgumentExtension::None && ty_bits(ty) < word_bits {
                     assert_eq!(word_rc, from_reg.get_class());
                     let signed = match ext {

cranelift/filetests/filetests/isa/aarch64/call.clif

@@ -1,4 +1,5 @@
 test compile
+set enable_probestack=false
 target aarch64
 
 function %f1(i64) -> i64 {
@@ -18,7 +19,7 @@ block0(v0: i64):
 ; nextln:  ret
 
 function %f2(i32) -> i64 {
-    fn0 = %g(i32 uext) -> i64
+    fn0 = %g(i32 uext) -> i64 baldrdash_system_v
 
 block0(v0: i32):
     v1 = call fn0(v0)
@@ -27,27 +28,22 @@ block0(v0: i32):
 
 ; check:  stp fp, lr, [sp, #-16]!
 ; nextln:  mov fp, sp
-; nextln:  mov w0, w0
+; check:  mov w0, w0
 ; nextln:  ldr x1, 8 ; b 12 ; data
 ; nextln:  blr x1
-; nextln:  mov sp, fp
+; check:  mov sp, fp
 ; nextln:  ldp fp, lr, [sp], #16
 ; nextln:  ret
 
-function %f3(i32) -> i32 uext {
+function %f3(i32) -> i32 uext baldrdash_system_v {
 block0(v0: i32):
     return v0
 }
 
-; check:  stp fp, lr, [sp, #-16]!
+; check:  mov w0, w0
-; nextln:  mov fp, sp
-; nextln:  mov w0, w0
-; nextln:  mov sp, fp
-; nextln:  ldp fp, lr, [sp], #16
-; nextln:  ret
 
 function %f4(i32) -> i64 {
-    fn0 = %g(i32 sext) -> i64
+    fn0 = %g(i32 sext) -> i64 baldrdash_system_v
 
 block0(v0: i32):
     v1 = call fn0(v0)
@@ -56,24 +52,19 @@ block0(v0: i32):
 
 ; check:  stp fp, lr, [sp, #-16]!
 ; nextln:  mov fp, sp
-; nextln:  sxtw x0, w0
+; check:  sxtw x0, w0
 ; nextln:  ldr x1, 8 ; b 12 ; data
 ; nextln:  blr x1
-; nextln:  mov sp, fp
+; check:  mov sp, fp
 ; nextln:  ldp fp, lr, [sp], #16
 ; nextln:  ret
 
-function %f5(i32) -> i32 sext {
+function %f5(i32) -> i32 sext baldrdash_system_v {
 block0(v0: i32):
     return v0
 }
 
-; check:  stp fp, lr, [sp, #-16]!
+; check:  sxtw x0, w0
-; nextln:  mov fp, sp
-; nextln:  sxtw x0, w0
-; nextln:  mov sp, fp
-; nextln:  ldp fp, lr, [sp], #16
-; nextln:  ret
 
 function %f6(i8) -> i64 {
     fn0 = %g(i32, i32, i32, i32, i32, i32, i32, i32, i8 sext) -> i64
@@ -97,8 +88,7 @@ block0(v0: i8):
 ; nextln:  movz x5, #42
 ; nextln:  movz x6, #42
 ; nextln:  movz x7, #42
-; nextln:  sxtb x8, w8
+; nextln:  sturb w8, [sp]
-; nextln:  stur x8, [sp]
 ; nextln:  ldr x8, 8 ; b 12 ; data
 ; nextln:  blr x8
 ; nextln:  add sp, sp, #16
@@ -125,8 +115,7 @@ block0(v0: i8):
 ; nextln:  movz x5, #42
 ; nextln:  movz x6, #42
 ; nextln:  movz x7, #42
-; nextln:  sxtb x9, w9
+; nextln:  sturb w9, [x8]
-; nextln:  stur x9, [x8]
 ; nextln:  mov sp, fp
 ; nextln:  ldp fp, lr, [sp], #16
 ; nextln:  ret