Repository: bytecodealliance/wasmtime
Pull request #8540 (2024-05-03 17:27:15)

Enable the tail calling convention by default
Switch to enabling the wasm tail call feature by default when the target is one of `x86_64`, `aarch64`, or `riscv64`, and the compilation strategy is `cranelift`. We also added tests checking that the feature is not enabled for either winch or the `s390x` backend.


Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

<!--
Please make sure you include the following information:

- If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

- Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->


elliottt: I did some perf testing during the initial tail-call refactoring PRs and didn't notice any regression in perf. I'll do another sightglass run with this branch to verify that.

We initially tried to implement this in `Config::new`, but as we still have some cases that don't permit tail calls, it ended up being easiest to interpret the default in `validate`. I agree that the conditions are a little unfortunate, but I think this is the best option currently.
alexcrichton: Do you think it would be workable to enable tail calls by default except for s390x and then have a validation check that tail calls aren't enabled for winch? That would require `-W tail-call=n` for running with Winch but that I think would compose a bit better
elliottt: Where would we set the default for `tail-call` that isn't `Config::new`? The problem with setting it there is that we don't yet know the compilation target. That was part of what motivated delaying the interpretation of the default to `Config::validate`, as we would definitely know what target we were compiling for at that point. Otherwise, I do like the approach of explicitly disabling tail calls when also targeting winch.
fitzgen: > Nice! Out of curiosity do you know if a sightglass run has been done to confirm there's no perf regression?

Yep, there were multiple runs at various points here, and there is no longer any gap between `tail` and `wasmtime-systemv` anymore. In fact, I'm 99% sure the two calling conventions are identical when there aren't any actual tail calls.

> Could the validation/etc be done without needing `mut self` on `Config::validate`? Instead could this preserve the `&self`-ness and perhaps add other validations to ensure the stars align right?

We tried to do this in `Config::new` initially, but because we don't know the compilation strategy (cranelift vs winch) or target (s309x or not) yet, we therefore don't know what default value to choose.

The only alternatives I see are

1. Checking these conditions when setting every related knob (i.e. check that the target isn't set to s390x when enabling tail calls, check that tail calls aren't set when configuring s390x as a target, etc...). This is annoying because there are now many checks to keep in sync with each other.

2. Enable tail calls by default, don't change any other logic. This effectively will require that s390x and winch configs explicitly disable tail calls, which is kind of annoying.

3. Move the choose-the-default-if-it-wasn't-explicitly-configured logic we added here out of `validate` and into a `finalize_defaults` method or something that is called just before `validate`. This way `validate` would still be `&self` and we would have a clear division between validation and default computation. But this is sort of just rearranging deck chairs.

4. Don't enable tail calls by default until all configs can support them. This is pretty suboptimal. Do we just wait for s390x? Or winch too? This could be a long time and I really want to start leveraging all the good work that has gone into this feature, deleting the old calling conventions, and all that. FWIW, we have prior art of enabling wasm features by default on some targets but not others when they aren't ready there yet (reference types, for example).
alexcrichton: Ah sorry yeah I was thinking `Config::new` would be the place, but I always forget about cross-compilation which is why a `cfg!` for s390x wouldn't work. I also agree that all the other alternatives you outlined Nick aren't great, so I'm sold.

As a bikeshed, though, how about restructuring this slightly? Something like:

* Leave `validate` as-is with no changes
* Add a `fn conditionally_enable_features(&mut self)` method which is called just before `validate`
* Move this logic to `conditionally_enable_features`

Also, in the case that `self.tunables.tail_callable` is `None` I think that `conditionally_enable_features` would be able to do nothing in this case, right? It's only in the `None` case where we infer a proper default from it given other configuration settings?
alexcrichton: Oh also no need to worry about perf, just wanted to double check, sounds like plenty of due diligence

Patch:
diff --git a/crates/wasmtime/src/config.rs b/crates/wasmtime/src/config.rs
index e063c2650..c7cf11958 100644
--- a/crates/wasmtime/src/config.rs
+++ b/crates/wasmtime/src/config.rs
@@ -1708,6 +1708,24 @@ 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, but not targeting s390x.
+        if self.tunables.tail_callable.is_none() {
+            #[cfg(feature = "cranelift")]
+            let default_tail_calls = self.compiler_config.strategy == Strategy::Cranelift
+                && self.compiler_config.target.as_ref().map_or_else(
+                    || target_lexicon::Triple::host().architecture,
+                    |triple| triple.architecture,
+                ) != Architecture::S390x;
+            #[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)
diff --git a/crates/wasmtime/src/engine.rs b/crates/wasmtime/src/engine.rs
index 540aa35c5..9b0453cf9 100644
--- a/crates/wasmtime/src/engine.rs
+++ b/crates/wasmtime/src/engine.rs
@@ -101,7 +101,12 @@ impl Engine {
             crate::runtime::vm::debug_builtins::ensure_exported();
         }
 
-        let config = config.clone();
+        let config = {
+            let mut config = config.clone();
+            config.conditionally_enable_defaults();
+            config
+        };
+
         let tunables = config.validate()?;
 
         #[cfg(any(feature = "cranelift", feature = "winch"))]


Test patch:
diff --git a/tests/all/defaults.rs b/tests/all/defaults.rs
new file mode 100644
index 000000000..0e869e567
--- /dev/null
+++ b/tests/all/defaults.rs
@@ -0,0 +1,62 @@
+use wasmtime::*;
+
+#[test]
+fn test_tail_call_default() -> Result<()> {
+    for (expected, cfg) in [
+        (
+            true,
+            Config::new()
+                .strategy(Strategy::Cranelift)
+                .target("x86_64")?,
+        ),
+        (
+            true,
+            Config::new()
+                .strategy(Strategy::Cranelift)
+                .target("aarch64")?,
+        ),
+        (
+            true,
+            Config::new()
+                .strategy(Strategy::Cranelift)
+                .target("riscv64")?,
+        ),
+        (
+            false,
+            Config::new()
+                .strategy(Strategy::Cranelift)
+                .target("s390x")?,
+        ),
+        (
+            false,
+            Config::new().strategy(Strategy::Winch).target("x86_64")?,
+        ),
+        (
+            false,
+            Config::new().strategy(Strategy::Winch).target("aarch64")?,
+        ),
+        (
+            false,
+            Config::new()
+                .strategy(Strategy::Cranelift)
+                .wasm_tail_call(false)
+                .target("x86_64")?,
+        ),
+    ] {
+        let engine = Engine::new(cfg)?;
+
+        let wat = r#"
+            (module $from_name_section
+                (func (export "run") (return_call 0))
+            )
+        "#;
+
+        let result = engine.precompile_module(wat.as_bytes()).map(|_| ());
+
+        eprintln!("for config {cfg:?}, got: {result:?}");
+
+        assert_eq!(expected, result.is_ok());
+    }
+
+    Ok(())
+}
diff --git a/tests/all/main.rs b/tests/all/main.rs
index f778edfde..62aaf46ce 100644
--- a/tests/all/main.rs
+++ b/tests/all/main.rs
@@ -7,6 +7,7 @@ mod code_too_large;
 mod component_model;
 mod coredump;
 mod debug;
+mod defaults;
 mod epoch_interruption;
 mod externals;
 mod fuel;