fitzgen opened PR #6635 from fitzgen:x64-tail-calls
to bytecodealliance:main
:
<!--
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
-->
fitzgen requested wasmtime-compiler-reviewers for a review on PR #6635.
fitzgen requested abrown for a review on PR #6635.
fitzgen requested cfallin for a review on PR #6635.
fitzgen updated PR #6635.
fitzgen updated PR #6635.
fitzgen updated PR #6635.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
A comment here noting why not delegate to
Inst::JmpKnown
(in the usual pseudoinst way, i.e.let inst = ...; inst.emit(...);
) would be good. I think the main reason is that we have different metadata in this case, in particular we don't have a label for the target but rather a different kind of relocation; is that right?
cfallin created PR review comment:
possibly just a semantic quibble but "pop FP" confused me here: to pop means to adjust SP, but here we're restoring FP's old value without adjusting the stack at all. Could we write it that way ("restore FP") instead?
cfallin created PR review comment:
Commented-out lines here -- we should restore these or else delete them if we decide we don't need them (here and below).
cfallin created PR review comment:
I'm possibly confused here, but: isn't this using the wrong FP, i.e., the value we already restored above, to whatever the original caller had in RBP when we were called? I think we want to use our FP as a frame of reference here.
cfallin created PR review comment:
Ah, actually, after reading further, I see that this is
fp
which is set up in the lowering helper to be a copy (MovFromPReg
) ofrbp
, so this makes sense.
cfallin submitted PR review:
Overall this looks totally reasonable to me -- excited to see the final piece fall into place, at least for x86-64!
In addition to comments above I think it would be nice to see the "extra ret-addr movement" go away for the overwhelmingly common case (no stack args) either in this PR or in an immediate followup -- it seems to me that it shouldn't be too much work (tens of lines of code?
Option<Reg>
forret_addr
, and check if old_size == new_size?) and it's an important (in)efficiency to clean up soon when using tail-calls as a sort of fine-grained control flow mechanism (BB jumps between funclets etc).Otherwise, happy to see this merged with comments addressed!
fitzgen created PR review comment:
That's correct. Added a comment.
fitzgen created PR review comment:
Done.
fitzgen created PR review comment:
This is our original FP, saved in a temporary. I saw the email for your comment about how you understood after reading more of the code, but I can't actually find that comment here in this review, so I'm mostly just commenting here for posterity.
fitzgen created PR review comment:
(Oh I just hadn't refreshed the tab. Doh.)
fitzgen updated PR #6635.
fitzgen has enabled auto merge for PR #6635.
fitzgen merged PR #6635.
Last updated: Dec 23 2024 at 12:05 UTC