wasmtime / PR #7535 winch: Multi-Value Part 1 · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #7535 winch: Multi-Value Part 1

Wasmtime GitHub notifications bot (Nov 14 2023 at 12:49):

saulecabrera opened PR #7535 from saulecabrera:winch-multi-value to bytecodealliance:main:

This change adds initial support for the Multi-Value proposal to Winch, focusing on function calls, support for Multi-Value for blocks will be added on top of this change in a different pull request.

This change is divided into two main parts:

Infrastructure changes needed to support Multi-Value

Handling Multi-Value returns for function calls

Infrastructure changes needed to support Multi-Value

Some notable changes were needed in order to support this proposal, namely:

Introducing the notion of ABIOperands and rewriting the ABIParams and ABIResults in terms of ABIOperands.

In Winch's default calling convention, results are stored in reverse order: if a function produces multiple results, the first result is stored in a memory location and the last result is stored in a register; given that function results are captured in the value stack after a function call, this approach makes it trivial to maintain the following value stack invariants:

Spilled memory values always precede register values

Spilled values are stored from oldest to newest, matching their respective locations on the machine stack.

Introducing type-sized spills: Prior to this change, only floats produced type-sized spills, this created an inconsistency on how slots for results are handled and made it harder to manage the stack for multiple-results; this change introduces type-sized spills for integer and floats, making the spilling strategy uniform across the compiler. This might introduce some overhead, since this change is now introducing two instructions were previously there only used to be one (e.g. push vs sub + mov). My intention is to profile, and try to improve this strategy if possible later on.

Handling Multi-Value returns for function calls

The approach for handling multiple results for function calls involves:

Adding an implicit extra parameter to the ABIParams definition, when a function returns multiple values; from the callee's perspective, this parameter is treated like any other "special" parameter (e.g VMContext), to which, we give it a well know LocalSlot for later referencing when storing the return values. From the caller's perspective, extra stack space is created before the call, and the address to that space is passed in to the callee, via the return pointer parameter and once the function returns, the values in the created space are captured as values in the value stack.

Wasmtime GitHub notifications bot (Nov 14 2023 at 12:49):

saulecabrera requested fitzgen for a review on PR #7535.

Wasmtime GitHub notifications bot (Nov 14 2023 at 12:49):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #7535.

Wasmtime GitHub notifications bot (Nov 14 2023 at 12:49):

saulecabrera requested wasmtime-core-reviewers for a review on PR #7535.

Wasmtime GitHub notifications bot (Nov 14 2023 at 14:17):

saulecabrera updated PR #7535.

Wasmtime GitHub notifications bot (Nov 14 2023 at 14:18):

saulecabrera updated PR #7535.

Wasmtime GitHub notifications bot (Nov 14 2023 at 14:26):

saulecabrera edited PR #7535:

This change adds initial support for the Multi-Value proposal to Winch, focusing on function calls, support for Multi-Value for blocks will be added on top of this change in a different pull request.

This change is divided into two main parts:

Infrastructure changes needed to support Multi-Value

Handling Multi-Value returns for function calls

Infrastructure changes needed to support Multi-Value

Some notable changes were needed in order to support this proposal, namely:

Introducing the notion of ABIOperands and rewriting the ABIParams and ABIResults in terms of ABIOperands.

In Winch's default calling convention, results are stored in reverse order: if a function produces multiple results, the first result is stored in a memory location and the last result is stored in a register; given that function results are captured in the value stack after a function call, this approach makes it trivial to maintain the following value stack invariants:

Spilled memory values always precede register values

Spilled values are stored from oldest to newest, matching their respective locations on the machine stack.

Introducing type-sized spills: Prior to this change, only floats produced type-sized spills, this created an inconsistency on how slots for results are handled and made it harder to manage the stack for multiple-results; this change introduces type-sized spills for integer and floats, making the spilling strategy uniform across the compiler. This might introduce some overhead, since this change is now, in the 32-bit case, introducing two instructions were previously there only used to be one (e.g. push vs sub + mov). My intention is to profile, and try to improve this strategy if needed, later on.

Handling Multi-Value returns for function calls

The approach for handling multiple results for function calls involves:

Adding an implicit extra parameter to the ABIParams definition, when a function returns multiple values; from the callee's perspective, this parameter is treated like any other "special" parameter (e.g VMContext), to which, we give it a well know LocalSlot for later referencing when storing the return values. From the caller's perspective, extra stack space is created before the call, and the address to that space is passed in to the callee, via the return pointer parameter and once the function returns, the values in the created space are captured as values in the value stack.

Wasmtime GitHub notifications bot (Nov 14 2023 at 17:41):

fitzgen submitted PR review:

LGTM but I'm still a little sketched out by those disassembly changes where I feel like I'm only seeing one half of a change or something like that. Maybe you can explain what is going on?

Wasmtime GitHub notifications bot (Nov 14 2023 at 17:41):

fitzgen submitted PR review:

LGTM but I'm still a little sketched out by those disassembly changes where I feel like I'm only seeing one half of a change or something like that. Maybe you can explain what is going on?

Wasmtime GitHub notifications bot (Nov 14 2023 at 17:41):

fitzgen created PR review comment:

Could we document what the map closure is supposed to do here? Was not immediately obvious to me when reading the code.

Wasmtime GitHub notifications bot (Nov 14 2023 at 17:41):

fitzgen created PR review comment:

This seems like it is unnecessarily setting us up for porting issues in the future when we have a 32-bit target. Can we just put the size directly or use mem::size_of::<u32>() or something instead?

Wasmtime GitHub notifications bot (Nov 14 2023 at 17:41):

fitzgen created PR review comment:

Ah, great!

Wasmtime GitHub notifications bot (Nov 14 2023 at 17:41):

fitzgen created PR review comment:

Is this an expected/intended change in this first commit? I thought this was purely a refactoring to prep for multi-value, and I wouldn't expect any functional changes. Certainly not ones where we are changing from where we are pushing values to the stack when I don't see any corresponding changes in how we pop values from the stack.

But maybe this is addressed in follow up commits that I haven't gotten to yet...

Wasmtime GitHub notifications bot (Nov 14 2023 at 17:41):

fitzgen created PR review comment:

Documentation is out of sync with the code: it returns a set not an iterator.

Wasmtime GitHub notifications bot (Nov 14 2023 at 17:41):

fitzgen created PR review comment:

Ditto regarding map

Wasmtime GitHub notifications bot (Nov 14 2023 at 17:41):

fitzgen created PR review comment:

:eyes:

Wasmtime GitHub notifications bot (Nov 14 2023 at 17:41):

fitzgen created PR review comment:

debug_assert_eq! will give better diagnostics on panic.