Avoid copying the frame for tail calls on riscv64#8383
Avoid copying the frame for tail calls on riscv64#8383elliottt merged 4 commits intobytecodealliance:mainfrom
Conversation
github-actions
bot
added
cranelift
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
elliottt
force-pushed
the
trevor/tail-calls-riscv64
branch
from
8bb7ee8 to
6a72c2b
Compare
github-actions
bot
added
the
cranelift:area:machinst
elliottt
force-pushed
the
trevor/tail-calls-riscv64
branch
from
6a72c2b to
3350226
Compare
elliottt
requested review from
afonso360,
fitzgen and
jameysharp
and removed request for
a team
afonso360
left a comment
afonso360
left a comment
There was a problem hiding this comment.
👋 Hey,
I don't think I can do a very good review here, I don't remember a lot about how our frames are set up and haven't been following the rest of the tail calls work very closely. So I'd prefer if someone else could review this.
| let new_stack_words = new_stack_arg_size / 8; | ||
| let insts = 4 + 2 + 2 * new_stack_words; | ||
| let space_needed = insts * u32::try_from(Inst::UNCOMPRESSED_INSTRUCTION_SIZE).unwrap(); | ||
| if sink.island_needed(space_needed) { |
There was a problem hiding this comment.
Is this island check now done in some common code, or do we no longer need it? I remember having some issues with missing islands when we enabled compressed jumps since with the compressed extensions we have a very short jump range.
There was a problem hiding this comment.
Thanks for catching this, that check isn't being done in any generic code. I'll add this back in!
There was a problem hiding this comment.
On further inspection, there is generic code in VCode::emit that emits islands based on the backend's Inst::worst_case_size() estimate. Both gen_epilogue and these ReturnCall/ReturnCallInd pseudo-instructions need to fit within that worst-case size. These pseudo-instructions might be slightly longer than gen_epilogue right now but they're almost the same, so I think they don't need an explicit island check.
There was a problem hiding this comment.
I did some more refactoring, and the resulting code generated for a return_call or return_call_indirect should now be pretty close to the same size as gen_epilogue. Lots of chatting with @jameysharp has me convinced that we shouldn't need to check for island emission at this point anymore, and I've removed the manual resets at the call-sites for emit_return_call_common_sequence to ensure that we trigger an assert if this isn't true.
elliottt
force-pushed
the
trevor/tail-calls-riscv64
branch
from
e381d21 to
2d9fc23
Compare
This reverts the key parts of e3a08d4 (bytecodealliance#8151), because it turns out that we didn't need that abstraction. Several changes in the last month have enabled this: - bytecodealliance#8292 and then bytecodealliance#8316 allow us to refer to either incoming or outgoing argument areas in a (mostly) consistent way - bytecodealliance#8327, bytecodealliance#8377, and bytecodealliance#8383 demonstrate that we never need to delay writing stack arguments directly to their final location
elliottt
force-pushed
the
trevor/tail-calls-riscv64
branch
from
d993630 to
301524d
Compare
This reverts the key parts of e3a08d4 (bytecodealliance#8151), because it turns out that we didn't need that abstraction. Several changes in the last month have enabled this: - bytecodealliance#8292 and then bytecodealliance#8316 allow us to refer to either incoming or outgoing argument areas in a (mostly) consistent way - bytecodealliance#8327, bytecodealliance#8377, and bytecodealliance#8383 demonstrate that we never need to delay writing stack arguments directly to their final location prtest:full
This reverts the key parts of e3a08d4 (#8151), because it turns out that we didn't need that abstraction. Several changes in the last month have enabled this: - #8292 and then #8316 allow us to refer to either incoming or outgoing argument areas in a (mostly) consistent way - #8327, #8377, and #8383 demonstrate that we never need to delay writing stack arguments directly to their final location prtest:full
elliottt
force-pushed
the
trevor/tail-calls-riscv64
branch
from
301524d to
ba40e76
Compare
jameysharp
left a comment
jameysharp
left a comment
There was a problem hiding this comment.
I'm convinced this is all correct. Ship it!
3 participants
To mirror the implementation in the x64 backend, switch to eagerly reserving enough space in the incoming argument area for any tail call present in the function being compiled.
The diff here is large due to the change of representation for incoming arguments in the riscv64 specific
AModetype. Now that we reference incoming arguments with a negative offset from the beginning of the incoming argument area, all references to incoming arguments changed in the riscv64 filetests. These would have ended up being no-op changes in the disassembly, though I took this opportunity to make them SP relative now, to move closer to frame pointers not being strictly required for tail calls.As this is the last backend that was waiting to be migrated to the approach of resizing the incoming argument area in the function prologue, I've also deleted the shared code in
machinst/abi.rsthat's no longer needed.NOTE: this doesn't make the change to enable callee-save registers with the tail calling convention on aarch64, but that will be a relatively small change following this PR.