Exit through Cranelift-generated trampolines for builtins#8152
Merged
alexcrichton merged 5 commits intoMar 22, 2024
Merged
Conversation
Member
Author
|
I'll note that I'm opening this up as a draft because Winch doesn't work yet. I'll need to update invocation of those libcalls in addition to updating how trampolines are required. I plan on waiting until #8109 lands to fully implement that though to avoid implementing this new trampoline in Winch. |
github-actions
Bot
added
wasmtime:api
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "wasmtime:api", "winch"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
fitzgen
reviewed
Mar 18, 2024
Member
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
Looks great! Excited to remove the last vestiges of hand-written asm.
alexcrichton
force-pushed
the
builtin-trampolines-through-cranelift
branch
3 times, most recently
from
9735d55 to
2dd385c
Compare
alexcrichton
force-pushed
the
builtin-trampolines-through-cranelift
branch
from
2dd385c to
fa18c36
Compare
alexcrichton
commented
Mar 21, 2024
alexcrichton
force-pushed
the
builtin-trampolines-through-cranelift
branch
from
3810ab5 to
f08e292
Compare
Member
Author
|
Ok various prerequisites are now landed on |
alexcrichton
requested review from
elliottt,
fitzgen and
saulecabrera
and removed request for
a team and
abrown
fitzgen
approved these changes
Mar 22, 2024
elliottt
reviewed
Mar 22, 2024
saulecabrera
approved these changes
Mar 22, 2024
Member
saulecabrera
left a comment
saulecabrera
left a comment
There was a problem hiding this comment.
Winch pieces look great to me, thanks!
Comment on lines
+92
to
+96
| Local(FuncIndex), | ||
| /// Imported function. | ||
| Import(&'a CalleeInfo), | ||
| Import(FuncIndex), | ||
| /// Function reference. | ||
| FuncRef(&'a ABISig), | ||
| FuncRef(TypeIndex), |
This commit changes how builtin functions in Wasmtime (think `memory.grow`) are implemented. These functions are required to exit through some manner of trampoline to handle runtime requirements for backtracing right now. Currently this is done via inline assembly for each architecture (or external assembly for s390x). This is a bit unfortunate as it's a lot of hand-coding and making sure everything is right, and it's not easy to update as it's multiple platforms to update. The change in this commit is to instead use Cranelift-generated trampolines for this purpose instead. The path for invoking a builtin function now looks like: * Wasm code calls a statically known symbol for each builtin. * The statically known symbol will perform exit trampoline duties (e.g. pc/fp/etc) and then load a function pointer to the host implementation. * The host implementation is invoked and then proceeds as usual. The main new piece for this PR is that all wasm modules and functions are compiled in parallel but an output of this compilation phase is what builtin functions are required. All builtin functions are then unioned together into one set and then anything required is generated just afterwards. That means that only one builtin-trampoline per-module is generated per-builtin. This work is inspired by bytecodealliance#8135 and my own personal desire to have as much about our ABI details flowing through Cranelift as we can. This in theory makes it more flexible to deal with future improvements to our ABI. prtest:full
This commit refactors the Winch compiler to use the new trampolines for all Wasmtime builtins created in the previous commits. This required a fair bit of refactoring to handle plumbing through a new kind of relocation and function call. Winch's `FuncEnv` now contains a `PrimaryMap` from `UserExternalNameRef` to `UserExternalName`. This is because there's now more than one kind of name than just wasm function relocations, so the raw index space of `UserExternalNameRef` is no longer applicable. This required threading `FuncEnv` to more locations along with some refactorings to ensure that lifetimes work out ok. The `CompiledFunction` no longer stores a trait object of how to map name refs to names and now directly has a `Primarymap`. This also means that Winch's return value from its `TargetIsa` is a `CompiledFunction` as opposed to the previous just-a-`MachBuffer` so it can also package up all the relocation information. This ends up having `winch-codegen` depend on `wasmtime-cranelift-shared` as a new dependency.
alexcrichton
force-pushed
the
builtin-trampolines-through-cranelift
branch
from
f08e292 to
ed948fa
Compare
dhil
pushed a commit
to dhil/wasmtime
that referenced
this pull request
Mar 26, 2024
…liance#8152) * Exit through Cranelift-generated trampolines for builtins This commit changes how builtin functions in Wasmtime (think `memory.grow`) are implemented. These functions are required to exit through some manner of trampoline to handle runtime requirements for backtracing right now. Currently this is done via inline assembly for each architecture (or external assembly for s390x). This is a bit unfortunate as it's a lot of hand-coding and making sure everything is right, and it's not easy to update as it's multiple platforms to update. The change in this commit is to instead use Cranelift-generated trampolines for this purpose instead. The path for invoking a builtin function now looks like: * Wasm code calls a statically known symbol for each builtin. * The statically known symbol will perform exit trampoline duties (e.g. pc/fp/etc) and then load a function pointer to the host implementation. * The host implementation is invoked and then proceeds as usual. The main new piece for this PR is that all wasm modules and functions are compiled in parallel but an output of this compilation phase is what builtin functions are required. All builtin functions are then unioned together into one set and then anything required is generated just afterwards. That means that only one builtin-trampoline per-module is generated per-builtin. This work is inspired by bytecodealliance#8135 and my own personal desire to have as much about our ABI details flowing through Cranelift as we can. This in theory makes it more flexible to deal with future improvements to our ABI. prtest:full * Fix some build issues * Update winch test expectations * Update Winch to use new builtin shims. This commit refactors the Winch compiler to use the new trampolines for all Wasmtime builtins created in the previous commits. This required a fair bit of refactoring to handle plumbing through a new kind of relocation and function call. Winch's `FuncEnv` now contains a `PrimaryMap` from `UserExternalNameRef` to `UserExternalName`. This is because there's now more than one kind of name than just wasm function relocations, so the raw index space of `UserExternalNameRef` is no longer applicable. This required threading `FuncEnv` to more locations along with some refactorings to ensure that lifetimes work out ok. The `CompiledFunction` no longer stores a trait object of how to map name refs to names and now directly has a `Primarymap`. This also means that Winch's return value from its `TargetIsa` is a `CompiledFunction` as opposed to the previous just-a-`MachBuffer` so it can also package up all the relocation information. This ends up having `winch-codegen` depend on `wasmtime-cranelift-shared` as a new dependency. * Review feedback
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit changes how builtin functions in Wasmtime (think
memory.grow) are implemented. These functions are required to exit through some manner of trampoline to handle runtime requirements for backtracing right now. Currently this is done via inline assembly for each architecture (or external assembly for s390x). This is a bit unfortunate as it's a lot of hand-coding and making sure everything is right, and it's not easy to update as it's multiple platforms to update.The change in this commit is to instead use Cranelift-generated trampolines for this purpose instead. The path for invoking a builtin function now looks like:
The main new piece for this PR is that all wasm modules and functions are compiled in parallel but an output of this compilation phase is what builtin functions are required. All builtin functions are then unioned together into one set and then anything required is generated just afterwards. That means that only one builtin-trampoline per-module is generated per-builtin.
This work is inspired by #8135 and my own personal desire to have as much about our ABI details flowing through Cranelift as we can. This in theory makes it more flexible to deal with future improvements to our ABI.