Always reserve space for outgoing args#8319
Conversation
github-actions
bot
added
cranelift
elliottt
force-pushed
the
trevor/function-call-arg-space
branch
4 times, most recently
from
1691790 to
fdaaf62
Compare
github-actions
bot
added
the
cranelift:area:aarch64
elliottt
force-pushed
the
trevor/function-call-arg-space
branch
from
fdaaf62 to
3f8cea2
Compare
Co-authored-by: Jamey Sharp <jsharp@fastly.com>
elliottt
force-pushed
the
trevor/function-call-arg-space
branch
from
3f8cea2 to
fb5f583
Compare
elliottt
requested review from
a team,
abrown and
fitzgen
and removed request for
a team
|
I'm offline until next week if someone else can review this. |
|
I can take a look tomorrow when back in office! |
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
LGTM with some minor nitpicks below
| abi.emit_stack_pre_adjust(ctx); | ||
|
|
| for inst in retval_insts { | ||
| ctx.emit(inst); | ||
| } | ||
| abi.emit_stack_post_adjust(ctx); |
| /// When setting up for a call, ensure that `space` bytes are available in the outgoing | ||
| /// argument area on the stack for arguments to that function, and any values returned through | ||
| /// the stack. | ||
| fn gen_reserve_argument_area(_space: u32) -> SmallInstVec<Self::I> { | ||
| smallvec![] | ||
| } |
There was a problem hiding this 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.
There was a problem hiding this 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.
Co-authored-by: Jamey Sharp <jamey@minilop.net>
5 participants
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