-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cranelift: Implement tail calls for x86_64 #6635
Conversation
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
1c6f497
to
047bed7
Compare
👋 Hey, I'm not sure if this is ready or not, but I implemented return call's on fuzzgen (#6641) and it found something. It is possible that this is related to #6640, but it feels quite different. Here we get a segfault, and we don't need the stack probe. Testcase
|
@afonso360 still haven't dug in fully yet, but it is interesting that the second function claims the first function's signature is something other than what it the function is declared as: the second function says the first three arguments are |
Oh boy, I removed I've updated the example above to remove them, and verified that it still reproduces the original issue. |
Fixed that fuzz bug, thanks again for reporting it @afonso360! Turns out that my codegen-the-arguments sequence was accidentally skipping "hidden" extra parameters like return pointers. Fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
for ret_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!
Co-Authored-By: Jamey Sharp <jsharp@fastly.com>
Will do this in an immediate follow up. Thanks for the review! |
No description provided.