Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wasmtime: Add support for Wasm tail calls #6774

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jul 25, 2023

This adds the Config::wasm_tail_call method and --wasm-features tail-call CLI flag to enable the Wasm tail calls proposal in Wasmtime.

This PR is mostly just plumbing and enabling tests, since all the prerequisite work (Wasmtime trampoline overhauls and Cranelift tail calls) was completed in earlier pull requests.

When Wasm tail calls are enabled, Wasm code uses the tail calling convention. The tail calling convention is known to cause a 1-7% slow down for regular code that isn't using tail calls, which is why it isn't used unconditionally. This involved shepherding Tunables through to Wasm signature construction methods. The eventual plan is for the tail calling convention to be used unconditionally, but not until the performance regression is addressed. This work is tracked in
#6759

Additionally while our x86-64, aarch64, and riscv64 backends support tail calls, the s390x backend does not support them yet. Attempts to use tail calls on s390x will return errors. Support for s390x is tracked in #6530

@fitzgen fitzgen requested a review from alexcrichton July 25, 2023 21:01
@fitzgen fitzgen requested review from a team as code owners July 25, 2023 21:01
@fitzgen fitzgen requested review from cfallin, a team and elliottt and removed request for a team, cfallin and elliottt July 25, 2023 21:01
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

One thing I might recommend doing though is to push a commit with prtest:full which enables tail calls by default. That'll help weed out any lingering issues and the commit can be backed out before landing to keep it off-by-default in tree

crates/environ/src/compilation.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:wasm fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime winch Winch issues or pull requests labels Jul 25, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @fitzgen, @peterhuene, @saulecabrera

This issue or pull request has been labeled: "cranelift", "cranelift:wasm", "fuzzing", "wasmtime:api", "wasmtime:config", "winch"

Thus the following users have been cc'd because of the following labels:

  • fitzgen: fuzzing
  • peterhuene: wasmtime:api
  • saulecabrera: winch

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@github-actions
Copy link

github-actions bot commented Jul 25, 2023

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

  • If you added a new Config method, you wrote extensive documentation for
    it.

    Our documentation should be of the following form:

    Short, simple summary sentence.
    
    More details. These details can be multiple paragraphs. There should be
    information about not just the method, but its parameters and results as
    well.
    
    Is this method fallible? If so, when can it return an error?
    
    Can this method panic? If so, when does it panic?
    
    # Example
    
    Optional example here.
    
  • If you added a new Config method, or modified an existing one, you
    ensured that this configuration is exercised by the fuzz targets.

    For example, if you expose a new strategy for allocating the next instance
    slot inside the pooling allocator, you should ensure that at least one of our
    fuzz targets exercises that new strategy.

    Often, all that is required of you is to ensure that there is a knob for this
    configuration option in wasmtime_fuzzing::Config (or one
    of its nested structs).

    Rarely, this may require authoring a new fuzz target to specifically test this
    configuration. See our docs on fuzzing for more details.

  • If you are enabling a configuration option by default, make sure that it
    has been fuzzed for at least two weeks before turning it on by default.


To modify this label's message, edit the .github/label-messager/wasmtime-config.md file.

To add new label messages or remove existing label messages, edit the
.github/label-messager.json configuration file.

Learn more.

@fitzgen
Copy link
Member Author

fitzgen commented Jul 25, 2023

Doing a prtest:full run with tail calls enabled by default now.

@fitzgen fitzgen force-pushed the wasmtime-tail-calls branch 2 times, most recently from 1428982 to 0687b24 Compare July 25, 2023 23:02
@fitzgen
Copy link
Member Author

fitzgen commented Jul 27, 2023

Tracked down the riscv64 failure as #6780

@fitzgen fitzgen force-pushed the wasmtime-tail-calls branch from a40f04e to b75b108 Compare July 27, 2023 00:26
@fitzgen
Copy link
Member Author

fitzgen commented Jul 27, 2023

Looks like it is just s390x that is failing prtest:full now, which is expected as s390x doesn't support tail calls yet. Gonna remove the temp commit and enqueue this to merge!

This adds the `Config::wasm_tail_call` method and `--wasm-features tail-call`
CLI flag to enable the Wasm tail calls proposal in Wasmtime.

This PR is mostly just plumbing and enabling tests, since all the prerequisite
work (Wasmtime trampoline overhauls and Cranelift tail calls) was completed in
earlier pull requests.

When Wasm tail calls are enabled, Wasm code uses the `tail` calling
convention. The `tail` calling convention is known to cause a 1-7% slow down for
regular code that isn't using tail calls, which is why it isn't used
unconditionally. This involved shepherding `Tunables` through to Wasm signature
construction methods. The eventual plan is for the `tail` calling convention to
be used unconditionally, but not until the performance regression is
addressed. This work is tracked in
bytecodealliance#6759

