Wasmtime GitHub notifications bot (Apr 09 2024 at 19:39): Wasmtime GitHub notifications bot (Apr 09 2024 at 19:39):elliottt opened PR #8319 from elliottt:trevor/function-call-arg-space to bytecodealliance:main:
Unconditionally reserve space for outgoing function call arguments in cranelift, allowing us to adjust SP once in the function prologue, instead of managing it explicitly around every function call.
<!--
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 (Apr 09 2024 at 19:45):elliottt updated PR #8319.
Wasmtime GitHub notifications bot (Apr 09 2024 at 19:45):elliottt updated PR #8319.
Wasmtime GitHub notifications bot (Apr 09 2024 at 19:46):elliottt updated PR #8319.
Wasmtime GitHub notifications bot (Apr 09 2024 at 21:09):elliottt updated PR #8319.
Wasmtime GitHub notifications bot (Apr 10 2024 at 15:50):elliottt updated PR #8319.
Wasmtime GitHub notifications bot (Apr 10 2024 at 18:56):elliottt updated PR #8319.
Wasmtime GitHub notifications bot (Apr 10 2024 at 20:47):elliottt updated PR #8319.
Wasmtime GitHub notifications bot (Apr 10 2024 at 20:53):elliottt edited PR #8319:
Instead of reserving stack space for stack arguments and return values at the call site that requires them, eagerly reserve the maximum amount of space needed in the function prologue. This change minimizes manipulation of the stack pointer, and in the case of tail calls only modifies the stack pointer to recover the outgoing argument space that is released by the tail callee. Additionally, eagerly reserving space for stack arguments in the non-s390x backends moves us closer to agreement on how to handle calls across all backends, as it already follows this frame layout.
This change sets us up to be able to remove the notion of a nominal SP from all backends, as the stack pointer will now be a stable point to address everything within the frame with a positive offset.
<!--
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 (Apr 10 2024 at 21:03):elliottt has marked PR #8319 as ready for review.
Wasmtime GitHub notifications bot (Apr 10 2024 at 21:03):elliottt requested abrown for a review on PR #8319.
Wasmtime GitHub notifications bot (Apr 10 2024 at 21:03):elliottt requested wasmtime-compiler-reviewers for a review on PR #8319.
Wasmtime GitHub notifications bot (Apr 10 2024 at 21:03):elliottt requested fitzgen for a review on PR #8319.
Wasmtime GitHub notifications bot (Apr 10 2024 at 21:03):elliottt requested wasmtime-core-reviewers for a review on PR #8319.
Wasmtime GitHub notifications bot (Apr 10 2024 at 21:03):elliottt requested wasmtime-core-reviewers for a review on PR #8319.
Wasmtime GitHub notifications bot (Apr 10 2024 at 21:04):elliottt edited PR #8319:
Instead of reserving stack space for stack arguments and return values at the call site that requires them, eagerly reserve the maximum amount of space needed in the function prologue. This change minimizes manipulation of the stack pointer, and in the case of tail calls only modifies the stack pointer to recover the outgoing argument space that is released by the tail callee. Additionally, eagerly reserving space for stack arguments in the non-s390x backends moves us closer to agreement on how to handle calls across all backends, as it already follows this frame layout.
This change sets us up to be able to remove the notion of a nominal SP from all backends, as the stack pointer will now be a stable point to address everything within the frame with a positive offset.
Co-authored-by: Jamey Sharp <jsharp@fastly.com>
<!--
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 (Apr 11 2024 at 08:33):I'm offline until next week if someone else can review this.
Wasmtime GitHub notifications bot (Apr 11 2024 at 10:52):cfallin commented on PR #8319:
I can take a look tomorrow when back in office!
Wasmtime GitHub notifications bot (Apr 11 2024 at 15:58):fitzgen submitted PR review:
LGTM with some minor nitpicks below
Wasmtime GitHub notifications bot (Apr 11 2024 at 15:58):fitzgen submitted PR review:
LGTM with some minor nitpicks below
Wasmtime GitHub notifications bot (Apr 11 2024 at 15:58):fitzgen created PR review comment:
Woo!
Wasmtime GitHub notifications bot (Apr 11 2024 at 15:58):fitzgen created PR review comment:
\o/
Wasmtime GitHub notifications bot (Apr 11 2024 at 15:58):fitzgen created PR review comment:
Does this ever get overwritten? At a glance, it seems like a pretty sus default to just ignore the requested space! Especially since the dual restore method is not a no-op! I feel like a comment explaining that the space is pre-reserved when the stack frame is pushed, and that restoring the argument area is only used for tail calls in practice since they are otherwise preserved, would be helpful somewhere in this viscinity.
Wasmtime GitHub notifications bot (Apr 11 2024 at 17:24):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Apr 11 2024 at 17:24):jameysharp created PR review comment:
The default implementations are not overridden in this PR, but we want to allow individual backends to implement an optimization where they minimize the amount of outgoing argument area they use across a function call. To do that, they need to know how much stack is actually needed for the current call. So we've ensured that _at least_ this much space is available during the prologue, which is why a no-op is okay here, and overriding can arrange that there is _less_ space available if desired. #8327 illustrates roughly how that would work, except that's for return-calls, where this optimization is actually required.
Yeah, we should write better comments here.
Wasmtime GitHub notifications bot (Apr 11 2024 at 17:53):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Apr 11 2024 at 17:53):jameysharp created PR review comment:
/// When setting up for a call, ensure that `space` bytes are available in the outgoing /// argument area on the stack. The specified amount of space is the minimum required for both /// arguments to that function, and any values returned through /// the stack. There are two reasonable implementations which each target can choose between: /// 1. At least this much space is reserved during the prologue and `StackAMode::OutgoingArg` refers /// to the bottom of the reserved area, so this method does nothing. /// 2. `StackAMode::OutgoingArg` refers to the top of this area, so this method needs to adjust the stack /// pointer to trim any unused portion of the bottom of the stack frame immediately before the call. /// `gen_restore_argument_area` needs to undo any stack pointer changes made here.(Sorry, I can't easily word-wrap this in the web interface.)
Wasmtime GitHub notifications bot (Apr 11 2024 at 18:12):elliottt updated PR #8319.
Wasmtime GitHub notifications bot (Apr 11 2024 at 18:14):elliottt updated PR #8319.
Wasmtime GitHub notifications bot (Apr 11 2024 at 19:27):elliottt merged PR #8319.
Last updated: Nov 22 2024 at 16:03 UTC