Simplify ABI selection and tail call integration (bytecodealliance#9061)

This commit simplifies the selection of an ABI for wasm functions now
that all Cranelift backends implement tail calls. All wasm functions use
the `Tail` calling convention except when the `winch_callable` tunable
is enabled meaning that Winch-generated functions are being called.

This then additionally simplifies the activation of the tail call
proposal. It's not unconditionally active and the same across all
compilers. The Winch compiler is updated to return an error for
unsupported instructions rather than panicking so the embedder API is
suitable for feeding unsupported modules to Winch. This means that tail
calls in Winch behaves similarly to GC in Cranelift or other unsupported
proposals like SIMD in Winch.

Author: alexcrichton

crates/cranelift/src/lib.rs

@@ -158,47 +158,26 @@ fn wasm_call_signature(
     wasm_func_ty: &WasmFuncType,
     tunables: &Tunables,
 ) -> ir::Signature {
-    // NB: this calling convention in the near future is expected to be
-    // unconditionally switched to the "tail" calling convention once all
-    // platforms have support for tail calls.
+    // The default calling convention is `CallConv::Tail` to enable the use of
+    // tail calls in modules when needed. Note that this is used even if the
+    // tail call proposal is disabled in wasm. This is not interacted with on
+    // the host so it's purely an internal detail of wasm itself.
     //
-    // Also note that the calling convention for wasm functions is purely an
-    // internal implementation detail of cranelift and Wasmtime. Native Rust
-    // code does not interact with raw wasm functions and instead always
-    // operates through trampolines either using the `array_call_signature` or
-    // `native_call_signature` where the default platform ABI is used.
-    let call_conv = match isa.triple().architecture {
-        // If the tail calls proposal is enabled, we must use the tail calling
-        // convention. We don't use it by default yet because of
-        // https://github.com/bytecodealliance/wasmtime/issues/6759
-        _ if tunables.tail_callable => {
-            assert!(
-                !tunables.winch_callable,
-                "Winch doesn't support the WebAssembly tail call proposal",
-            );
-
-            CallConv::Tail
-        }
-
-        // The winch calling convention is only implemented for x64 and aarch64
-        arch if tunables.winch_callable => {
-            assert!(
-                matches!(arch, Architecture::X86_64 | Architecture::Aarch64(_)),
-                "The Winch calling convention is only implemented for x86_64 and aarch64"
-            );
-            CallConv::Winch
-        }
-
-        // On s390x the "wasmtime" calling convention is used to give vectors
-        // little-endian lane order at the ABI layer which should reduce the
-        // need for conversion when operating on vector function arguments. By
-        // default vectors on s390x are otherwise in big-endian lane order which
-        // would require conversions.
-        Architecture::S390x => CallConv::WasmtimeSystemV,
-
-        // All other platforms pick "fast" as the calling convention since it's
-        // presumably, well, the fastest.
-        _ => CallConv::Fast,
+    // The Winch calling convention is used instead when generating trampolines
+    // which call Winch-generated functions. The winch calling convention is
+    // only implemented for x64 and aarch64, so assert that here and panic on
+    // other architectures.
+    let call_conv = if tunables.winch_callable {
+        assert!(
+            matches!(
+                isa.triple().architecture,
+                Architecture::X86_64 | Architecture::Aarch64(_)
+            ),
+            "The Winch calling convention is only implemented for x86_64 and aarch64"
+        );
+        CallConv::Winch
+    } else {
+        CallConv::Tail
     };
     let mut sig = blank_sig(isa, call_conv);
     let cvt = |ty: &WasmValType| ir::AbiParam::new(value_type(isa, *ty));

crates/environ/src/tunables.rs

@@ -60,9 +60,6 @@ pub struct Tunables {
     /// be deterministic.
     pub relaxed_simd_deterministic: bool,
 
-    /// Whether or not Wasm functions can be tail-called or not.
-    pub tail_callable: bool,
-
     /// Whether or not Wasm functions target the winch abi.
     pub winch_callable: bool,
 }
@@ -115,7 +112,6 @@ impl Tunables {
             generate_address_map: true,
             debug_adapter_modules: false,
             relaxed_simd_deterministic: false,
-            tail_callable: false,
             winch_callable: false,
         }
     }

crates/wasmtime/src/config.rs

@@ -150,7 +150,6 @@ struct ConfigTunables {
     generate_address_map: Option<bool>,
     debug_adapter_modules: Option<bool>,
     relaxed_simd_deterministic: Option<bool>,
-    tail_callable: Option<bool>,
 }
 
 /// User-provided configuration for the compiler.
@@ -288,10 +287,6 @@ impl Config {
         ret.wasm_simd(true);
         ret.wasm_backtrace_details(WasmBacktraceDetails::Environment);
 
-        // This is on-by-default in `wasmparser` since it's a stage 4+ proposal
-        // but it's not implemented in Wasmtime yet so disable it.
-        ret.features.set(WasmFeatures::TAIL_CALL, false);
-
         ret
     }
 
@@ -721,7 +716,6 @@ impl Config {
     /// [WebAssembly tail calls proposal]: https://github.com/WebAssembly/tail-call
     pub fn wasm_tail_call(&mut self, enable: bool) -> &mut Self {
         self.features.set(WasmFeatures::TAIL_CALL, enable);
-        self.tunables.tail_callable = Some(enable);
         self
     }
 
@@ -1729,20 +1723,6 @@ impl Config {
         self
     }
 
-    pub(crate) fn conditionally_enable_defaults(&mut self) {
-        // If tail calls were not explicitly enabled/disabled (i.e. tail_callable is None), enable
-        // them if we are targeting a backend that supports them. Currently the Cranelift
-        // compilation strategy is the only one that supports tail calls.
-        if self.tunables.tail_callable.is_none() {
-            #[cfg(feature = "cranelift")]
-            let default_tail_calls = self.compiler_config.strategy == Some(Strategy::Cranelift);
-            #[cfg(not(feature = "cranelift"))]
-            let default_tail_calls = false;
-
-            self.wasm_tail_call(default_tail_calls);
-        }
-    }
-
     pub(crate) fn validate(&self) -> Result<Tunables> {
         if self.features.contains(WasmFeatures::REFERENCE_TYPES)
             && !self.features.contains(WasmFeatures::BULK_MEMORY)
@@ -1813,18 +1793,13 @@ impl Config {
             generate_address_map
             debug_adapter_modules
             relaxed_simd_deterministic
-            tail_callable
         }
 
         // If we're going to compile with winch, we must use the winch calling convention.
         #[cfg(any(feature = "cranelift", feature = "winch"))]
         {
             tunables.winch_callable = self.compiler_config.strategy == Some(Strategy::Winch);
 
-            if tunables.winch_callable && tunables.tail_callable {
-                bail!("Winch does not support the WebAssembly tail call proposal");
-            }
-
             if tunables.winch_callable && !tunables.table_lazy_init {
                 bail!("Winch requires the table-lazy-init configuration option");
             }

crates/wasmtime/src/engine.rs

@@ -98,12 +98,7 @@ impl Engine {
             crate::runtime::vm::debug_builtins::ensure_exported();
         }
 
-        let config = {
-            let mut config = config.clone();
-            config.conditionally_enable_defaults();
-            config
-        };
-
+        let config = config.clone();
         let tunables = config.validate()?;
 
         #[cfg(any(feature = "cranelift", feature = "winch"))]

crates/wasmtime/src/engine/serialization.rs

@@ -366,7 +366,6 @@ impl Metadata<'_> {
             guard_before_linear_memory,
             table_lazy_init,
             relaxed_simd_deterministic,
-            tail_callable,
             winch_callable,
 
             // This doesn't affect compilation, it's just a runtime setting.
@@ -429,7 +428,6 @@ impl Metadata<'_> {
             other.relaxed_simd_deterministic,
             "relaxed simd deterministic semantics",
         )?;
-        Self::check_bool(tail_callable, other.tail_callable, "WebAssembly tail calls")?;
         Self::check_bool(
             winch_callable,
             other.winch_callable,

tests/all/defaults.rs

@@ -3,40 +3,47 @@ use wasmtime::*;
 #[test]
 #[cfg_attr(miri, ignore)]
 fn test_tail_call_default() -> Result<()> {
-    for (expected, cfg) in [
+    for (line, expected, cfg) in [
         (
+            line!(),
             true,
             Config::new()
                 .strategy(Strategy::Cranelift)
                 .target("x86_64")?,
         ),
         (
+            line!(),
             true,
             Config::new()
                 .strategy(Strategy::Cranelift)
                 .target("aarch64")?,
         ),
         (
+            line!(),
             true,
             Config::new()
                 .strategy(Strategy::Cranelift)
                 .target("riscv64")?,
         ),
         (
+            line!(),
             true,
             Config::new()
                 .strategy(Strategy::Cranelift)
                 .target("s390x")?,
         ),
         (
+            line!(),
             false,
             Config::new().strategy(Strategy::Winch).target("x86_64")?,
         ),
         (
+            line!(),
             false,
             Config::new().strategy(Strategy::Winch).target("aarch64")?,
         ),
         (
+            line!(),
             false,
             Config::new()
                 .strategy(Strategy::Cranelift)
@@ -54,7 +61,7 @@ fn test_tail_call_default() -> Result<()> {
 
         let result = engine.precompile_module(wat.as_bytes()).map(|_| ());
 
-        eprintln!("for config {cfg:?}, got: {result:?}");
+        eprintln!("for config on line {line}, got: {result:?}");
 
         assert_eq!(expected, result.is_ok());
     }

tests/all/module.rs

@@ -270,10 +270,6 @@ fn tail_call_defaults() -> Result<()> {
             wasm_with_tail_calls,
         );
         assert!(err.is_err());
-
-        // can't enable with winch
-        let err = Engine::new(Config::new().strategy(Strategy::Winch).wasm_tail_call(true));
-        assert!(err.is_err());
     }
     Ok(())
 }

winch/codegen/src/codegen/mod.rs

@@ -72,6 +72,10 @@ where
 
     /// Information about the source code location.
     pub source_location: SourceLocation,
+
+    /// Flag indicating whether during translation an unsupported instruction
+    /// was found.
+    pub found_unsupported_instruction: Option<&'static str>,
 }
 
 impl<'a, 'translation, 'data, M> CodeGen<'a, 'translation, 'data, M>
@@ -91,6 +95,7 @@ where
             env,
             source_location: Default::default(),
             control_frames: Default::default(),
+            found_unsupported_instruction: None,
         }
     }
 
@@ -226,6 +231,10 @@ where
                 self,
                 offset,
             ))??;
+
+            if let Some(insn) = self.found_unsupported_instruction {
+                anyhow::bail!("unsupported instruction in Winch: {insn}")
+            }
         }
         validator.finish(body.original_position())?;
         return Ok(());

winch/codegen/src/visitor.rs

@@ -36,7 +36,8 @@ macro_rules! def_unsupported {
 
                 fn $visit(&mut self $($(,$arg: $argty)*)?) -> Self::Output {
                     $($(let _ = $arg;)*)?
-                        todo!(stringify!($op))
+
+                    self.found_unsupported_instruction = Some(stringify!($op));
                 }
             );
         )*