Additionally while our x86-64, aarch64, and riscv64 backends support tail calls,
the s390x backend does not support them yet. Attempts to use tail calls on s390x
will return errors. Support for s390x is tracked in
bytecodealliance#6530
Instead of passing as an argument to every `Compiler` method.
@fitzgen fitzgen force-pushed the wasmtime-tail-calls branch from b75b108 to 82b7774 Compare July 27, 2023 15:35
@fitzgen fitzgen enabled auto-merge July 27, 2023 15:35
@fitzgen fitzgen added this pull request to the merge queue Jul 27, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 27, 2023
@fitzgen
Copy link
Member Author

fitzgen commented Jul 27, 2023

Hm looks like there is actually a second riscv64 bug lurking here.

@fitzgen fitzgen enabled auto-merge July 27, 2023 19:05
@fitzgen fitzgen disabled auto-merge July 27, 2023 19:11
They still use `jalr` instead of `jal` but this allows us to use the `RiscvCall`
reloc, which Wasmtime handles. Before we were using `LoadExternalName` which
produces an `Abs8` reloc, which Wasmtime intentionally does not handle since
that involves patching code at runtime, which makes loading code slower.
@fitzgen fitzgen force-pushed the wasmtime-tail-calls branch from 6c0d1b0 to 749b523 Compare July 27, 2023 19:16
@fitzgen fitzgen enabled auto-merge July 27, 2023 19:16
@fitzgen fitzgen added this pull request to the merge queue Jul 27, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 27, 2023
@fitzgen fitzgen enabled auto-merge July 27, 2023 20:59
@fitzgen fitzgen added this pull request to the merge queue Jul 27, 2023
Merged via the queue into bytecodealliance:main with commit 868f0c3 Jul 27, 2023
@fitzgen fitzgen deleted the wasmtime-tail-calls branch July 27, 2023 22:19
fitzgen added a commit to fitzgen/website that referenced this pull request Jul 27, 2023
dtig pushed a commit to WebAssembly/website that referenced this pull request Jul 31, 2023
geekbeast pushed a commit to geekbeast/wasmtime that referenced this pull request Aug 1, 2023
* main: (47 commits)
  Add core dump support to the runtime (bytecodealliance#6513)
  Resource table tracks child relationships (bytecodealliance#6779)
  Wasmtime: Move `OnDemandInstanceAllocator` to its own module (bytecodealliance#6790)
  wasi: Test the stdio streams implementation (bytecodealliance#6764)
  Don't generate same-named imports in fact modules (bytecodealliance#6783)
  Wasmtime: Add support for Wasm tail calls (bytecodealliance#6774)
  Cranelift: Fix `ABIMachineSpec::gen_add_imm` for riscv64 (bytecodealliance#6780)
  Update the wasm-tools family of crates, disallow empty component types (bytecodealliance#6777)
  Fix broken link to WASI API documentation (bytecodealliance#6775)
  A bunch of cleanups for cranelift-codegen-meta (bytecodealliance#6772)
  Implement component-to-component calls with resources (bytecodealliance#6769)
  Ignore async_stack_size if async_support is disabled (bytecodealliance#6771)
  A bunch of minor cleanups (bytecodealliance#6767)
  Fix flaky tests in preview2 streams (bytecodealliance#6763)
  Refactor and simplify component trampolines (bytecodealliance#6751)
  Cranelift: Implement tail calls on riscv64 (bytecodealliance#6749)
  WASI Preview 2: rewrite streams and pollable implementation (bytecodealliance#6556)
  cranelift-wasm: Add support for translating Wasm tail calls to CLIF (bytecodealliance#6760)
  Cranelift: Get tail calls working on aarch64 (bytecodealliance#6723)
  Implement component model resources in Wasmtime (bytecodealliance#6691)
  ...
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Aug 18, 2023
* Wasmtime: Add support for Wasm tail calls

This adds the `Config::wasm_tail_call` method and `--wasm-features tail-call`
CLI flag to enable the Wasm tail calls proposal in Wasmtime.

This PR is mostly just plumbing and enabling tests, since all the prerequisite
work (Wasmtime trampoline overhauls and Cranelift tail calls) was completed in
earlier pull requests.

When Wasm tail calls are enabled, Wasm code uses the `tail` calling
convention. The `tail` calling convention is known to cause a 1-7% slow down for
regular code that isn't using tail calls, which is why it isn't used
unconditionally. This involved shepherding `Tunables` through to Wasm signature
construction methods. The eventual plan is for the `tail` calling convention to
be used unconditionally, but not until the performance regression is
addressed. This work is tracked in
bytecodealliance#6759

Additionally while our x86-64, aarch64, and riscv64 backends support tail calls,
the s390x backend does not support them yet. Attempts to use tail calls on s390x
will return errors. Support for s390x is tracked in
bytecodealliance#6530

* Store `Tunables` inside the `Compiler`

Instead of passing as an argument to every `Compiler` method.

* Cranelift: Support "direct" return calls on riscv64

They still use `jalr` instead of `jal` but this allows us to use the `RiscvCall`
reloc, which Wasmtime handles. Before we were using `LoadExternalName` which
produces an `Abs8` reloc, which Wasmtime intentionally does not handle since
that involves patching code at runtime, which makes loading code slower.

* Fix tests that assume tail call support on s390x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:wasm cranelift Issues related to the Cranelift code generator fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime winch Winch issues or pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants