Wasmtime GitHub notifications bot (May 15 2024 at 22:07): Wasmtime GitHub notifications bot (May 15 2024 at 22:07):elliottt requested cfallin for a review on PR #8631.
Wasmtime GitHub notifications bot (May 15 2024 at 22:07):elliottt requested wasmtime-compiler-reviewers for a review on PR #8631.
Wasmtime GitHub notifications bot (May 15 2024 at 22:07):elliottt opened PR #8631 from elliottt:trevor/remove-virtual-sp-offset to bytecodealliance:main:
After the changes required to support tail calls, the general frame layout in cranelift now always includes a stack allocation that's large enough to hold all outgoing args/return values. An effect of this change is that the only place that the virtual sp offset was meaningfully changed was in the function prologue, where it was always updated by the constant amount required for the outgoing arguments area. As the only other place that the virtual sp offset was modified was in the handling for call instructions to functions that used the tail-call abi, the need for the virtual sp offset is no longer obvious.
This PR removes the virtual sp offset from all backends, and instead maintains the invariant that SP always points to the end of the stack frame, right after the outgoing arguments area. This does require the call pseudo-instruction to decrement SP after a call to a tail-call function, as those free their incoming argument area, but I believe this change to be a benefit, as we now no longer directly manipulate SP outside of the function prologue and epilogue.
<!--
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 (May 15 2024 at 22:09):elliottt edited PR #8631:
After the changes required to support tail calls, the general frame layout in cranelift now always includes a stack allocation that's large enough to hold all outgoing args/return values. An effect of this change is that the only place that the virtual sp offset was meaningfully changed was in the function prologue, where it was always updated to include the size of the outgoing arguments area. As the only other place that the virtual sp offset was modified was in the handling for call instructions to functions that used the tail-call abi, the need for the virtual sp offset is no longer obvious.
This PR removes the virtual sp offset from all backends, and instead maintains the invariant that SP always points to the end of the stack frame, right after the outgoing arguments area. This does require the call pseudo-instruction to decrement SP after a call to a tail-call function, as those free their incoming argument area, but I believe this change to be a benefit, as we now no longer directly manipulate SP outside of the function prologue and epilogue.
<!--
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 (May 15 2024 at 23:27):elliottt updated PR #8631.
Wasmtime GitHub notifications bot (May 15 2024 at 23:30):elliottt updated PR #8631.
Wasmtime GitHub notifications bot (May 15 2024 at 23:31):elliottt updated PR #8631.
Wasmtime GitHub notifications bot (May 15 2024 at 23:34):elliottt edited PR #8631:
After the changes required to support tail calls, the general frame layout in cranelift always includes a stack allocation that's large enough to hold all outgoing args/return values. An effect of this change is that the only place that the virtual SP offset (the offset from the current value of SP to the nominal SP) was meaningfully changed was in the function prologue, where it was always updated to include the size of the outgoing argument area. As the only other place that the virtual SP offset was modified was in the handling for call instructions to functions that used the tail-call abi, the need for the virtual SP offset is no longer obvious.
This PR removes the virtual SP offset from all backends, and instead maintains the invariant that SP always points to the end of the stack frame, right after the outgoing arguments area. This does require the call pseudo-instruction to decrement SP after a call to a tail-call function, as those free their incoming argument area, but I believe this change to be a benefit as we now no longer directly manipulate SP outside of the function prologue and epilogue.
<!--
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 (May 15 2024 at 23:40):elliottt edited PR #8631:
After the changes required to support tail calls, the general frame layout in cranelift always includes a stack allocation that's large enough to hold all outgoing args/return values. An effect of this change is that the only place that the virtual SP offset (the offset from the current value of SP to the nominal SP) was meaningfully changed was in the function prologue, where it was always updated to include the size of the outgoing argument area. As the only other place that the virtual SP offset was modified was in the handling for call instructions to functions that used the tail-call abi, the need for the virtual SP offset is no longer obvious.
This PR removes the virtual SP offset from all backends, and instead maintains the invariant that SP always points to the end of the stack frame, right after the outgoing arguments area. This does require the call pseudo-instruction to decrement SP after a call to a tail-call function, as those free their incoming argument area, but I believe this change to be a benefit as we now no longer directly manipulate SP outside of the function prologue and epilogue.
Looking forward a bit, another benefit of this change is that it better sets us up to handle compiling without frame pointers: as SP is now a consistent base that we can address anything in the frame from, we're no longer reliant on FP to address some parts of the frame.
<!--
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 (May 16 2024 at 16:45):cfallin submitted PR review:
Excellent simplification, thanks!
Wasmtime GitHub notifications bot (May 16 2024 at 16:45):cfallin submitted PR review:
Excellent simplification, thanks!
Wasmtime GitHub notifications bot (May 16 2024 at 16:45):cfallin created PR review comment:
(amend comment) ...and are generated after the register allocator has run and thus cannot have register allocator-inserted references to SP offsets.
Wasmtime GitHub notifications bot (May 16 2024 at 16:45):cfallin created PR review comment:
s/argumetn/argument/ (and below)
Wasmtime GitHub notifications bot (May 16 2024 at 17:11):elliottt updated PR #8631.
Wasmtime GitHub notifications bot (May 16 2024 at 17:11):elliottt submitted PR review.
Wasmtime GitHub notifications bot (May 16 2024 at 17:11):elliottt created PR review comment:
Hello
argumetn, my old friend.
Wasmtime GitHub notifications bot (May 16 2024 at 18:38):elliottt merged PR #8631.
Last updated: Nov 22 2024 at 17:03 UTC