Wasmtime GitHub notifications bot (Mar 20 2024 at 21:43): Wasmtime GitHub notifications bot (Mar 20 2024 at 21:43):elliottt requested abrown for a review on PR #8198.
Wasmtime GitHub notifications bot (Mar 20 2024 at 21:43):elliottt opened PR #8198 from elliottt:trevor/fix-cranelift-winch-returns to bytecodealliance:main:
Cranelift's Winch calling convention implementation was incorrect in its handling of return values in three ways:
- Winch returns the last return value in a register instead of the first,
- Winch doesn't align entries returned through the stack to 8-byte boundaries,
- and finally, Winch places the first returned value at the largest offset from the return location, instead of the smallest.
This PR addresses these three issues, enabling progress towards removing the trampoline implementations that are defined in terms of Winch's
MacroAssemblertrait.
<!--
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
-->
Wasmtime GitHub notifications bot (Mar 20 2024 at 21:43):elliottt requested wasmtime-compiler-reviewers for a review on PR #8198.
Wasmtime GitHub notifications bot (Mar 20 2024 at 21:43):elliottt requested alexcrichton for a review on PR #8198.
Wasmtime GitHub notifications bot (Mar 20 2024 at 21:43):elliottt requested saulecabrera for a review on PR #8198.
Wasmtime GitHub notifications bot (Mar 20 2024 at 21:46):alexcrichton submitted PR review:
Nice!
Would it perhaps also make sense to add a runtest exercising this to ensure at runtime the result works in Cranelift at least?
Wasmtime GitHub notifications bot (Mar 20 2024 at 21:47):elliottt edited PR #8198:
Cranelift's Winch calling convention implementation was incorrect in its handling of return values in three ways:
- Winch returns the last return value in a register instead of the first,
- Winch doesn't align entries returned through the stack to 8-byte boundaries,
- and finally, Winch places the first returned value at the largest offset from the return location, instead of the smallest.
This PR addresses these three issues, enabling progress towards removing the trampoline implementations that are defined in terms of Winch's
MacroAssemblertrait.This PR only fixes return value handling for the x64 backend, as the aarch64 backend is currently exempted from testing. For this reason I'm wondering if it would make more sense to completely disable the Winch calling convention for aarch64, and leave fixing that implementation for when we start putting more work into aarch64 in Winch.
<!--
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
-->
Wasmtime GitHub notifications bot (Mar 20 2024 at 21:48):elliottt commented on PR #8198:
Would it perhaps also make sense to add a runtest exercising this to ensure at runtime the result works in Cranelift at least?
This seems reasonable to me! I'll investigate adding one.
Wasmtime GitHub notifications bot (Mar 20 2024 at 21:58):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (Mar 20 2024 at 21:58):saulecabrera commented on PR #8198:
Oops, I didn't realize that Alex had already approved.
Wasmtime GitHub notifications bot (Mar 20 2024 at 22:07):elliottt updated PR #8198.
Wasmtime GitHub notifications bot (Mar 20 2024 at 22:15):elliottt updated PR #8198.
Wasmtime GitHub notifications bot (Mar 20 2024 at 22:15):elliottt requested fitzgen for a review on PR #8198.
Wasmtime GitHub notifications bot (Mar 20 2024 at 22:15):elliottt requested wasmtime-core-reviewers for a review on PR #8198.
Wasmtime GitHub notifications bot (Mar 20 2024 at 22:16):elliottt edited PR #8198:
Cranelift's Winch calling convention implementation was incorrect in its handling of return values in three ways:
- Winch returns the last return value in a register instead of the first,
- Winch doesn't align entries returned through the stack to 8-byte boundaries,
- and finally, Winch places the first returned value at the largest offset from the return location, instead of the smallest.
This PR addresses these three issues, enabling progress towards removing the trampoline implementations that are defined in terms of Winch's
MacroAssemblertrait.Additionally, this PR disables the Winch calling convention for Cranelift's
aarch64backend, as Winch support foraarch64is currently minimal.<!--
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
-->
Wasmtime GitHub notifications bot (Mar 20 2024 at 22:27):elliottt updated PR #8198.
Wasmtime GitHub notifications bot (Mar 20 2024 at 22:40):elliottt updated PR #8198.
Wasmtime GitHub notifications bot (Mar 20 2024 at 23:50):elliottt merged PR #8198.
Last updated: Dec 23 2024 at 13:07 UTC