Wasmtime GitHub notifications bot (Jun 23 2023 at 16:30): Wasmtime GitHub notifications bot (Jun 23 2023 at 16:30):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
-->
Wasmtime GitHub notifications bot (Jun 23 2023 at 16:30):fitzgen requested wasmtime-compiler-reviewers for a review on PR #6635.
Wasmtime GitHub notifications bot (Jun 23 2023 at 16:30):fitzgen requested abrown for a review on PR #6635.
Wasmtime GitHub notifications bot (Jun 23 2023 at 16:52):fitzgen requested cfallin for a review on PR #6635.
Wasmtime GitHub notifications bot (Jun 23 2023 at 17:10):fitzgen updated PR #6635.
Wasmtime GitHub notifications bot (Jun 23 2023 at 18:16):fitzgen updated PR #6635.
Wasmtime GitHub notifications bot (Jun 26 2023 at 21:02):fitzgen updated PR #6635.
Wasmtime GitHub notifications bot (Jun 26 2023 at 23:13):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 26 2023 at 23:13):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 26 2023 at 23:13):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?
Wasmtime GitHub notifications bot (Jun 26 2023 at 23:13):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?
Wasmtime GitHub notifications bot (Jun 26 2023 at 23:13):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).
Wasmtime GitHub notifications bot (Jun 26 2023 at 23:13):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.
Wasmtime GitHub notifications bot (Jun 26 2023 at 23:16):cfallin created PR review comment:
Ah, actually, after reading further, I see that this is
fpwhich is set up in the lowering helper to be a copy (MovFromPReg) ofrbp, so this makes sense.
Wasmtime GitHub notifications bot (Jun 26 2023 at 23:19):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!
Wasmtime GitHub notifications bot (Jun 27 2023 at 16:28):fitzgen created PR review comment:
That's correct. Added a comment.
Wasmtime GitHub notifications bot (Jun 27 2023 at 16:29):fitzgen created PR review comment:
Done.
Wasmtime GitHub notifications bot (Jun 27 2023 at 16:30):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.
Wasmtime GitHub notifications bot (Jun 27 2023 at 16:31):fitzgen created PR review comment:
(Oh I just hadn't refreshed the tab. Doh.)
Wasmtime GitHub notifications bot (Jun 27 2023 at 16:40):fitzgen updated PR #6635.
Wasmtime GitHub notifications bot (Jun 27 2023 at 16:41):fitzgen has enabled auto merge for PR #6635.
Wasmtime GitHub notifications bot (Jun 27 2023 at 17:56):fitzgen merged PR #6635.
Last updated: Oct 23 2024 at 20:03 UTC