Stream: git-wasmtime

Topic: wasmtime / PR #7707 winch: Multi-Value Part 2: Blocks

Wasmtime GitHub notifications bot (Dec 19 2023 at 21:11):

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

Follow-up to: https://github.com/bytecodealliance/wasmtime/pull/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: 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.

---

I've been continuously fuzzing this change for a bit now (~48hrs), and the fuzzer hasn't reported any new issues.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Dec 19 2023 at 21:11):

saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #7707.

Wasmtime GitHub notifications bot (Dec 19 2023 at 21:11):

saulecabrera requested elliottt for a review on PR #7707.

Wasmtime GitHub notifications bot (Dec 19 2023 at 21:11):

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

Wasmtime GitHub notifications bot (Dec 19 2023 at 21:11):

saulecabrera requested pchickey for a review on PR #7707.

Wasmtime GitHub notifications bot (Dec 19 2023 at 21:11):

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

Wasmtime GitHub notifications bot (Dec 19 2023 at 21:56):

saulecabrera edited PR #7707:

Follow-up to: https://github.com/bytecodealliance/wasmtime/pull/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 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.

---

I've been continuously fuzzing this change for a bit now (~48hrs), and the fuzzer hasn't reported any new issues.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Dec 19 2023 at 21:57):

saulecabrera edited PR #7707:

Follow-up to: https://github.com/bytecodealliance/wasmtime/pull/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 the ABIResults until they are actually needed rather than calculating everything 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.

---

I've been continuously fuzzing this change for a bit now (~48hrs), and the fuzzer hasn't reported any new issues.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Dec 19 2023 at 21:58):

saulecabrera edited PR #7707:

Follow-up to: https://github.com/bytecodealliance/wasmtime/pull/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 the ABIResults until they are actually needed rather than calculating everything 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 deferred 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.

---

I've been continuously fuzzing this change for a bit now (~48hrs), and the fuzzer hasn't reported any new issues.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Dec 19 2023 at 21:58):

saulecabrera edited PR #7707:

Follow-up to: https://github.com/bytecodealliance/wasmtime/pull/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 the ABIResults until they are actually needed rather than calculating everything 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 deferred 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.

---

I've been continuously fuzzing this change for a bit now (~48hrs), and the fuzzer hasn't reported any new issues.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Dec 20 2023 at 01:03):

github-actions[bot] commented on PR #7707:

Subscribe to Label Action

cc @fitzgen, @saulecabrera

<details>
This issue or pull request has been labeled: "fuzzing", "winch"

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

• fitzgen: fuzzing
• saulecabrera: winch

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

Learn more.
</details>

Wasmtime GitHub notifications bot (Jan 04 2024 at 18:48):

elliottt commented on PR #7707:

Sorry this has sat for so long! I'll get a review done by EOD today.

Wasmtime GitHub notifications bot (Jan 04 2024 at 22:34):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 04 2024 at 22:34):

elliottt created PR review comment:

Is it worth adding an assert here that the area is not Uninit?

Wasmtime GitHub notifications bot (Jan 04 2024 at 22:35):

elliottt edited PR review comment.

Wasmtime GitHub notifications bot (Jan 05 2024 at 00:26):

elliottt submitted PR review:

This looks great, thank you for all the comments!

Wasmtime GitHub notifications bot (Jan 05 2024 at 00:26):

elliottt submitted PR review:

This looks great, thank you for all the comments!

Wasmtime GitHub notifications bot (Jan 05 2024 at 00:26):

elliottt created PR review comment:

Why convert this to a debug assert?

Wasmtime GitHub notifications bot (Jan 08 2024 at 13:47):

saulecabrera updated PR #7707.

Wasmtime GitHub notifications bot (Jan 08 2024 at 13:47):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jan 08 2024 at 13:47):

saulecabrera created PR review comment:

I meant to convert it to an assert_eq! instead. I pushed a fixed that changes it to assert_eq instead.

Wasmtime GitHub notifications bot (Jan 08 2024 at 13:47):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Jan 08 2024 at 13:48):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jan 08 2024 at 13:48):

saulecabrera created PR review comment:

Good point, I added an assert!

Wasmtime GitHub notifications bot (Jan 08 2024 at 19:41):

saulecabrera updated PR #7707.

Wasmtime GitHub notifications bot (Jan 08 2024 at 19:51):

saulecabrera has enabled auto merge for PR #7707.

Wasmtime GitHub notifications bot (Jan 08 2024 at 20:32):

saulecabrera merged PR #7707.

Last updated: Dec 23 2024 at 12:05 UTC