Gate support for implicit return area pointers behind an option (byte…

…codealliance#9511)

* Remove couple of unused isle helpers

* Gate support for implicit return area pointers behind an option

It implements an incorrect ABI and may be removed in the future due to
complexity reasons. By requiring to enable an option to use it, it
becomes harder to accidentally hit the ABI issue and allows a more
deprecation and eventual removal.

* Fix review comments

* Enable enable_multi_ret_implicit_sret for s390x tests that use i128

* Enable enable_multi_ret_implicit_sret for riscv tests that use vector types

* Enable enable_multi_ret_implicit_sret for more riscv tests

* Enable enable_multi_ret_implicit_sret for windows tests that use i128

cranelift/codegen/meta/src/shared/settings.rs

@@ -208,6 +208,35 @@ pub(crate) fn define() -> SettingGroup {
         false,
     );
 
+    settings.add_bool(
+        "enable_multi_ret_implicit_sret",
+        "Enable support for sret arg introduction when there are too many ret vals.",
+        r#"
+            When there are more returns than available return registers, the
+            return value has to be returned through the introduction of a
+            return area pointer. Normally this return area pointer has to be
+            introduced as `ArgumentPurpose::StructReturn` parameter, but for
+            backward compatibility reasons Cranelift also supports implicitly
+            introducing this parameter and writing the return values through it.
+
+            **This option currently does not conform to platform ABIs and the
+            used ABI should not be assumed to remain the same between Cranelift
+            versions.**
+
+            This option is **deprecated** and will be removed in the future.
+
+            Because of the above issues, and complexities of native ABI support
+            for the concept in general, Cranelift's support for multiple return
+            values may also be removed in the future (#9510). For the most
+            robust solution, it is recommended to build a convention on top of
+            Cranelift's primitives for passing multiple return values, for
+            example by allocating a stackslot in the caller, passing it as an
+            explicit StructReturn argument, storing return values in the callee,
+            and loading results in the caller.
+        "#,
+        false,
+    );
+
     settings.add_bool(
         "unwind_info",
         "Generate unwind information.",

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

@@ -16,6 +16,7 @@ use alloc::boxed::Box;
 use alloc::vec::Vec;
 use regalloc2::{MachineEnv, PReg, PRegSet};
 use smallvec::{smallvec, SmallVec};
+use std::borrow::ToOwned;
 use std::sync::OnceLock;
 
 // We use a generic implementation that factors out AArch64 and x64 ABI commonalities, because
@@ -297,7 +298,7 @@ impl ABIMachineSpec for AArch64MachineDeps {
                 };
 
                 let push_to_reg = if is_winch_return {
-                    // Winch uses the first registry to return the last result
+                    // Winch uses the first register to return the last result
                     i == params.len() - 1
                 } else {
                     // Use max_per_class_reg_vals & remaining_reg_vals otherwise
@@ -330,6 +331,14 @@ impl ABIMachineSpec for AArch64MachineDeps {
 
             // Spill to the stack
 
+            if args_or_rets == ArgsOrRets::Rets && !flags.enable_multi_ret_implicit_sret() {
+                return Err(crate::CodegenError::Unsupported(
+                    "Too many return values to fit in registers. \
+                    Use a StructReturn argument instead. (#9510)"
+                        .to_owned(),
+                ));
+            }
+
             // Compute the stack slot's size.
             let size = (ty_bits(param.value_type) / 8) as u32;
 

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

@@ -12,6 +12,7 @@ use alloc::{boxed::Box, vec::Vec};
 use core::marker::PhantomData;
 use regalloc2::{MachineEnv, PReg, PRegSet};
 use smallvec::{smallvec, SmallVec};
+use std::borrow::ToOwned;
 use std::sync::OnceLock;
 
 /// Support for the Pulley ABI from the callee side (within a function body).
@@ -52,7 +53,7 @@ where
 
     fn compute_arg_locs(
         call_conv: isa::CallConv,
-        _flags: &settings::Flags,
+        flags: &settings::Flags,
         params: &[ir::AbiParam],
         args_or_rets: ArgsOrRets,
         add_ret_area_ptr: bool,
@@ -113,6 +114,14 @@ where
                         extension: param.extension,
                     });
                 } else {
+                    if args_or_rets == ArgsOrRets::Rets && !flags.enable_multi_ret_implicit_sret() {
+                        return Err(crate::CodegenError::Unsupported(
+                            "Too many return values to fit in registers. \
+                            Use a StructReturn argument instead. (#9510)"
+                                .to_owned(),
+                        ));
+                    }
+
                     // Compute size and 16-byte stack alignment happens
                     // separately after all args.
                     let size = reg_ty.bits() / 8;

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

@@ -20,6 +20,7 @@ use alloc::vec::Vec;
 use regalloc2::{MachineEnv, PReg, PRegSet};
 
 use smallvec::{smallvec, SmallVec};
+use std::borrow::ToOwned;
 use std::sync::OnceLock;
 
 /// Support for the Riscv64 ABI from the callee side (within a function body).
@@ -88,7 +89,7 @@ impl ABIMachineSpec for Riscv64MachineDeps {
 
     fn compute_arg_locs(
         call_conv: isa::CallConv,
-        _flags: &settings::Flags,
+        flags: &settings::Flags,
         params: &[ir::AbiParam],
         args_or_rets: ArgsOrRets,
         add_ret_area_ptr: bool,
@@ -154,6 +155,14 @@ impl ABIMachineSpec for Riscv64MachineDeps {
                         extension: param.extension,
                     });
                 } else {
+                    if args_or_rets == ArgsOrRets::Rets && !flags.enable_multi_ret_implicit_sret() {
+                        return Err(crate::CodegenError::Unsupported(
+                            "Too many return values to fit in registers. \
+                            Use a StructReturn argument instead. (#9510)"
+                                .to_owned(),
+                        ));
+                    }
+
                     // Compute size and 16-byte stack alignment happens
                     // separately after all args.
                     let size = reg_ty.bits() / 8;

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

@@ -147,6 +147,7 @@ use crate::CodegenResult;
 use alloc::vec::Vec;
 use regalloc2::{MachineEnv, PRegSet};
 use smallvec::{smallvec, SmallVec};
+use std::borrow::ToOwned;
 use std::sync::OnceLock;
 
 // We use a generic implementation that factors out ABI commonalities.
@@ -290,7 +291,7 @@ impl ABIMachineSpec for S390xMachineDeps {
 
     fn compute_arg_locs(
         call_conv: isa::CallConv,
-        _flags: &settings::Flags,
+        flags: &settings::Flags,
         params: &[ir::AbiParam],
         args_or_rets: ArgsOrRets,
         add_ret_area_ptr: bool,
@@ -384,6 +385,14 @@ impl ABIMachineSpec for S390xMachineDeps {
                     extension: param.extension,
                 }
             } else {
+                if args_or_rets == ArgsOrRets::Rets && !flags.enable_multi_ret_implicit_sret() {
+                    return Err(crate::CodegenError::Unsupported(
+                        "Too many return values to fit in registers. \
+                        Use a StructReturn argument instead. (#9510)"
+                            .to_owned(),
+                    ));
+                }
+
                 // Compute size. Every argument or return value takes a slot of
                 // at least 8 bytes.
                 let size = (ty_bits(param.value_type) / 8) as u32;

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

@@ -15,6 +15,7 @@ use alloc::vec::Vec;
 use args::*;
 use regalloc2::{MachineEnv, PReg, PRegSet};
 use smallvec::{smallvec, SmallVec};
+use std::borrow::ToOwned;
 use std::sync::OnceLock;
 
 /// Support for the x64 ABI from the callee side (within a function body).
@@ -337,6 +338,14 @@ impl ABIMachineSpec for X64ABIMachineSpec {
                         extension: param.extension,
                     });
                 } else {
+                    if args_or_rets == ArgsOrRets::Rets && !flags.enable_multi_ret_implicit_sret() {
+                        return Err(crate::CodegenError::Unsupported(
+                            "Too many return values to fit in registers. \
+                            Use a StructReturn argument instead. (#9510)"
+                                .to_owned(),
+                        ));
+                    }
+
                     let size = reg_ty.bytes();
                     let size = if call_conv == CallConv::Winch
                         && args_or_rets == ArgsOrRets::Rets

cranelift/codegen/src/machinst/abi.rs

@@ -862,6 +862,9 @@ impl SigSet {
             /* extra ret-area ptr = */ false,
             ArgsAccumulator::new(&mut self.abi_args),
         )?;
+        if !flags.enable_multi_ret_implicit_sret() {
+            assert_eq!(sized_stack_ret_space, 0);
+        }
         let rets_end = u32::try_from(self.abi_args.len()).unwrap();
 
         // To avoid overflow issues, limit the return size to something reasonable.

cranelift/codegen/src/machinst/isle.rs

@@ -470,14 +470,6 @@ macro_rules! isle_lower_prelude_methods {
             }
         }
 
-        fn abi_sized_stack_arg_space(&mut self, abi: Sig) -> i64 {
-            self.lower_ctx.sigs()[abi].sized_stack_arg_space()
-        }
-
-        fn abi_sized_stack_ret_space(&mut self, abi: Sig) -> i64 {
-            self.lower_ctx.sigs()[abi].sized_stack_ret_space()
-        }
-
         fn abi_arg_only_slot(&mut self, arg: &ABIArg) -> Option<ABIArgSlot> {
             match arg {
                 &ABIArg::Slots { ref slots, .. } => {

cranelift/codegen/src/prelude_lower.isle

@@ -314,7 +314,7 @@
 
 ;; Extract a constant `i32` from a value defined by an `iconst`.
 ;; The value is sign extended to 32 bits.
-(spec (i32_from_iconst arg) 
+(spec (i32_from_iconst arg)
       (provide (= arg (extract 31 0 (sign_ext 64 result))))
       (require (= result (sign_ext (widthof result) arg))))
 (decl i32_from_iconst (i32) Value)
@@ -1044,14 +1044,6 @@
 (decl abi_no_ret_arg () Sig)
 (extern extractor abi_no_ret_arg abi_no_ret_arg)
 
-;; Size of the argument area.
-(decl abi_sized_stack_arg_space (Sig) i64)
-(extern constructor abi_sized_stack_arg_space abi_sized_stack_arg_space)
-
-;; Size of the return-value area.
-(decl abi_sized_stack_ret_space (Sig) i64)
-(extern constructor abi_sized_stack_ret_space abi_sized_stack_ret_space)
-
 ;; Incoming return area pointer (must be present).
 (decl abi_unwrap_ret_area_ptr () Reg)
 (extern constructor abi_unwrap_ret_area_ptr abi_unwrap_ret_area_ptr)

cranelift/codegen/src/settings.rs

@@ -529,6 +529,7 @@ enable_pinned_reg = false
 enable_atomics = true
 enable_safepoints = false
 enable_llvm_abi_extensions = false
+enable_multi_ret_implicit_sret = false
 unwind_info = true
 preserve_frame_pointers = false
 machine_code_cfg_info = false

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

@@ -2,6 +2,7 @@ test compile precise-output
 set unwind_info=false
 set enable_probestack=false
 set enable_llvm_abi_extensions
+set enable_multi_ret_implicit_sret
 target aarch64
 
 function %f1(i64) -> i64 {

cranelift/filetests/filetests/isa/aarch64/tail-call-conv.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 target aarch64
 
 ;; Test the `tail` calling convention with non-tail calls and stack arguments.

cranelift/filetests/filetests/isa/pulley32/call.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 target pulley32
 
 function %colocated_args_i64_rets_i64() -> i64 {

cranelift/filetests/filetests/isa/pulley64/call.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 target pulley64
 
 function %colocated_args_i64_rets_i64() -> i64 {

cranelift/filetests/filetests/isa/riscv64/bitcast-scalar-vector.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64 has_v
 

cranelift/filetests/filetests/isa/riscv64/call.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64
 

cranelift/filetests/filetests/isa/riscv64/issue-6954.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 target riscv64gc has_v has_zbkb has_zba has_zbb has_zbc has_zbs
 
 

cranelift/filetests/filetests/isa/riscv64/issue8847.clif

@@ -4,6 +4,7 @@ test compile
 set bb_padding_log2_minus_one=4
 set enable_alias_analysis=false
 set enable_llvm_abi_extensions=true
+set enable_multi_ret_implicit_sret
 set machine_code_cfg_info=true
 set enable_jump_tables=false
 set enable_heap_access_spectre_mitigation=false

cranelift/filetests/filetests/isa/riscv64/issue8866.clif

@@ -4,6 +4,7 @@ test compile
 set bb_padding_log2_minus_one=4
 set enable_alias_analysis=false
 set enable_llvm_abi_extensions=true
+set enable_multi_ret_implicit_sret
 set machine_code_cfg_info=true
 set enable_jump_tables=false
 set enable_heap_access_spectre_mitigation=false

cranelift/filetests/filetests/isa/riscv64/nan-canonicalization-has_v.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set enable_nan_canonicalization=true
 target riscv64 has_v
 

cranelift/filetests/filetests/isa/riscv64/nan-canonicalization.clif

@@ -1,5 +1,6 @@
 test compile precise-output
-set enable_nan_canonicalization=true 
+set enable_multi_ret_implicit_sret
+set enable_nan_canonicalization=true
 target riscv64
 
 function %f1(f64, f64) -> f64 {

cranelift/filetests/filetests/isa/riscv64/simd-abi-large-spill.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64gc has_v has_zvl4096b
 

cranelift/filetests/filetests/isa/riscv64/simd-abi.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 target riscv64 has_v
 
 ;; Tests both ABI and Regalloc spill/reload.

cranelift/filetests/filetests/isa/riscv64/simd-avg_round.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64 has_v
 

cranelift/filetests/filetests/isa/riscv64/simd-band.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64 has_v
 

cranelift/filetests/filetests/isa/riscv64/simd-bitselect.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64 has_v
 

cranelift/filetests/filetests/isa/riscv64/simd-bnot.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64 has_v
 

cranelift/filetests/filetests/isa/riscv64/simd-bor.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64 has_v
 

cranelift/filetests/filetests/isa/riscv64/simd-bxor.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64 has_v
 

cranelift/filetests/filetests/isa/riscv64/simd-ceil.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64 has_v
 

cranelift/filetests/filetests/isa/riscv64/simd-extractlane.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64 has_v
 

cranelift/filetests/filetests/isa/riscv64/simd-fabs.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64 has_v
 

cranelift/filetests/filetests/isa/riscv64/simd-fadd.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64 has_v
 

cranelift/filetests/filetests/isa/riscv64/simd-fcmp-eq.clif

@@ -1,4 +1,5 @@
 test compile precise-output
+set enable_multi_ret_implicit_sret
 set unwind_info=false
 target riscv64 has_v