-
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
Various cleanups to the ABI handling code #8875
Conversation
acf4cb1
to
42dee0c
Compare
Subscribe to Label Actioncc @cfallin, @fitzgen, @saulecabrera
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "isle", "winch"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
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.
I read through this today and it all looks pretty reasonable. I feel like I need to review the safety bits, such as some of that register code, some more, though. Or maybe @jameysharp or @elliottt if you want to jump in?
4787e67
to
6766a06
Compare
I'm excited to take a look next week if nobody else gets to this first! |
I tried to review this and got kind of lost. I really appreciate that there are nicely separated commits, but I think this would be easier to review if those were fully separate PRs, if that's not too much trouble? It would be especially helpful if you can explain more about the motivation for each change in the commit/PR description. |
Split out #8903 and added some motivation to the commit messages. I will rebase this PR once it merges. |
Rebased |
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.
Looks reasonable, thanks! Just one nit below.
It doesn't seem useful and in several locations we already use a different Opcode from what the user actually wrote. For example for tls_value, the implicit stackprobe or the implicit memcpy call for struct args. This also makes the recorded call sites more accurately represent the actual set of call sites by avoiding special casing of pseudo instructions that happen to include a call.
Instead remove the actual set of defs from the clobber set once they are known. This reduces the risk of getting the clobber set wrong and makes it easier to handle cases where the exact set is not known in advance.
I originally did these in the process of adding exception support to Cranelift, but I've extracted it out that branch and rebased it as these changes should be less controversial and it will help with rebasing the exception support branch in the future.