Stream: git-wasmtime

Topic: wasmtime / PR #9218 Winch multi values results

Wasmtime GitHub notifications bot (Sep 10 2024 at 10:19):

vulc41n opened PR #9218 from vulc41n:winch-multi-values to bytecodealliance:main:

Hey :wave:

This PR fixes how results are returned when using Winch. Cranelift uses x0-x7 and v0-v7 to store the first results, and then stores the remaining ones in the stack from sp to fp. Winch stores the last result in either x0 or v0, with the other results placed on the stack from fp to sp.

Thank you @saulecabrera for helping me figure this out :smiling_face:

<!--
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 (Sep 10 2024 at 10:19):

vulc41n requested wasmtime-compiler-reviewers for a review on PR #9218.

Wasmtime GitHub notifications bot (Sep 10 2024 at 10:19):

vulc41n requested fitzgen for a review on PR #9218.

Wasmtime GitHub notifications bot (Sep 10 2024 at 10:19):

vulc41n requested wasmtime-core-reviewers for a review on PR #9218.

Wasmtime GitHub notifications bot (Sep 10 2024 at 13:44):

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

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Sep 10 2024 at 15:55):

fitzgen submitted PR review:

Looks good, just a couple nitpicks below. Thanks!

Wasmtime GitHub notifications bot (Sep 10 2024 at 15:55):

fitzgen created PR review comment:

Since we are checking this condition so frequently, it may help to define an is_winch_return boolean at the start of this procedure that we can reuse throughout.

Wasmtime GitHub notifications bot (Sep 10 2024 at 15:55):

fitzgen created PR review comment:

Rather than checking for winch returns every iteration of the loop, can we initialize remaining_reg_vals such that this behavior happens automatically? I think that means initializing it to one when we are doing winch returns. That feels cleaner to me than adding edge case conditions to check in the middle of the body of the register assignment loop.

Wasmtime GitHub notifications bot (Sep 10 2024 at 16:13):

saulecabrera submitted PR review:

Left one small nit as well.

Wasmtime GitHub notifications bot (Sep 10 2024 at 16:13):

saulecabrera created PR review comment:

We have this exact implementation in the x64 backend. Maybe it's worth factoring it out into a function shared between both backends?

Wasmtime GitHub notifications bot (Sep 10 2024 at 16:21):

vulc41n submitted PR review.

Wasmtime GitHub notifications bot (Sep 10 2024 at 16:21):

vulc41n created PR review comment:

If remaining_reg_vals is one, it will put the first result in the registry, not the last one. I can add another variable named skip_reg_vals which is zero except when processing winch returns, then it is params.len() - 1. What do you think ?

Wasmtime GitHub notifications bot (Sep 10 2024 at 16:51):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 10 2024 at 16:51):

fitzgen created PR review comment:

Ah I missed that detail. In that case, I think it is fine as-is.

Wasmtime GitHub notifications bot (Sep 11 2024 at 09:12):

vulc41n updated PR #9218.

Wasmtime GitHub notifications bot (Sep 11 2024 at 20:15):

fitzgen submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Sep 11 2024 at 20:30):

fitzgen merged PR #9218.

Last updated: Nov 22 2024 at 16:03 UTC