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.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
vulc41n requested wasmtime-compiler-reviewers for a review on PR #9218.
vulc41n requested fitzgen for a review on PR #9218.
vulc41n requested wasmtime-core-reviewers for a review on PR #9218.
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>
fitzgen submitted PR review:
Looks good, just a couple nitpicks below. Thanks!
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.
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.
saulecabrera submitted PR review:
Left one small nit as well.
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?
vulc41n submitted PR review.
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 namedskip_reg_vals
which is zero except when processing winch returns, then it isparams.len() - 1
. What do you think ?
fitzgen submitted PR review.
fitzgen created PR review comment:
Ah I missed that detail. In that case, I think it is fine as-is.
vulc41n updated PR #9218.
fitzgen submitted PR review:
Thanks!
fitzgen merged PR #9218.
Last updated: Dec 23 2024 at 12:05 UTC