Wasmtime GitHub notifications bot (Oct 07 2022 at 00:17): Wasmtime GitHub notifications bot (Oct 07 2022 at 00:17):cfallin requested elliottt for a review on PR #5029.
Wasmtime GitHub notifications bot (Oct 07 2022 at 00:17):cfallin opened PR #5029 from cherrypick-2.0.0-pr5023 to release-2.0.0:
- Fix StructReturn handling: properly mark the clobber, and offset actual rets.
The legalization of
StructReturnwas causing issues in the new call-handling code: theStructReturnret was included in theSigDataas if it were an actual CLIF-level return value, but it is not.Prior to using regalloc constraints for return values, we unconditionally included rax (or the architecture's usual return register) as a def, so it would be properly handled as "clobbered" by the regalloc. With the new scheme, we include defs on the call only for CLIF-level outputs. Callees with
StructReturnargs were thus not known to clobber the return-value register, and values might be corrupted.This PR updates the code to include a
StructReturnret as a clobber rather than a returned value in the relevant spots. I observed it causing saves/restores of rax in some CLIF that @bjorn3 provided me, but I was having difficulty minimizing this into a test-case that I would be comfortable including as a precise-output case (including the whole thing verbatim would lock down a bunch of other irrelevant details and cause test-update noise later). If we can find a more minimized example I'm happy to include it as a filetest.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Oct 07 2022 at 00:18):cfallin edited PR #5029 from cherrypick-2.0.0-pr5023 to release-2.0.0:
The legalization of
StructReturnwas causing issues in the new call-handling code: theStructReturnret was included in theSigDataas if it were an actual CLIF-level return value, but it is not.Prior to using regalloc constraints for return values, we unconditionally included rax (or the architecture's usual return register) as a def, so it would be properly handled as "clobbered" by the regalloc. With the new scheme, we include defs on the call only for CLIF-level outputs. Callees with
StructReturnargs were thus not known to clobber the return-value register, and values might be corrupted.This PR updates the code to include a
StructReturnret as a clobber rather than a returned value in the relevant spots. I observed it causing saves/restores of rax in some CLIF that @bjorn3 provided me, but I was having difficulty minimizing this into a test-case that I would be comfortable including as a precise-output case (including the whole thing verbatim would lock down a bunch of other irrelevant details and cause test-update noise later). If we can find a more minimized example I'm happy to include it as a filetest.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Oct 07 2022 at 00:21):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Oct 07 2022 at 02:41):cfallin updated PR #5029 from cherrypick-2.0.0-pr5023 to release-2.0.0.
Wasmtime GitHub notifications bot (Oct 07 2022 at 04:37):cfallin merged PR #5029.
Last updated: Nov 22 2024 at 17:03 UTC