winch: Overhaul the internal ABI #7974

saulecabrera · 2024-02-21T16:04:33Z

This change overhauls Winch's ABI. This means that as part of this change, the default ABI now closely resembles Cranelift's ABI, particularly on the treatment of the VMContext. This change also fixes many wrong assumptions about trampolines, which are tied to how the previous ABI operated.

The main motivation behind this change is:

To make it easier to integrate Winch-generated functions with Wasmtime
Fix fuzz bugs related to imports
Solidify the implementation regarding the usage of a pinned register to hold the VMContext value throughout the lifetime of a function.

The previous implementation had the following characteristics, and wrong assumptions:

Assumed that internal functions don't receive a caller or callee VMContexts as parameters.
Worked correctly in the following scenarios:
- Wasm -> Native: since we can explicitly load the caller and callee VMContext, because we're calling a native import.
- (Native, Array) -> Wasm: because the native signatures define a tuple of VMContext as arguments.
It didn't work in the following scenario:
- Wasm -> Wasm: When calling imports from another WebAssembly instance (via direct call or call_indirect. The previous implementation wrongly assumed that there should be a trampoline in this case, but there isn't. The code was generated by the same compiler, so the same ABI should be used in both functions.

This change introduces the following changes, which fix the previous assumptions and bugs:

All internal functions declare a two extra pointer-sized parameters, which will hold the callee and caller VMContexts
Use a pinned register that will be considered live through the lifetime of the function instead of pinning it at the trampoline level. The pinning explicitlly happens when entering the function body and no other assumptions are made from there on.
Introduce the concept of special ContextArgs for function calls. This enum holds metadata about which context arguments are needed depending on the callee. The previous implementation of introducing register values at arbitrary locations in the value stack conflicts with the stack ordering principle which states that older values must always precede newer values. So we can't insert a register, because if a spill happens the order of the values will be wrong.

Finally, given that this change also enables the imports.wast test suite, it also includes a fix to global.{get, set} instructions which didn't account entirely for imported globals.

saulecabrera · 2024-02-21T16:07:45Z

winch/codegen/src/abi/mod.rs

@@ -60,6 +64,84 @@ pub(super) enum ParamsOrReturns {
    Returns,
 }

+/// Macro to get the pinned register holding the [VMContext].
+macro_rules! vmctx {


This change is fairly large already, so I opted to just introduce this macro and use it in a couple of places that are updated by this change. My plan is to follow up with a general replace after landing this change (but I can do so here too if preferred).

cfallin

Overall looks great, thanks! Just a few nits below.

winch/codegen/src/abi/mod.rs

winch/codegen/src/frame/mod.rs

cfallin · 2024-02-21T17:43:53Z

winch/codegen/src/frame/mod.rs

+    /// # Panics
+    /// This method panics if the index is not associated to a valid WebAssembly
+    /// local.
+    pub fn get_local_unchecked(&self, index: usize) -> &LocalSlot {


Rather than unchecked, which to me implies lack of a bounds check or similar, can we name this in a way that indicates the index-space is shifted relative to above? E.g. perhaps the above get_local becomes get_wasm_local and get_local_unchecked becomes get_raw_local?

Alternately, avoid exposing get_local_unchecked at all and have two separate methods get_callee_local() / get_caller_local()?

Thanks for the suggestion -- I ended up with get_frame_local and get_wasm_local to be able to distinguish one from another.

cfallin · 2024-02-21T17:45:53Z

winch/codegen/src/trampoline.rs

@@ -601,6 +528,9 @@ where
        callee_sig
            .params_without_retptr()
            .iter()
+            // Skip the first two arguments, which are the callee and caller
+            // VMContext pointers.
+            .skip(2)


here and perhaps elsewhere, can we have a constant (maybe an associated constant of ContextArgs) indicating how many args we expect? Basically it'd be nice to tie this to other definitions so that we don't miss it if we ever add, e.g., a third context arg.

Good catch -- I introduced MAX_CONTEXT_ARGS near the ContextArgs definition, which is used in the trampoline. In the non-trampoline case (normal function calls), the expected number of context arguments is already parametrized through ContextArgs::len

github-actions · 2024-02-21T20:45:05Z

Subscribe to Label Action

cc @saulecabrera

This issue or pull request has been labeled: "winch"

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

saulecabrera: winch

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

Learn more.

saulecabrera · 2024-02-21T21:16:49Z

Oh bad rebase -- sorry about that. Fixing it now.

This change overhauls Winch's ABI. This means that as part of this change, the default ABI now closely resembles Cranelift's ABI, particularly on the treatment of the VMContext. This change also fixes many wrong assumptions about trampolines, which are tied to how the previous ABI operated. The main motivation behind this change is: * To make it easier to integrate Winch-generated functions with Wasmtime * Fix fuzz bugs related to imports * Solidify the implementation regarding the usage of a pinned register to hold the VMContext value throughout the lifetime of a function. The previous implementation had the following characteristics, and wrong assumptions): * Assumed that nternal functions don't receive a caller or callee VMContexts as parameters. * Worked correctly in the following scenarios: * `Wasm -> Native`: since we can explicitly load the caller and callee `VMContext`, because we're calling a native import. * `(Native, Array) -> Wasm`: because the native signatures define a tuple of `VMContext` as arguments. * It didn't work in the following scenario: * `Wasm->Wasm`: When calling imports from another WebAssembly instance (via direct call or `call_indirect`. The previous implementation wrongly assumes that there should be a trampoline in this case, but there isn't. The code was generated by the same compiler, so the same ABI should be used in both functions, but it doesn't. This change introduces the following changes, which fix the previous assumptions and bugs: * All internal functions declare a two extra pointer-sized parameters, which will hold the callee and caller `VMContext`s * Use a pinned register that will be considered live through the lifetime of the function instead of pinning it at the trampoline level. The pinning explicitlly happens when entering the function body and no other assumptions are made from there on. * Introduce the concept of special `ContextArgs` for function calls. This enum holds metadata about which context arguments are needed depending on the callee. The previous implementation of introducing register values at arbitrary locations in the value stack conflicts with the stack ordering principle which states that older values must *always* precede newer values. So we can't insert a register, because if a spill happens the order of the values will be wrong. Finally, given that this change also enables the `imports.wast` test suite, it also includes a fix to `global.{get, set}` instructions which didn't account entirely for imported globals. Resolved conflicts Update Winch filetests

… `get_local_unchecked`

…text arguments.

This commit is a follow-up to bytecodealliance#7974. This commit makes use of the `vmctx!` macro across the compiler codebase

This commit is a follow-up to #7974. This commit makes use of the `vmctx!` macro across the compiler codebase

saulecabrera requested review from a team as code owners February 21, 2024 16:04

saulecabrera requested review from cfallin and removed request for a team February 21, 2024 16:04

saulecabrera commented Feb 21, 2024

View reviewed changes

cfallin approved these changes Feb 21, 2024

View reviewed changes

github-actions bot added the winch Winch issues or pull requests label Feb 21, 2024

saulecabrera force-pushed the winch-overhaul-abi branch from d3326d3 to 9319c49 Compare February 21, 2024 21:15

saulecabrera requested review from a team as code owners February 21, 2024 21:15

saulecabrera requested review from fitzgen and removed request for a team February 21, 2024 21:15

saulecabrera force-pushed the winch-overhaul-abi branch from 9319c49 to cdbc516 Compare February 21, 2024 21:20

saulecabrera added 3 commits February 21, 2024 16:50

Fix typos

20860cd

Use get_wasm_local and get_frame_local instead of get_local and…

a20dbbd

… `get_local_unchecked`

Introduce MAX_CONTEXT_ARGS and use it in the trampoline to skip con…

36608ed

…text arguments.

saulecabrera added this pull request to the merge queue Feb 21, 2024

Merged via the queue into bytecodealliance:main with commit 0e98a8d Feb 21, 2024
19 checks passed

saulecabrera deleted the winch-overhaul-abi branch February 21, 2024 23:15

saulecabrera added a commit to saulecabrera/wasmtime that referenced this pull request Feb 22, 2024

winch: Use the vmctx! macro where possible

5dc7d39

This commit is a follow-up to bytecodealliance#7974. This commit makes use of the `vmctx!` macro across the compiler codebase

saulecabrera mentioned this pull request Feb 22, 2024

winch: Use the vmctx! macro where possible #7977

Merged

github-merge-queue bot pushed a commit that referenced this pull request Feb 22, 2024

winch: Use the vmctx! macro where possible (#7977)

cb1cecd

This commit is a follow-up to #7974. This commit makes use of the `vmctx!` macro across the compiler codebase

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

winch: Overhaul the internal ABI #7974

winch: Overhaul the internal ABI #7974

saulecabrera commented Feb 21, 2024 •

edited

Loading

saulecabrera Feb 21, 2024

cfallin left a comment

cfallin Feb 21, 2024

saulecabrera Feb 21, 2024

cfallin Feb 21, 2024

saulecabrera Feb 21, 2024

github-actions bot commented Feb 21, 2024

saulecabrera commented Feb 21, 2024

winch: Overhaul the internal ABI #7974

winch: Overhaul the internal ABI #7974

Conversation

saulecabrera commented Feb 21, 2024 • edited Loading

saulecabrera Feb 21, 2024

Choose a reason for hiding this comment

cfallin left a comment

Choose a reason for hiding this comment

cfallin Feb 21, 2024

Choose a reason for hiding this comment

saulecabrera Feb 21, 2024

Choose a reason for hiding this comment

cfallin Feb 21, 2024

Choose a reason for hiding this comment

saulecabrera Feb 21, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2024

Subscribe to Label Action

saulecabrera commented Feb 21, 2024

saulecabrera commented Feb 21, 2024 •

edited

Loading