winch: Multi-Value Part 2: Blocks#7707
Merged
saulecabrera merged 2 commits intoJan 8, 2024
Merged
Conversation
saulecabrera
requested review from
elliottt and
pchickey
and removed request for
a team
github-actions
Bot
added
fuzzing
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "fuzzing", "winch"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Member
|
Sorry this has sat for so long! I'll get a review done by EOD today. |
elliottt
reviewed
Jan 4, 2024
elliottt
approved these changes
Jan 5, 2024
Member
elliottt
left a comment
elliottt
left a comment
There was a problem hiding this comment.
This looks great, thank you for all the comments!
This commit adds support for the Multi-Value proposal for blocks. In general, this change, introduces multiple building blocks to enable supporting arbitrary params and results in blocks: * `BlockType`: Introduce a block type, to categorize the type of each block, this makes it easier to categorize blocks per type and also makes it possible to defer the calculation of the `ABIResults` until they are actually needed rather than calculating everyghing upfront even though they might not be needed (when in an unreachable state). * Push/pop operations are now frame aware. Given that each `ControlStackFrame` contains all the information needed regarding params and results, this change moves the the implementation of the push and pop operations to the `ControlStackFrame` struct. * `StackState`: this struct holds the entry and exit invariants of each block; these invariants are pre-computed when entering the block and used throughout the code generation, to handle params, results and assert the respective invariants. In terms of the mechanics of the implementation: when entering each block, if there are results on the stack, the expected stack pointer offsets will be calculated via the `StackState`, and the `target_offset` will be used to create the block's `RetArea`. Note that when entering the block and calculating the `StackState` no space is actually reserved for the results, any space increase in the stack is deffered until the results are popped from the value stack via `ControlStackFrame::pop_abi_results`. The trickiest bit of the implementation is handling constant values that need to be placed on the right location on the machine stack. Given that constants are generally not spilled, this means that in order to keep the machine and value stack in sync (spilled-values-wise), values must be shuffled to ensure that constants are placed in the expected location results wise. See the comment in `ControlStackFrame::adjust_stack_results` for more details.
saulecabrera
force-pushed
the
winch-multi-value-blocks
branch
from
9f593ec to
9d9e038
Compare
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
2 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.
Follow-up to: #7535
This commit adds support for the Multi-Value proposal for blocks.
In general, this change, introduces multiple building blocks to enable supporting arbitrary params and results in blocks:
BlockType: This enum makes it easier to categorize blocks per type and also makes it possible to defer the calculation of theABIResultsuntil they are actually needed rather than calculating everything upfront even though they might not be needed (when in an unreachable state).ControlStackFramecontains all the information needed regarding params and results, this change moves the the implementation of the push and pop operations to theControlStackFramestruct.StackState: this struct holds the entry and exit invariants of each block; these invariants are pre-computed when entering the block and used throughout the code generation, to handle params, results and assert the respective invariants.In terms of the mechanics of the implementation: when entering each block, if there are results on the stack, the expected stack pointer offsets will be calculated via the
StackState, and thetarget_offsetwill be used to create the block'sRetArea. Note that when entering the block and calculating theStackStateno space is actually reserved for the results, any space increase in the stack is deferred until the results are popped from the value stack viaControlStackFrame::pop_abi_results.The trickiest bit of the implementation is handling constant values that need to be placed on the right location on the machine stack. Given that constants are generally not spilled, this means that in order to keep the machine and value stack in sync (spilled-values-wise), values must be shuffled to ensure that constants are placed in the expected location results-wise. See the comment in
ControlStackFrame::adjust_stack_resultsfor more details.I've been continuously fuzzing this change for a bit now (~48hrs), and the fuzzer hasn't reported any new issues.