Skip to content
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: Refactor return_call[_indirect] lowering #6666

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jun 28, 2023

Commons up some code paths and sets the stage for other architectures. This should have fewer calls back and forth between architecture specific and independent bits of code, which I have found hard to keep track of. Now, lowering tail calls is done in architecture specific code that can call out to architecture independent helpers as needed. Before it was architecture independent code that would call architecture specific hooks that would call architecture independent helpers. Too much stuff split across too many layers. This new approach removes at least one layer of indirection and unnecessarily confusing abstraction.

Commons up some code paths and sets the stage for other architectures. This
should have fewer calls back and forth between architecture specific and
independent bits of code, which I have found hard to keep track of. Now,
lowering tail calls is done in architecture specific code that can call out to
architecture independent helpers as needed. Before it was architecture
independent code that would call architecture specific hooks that would call
architecture independent helpers. Too much stuff split across too many
layers. This new approach removes at least one layer of indirection and
unnecessarily confusing abstraction.
@fitzgen fitzgen requested a review from cfallin June 28, 2023 20:56
@fitzgen fitzgen requested a review from a team as a code owner June 28, 2023 20:56
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen isle Related to the ISLE domain-specific language labels Jun 28, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

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:

  • cfallin: isle
  • fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me, thanks!

@fitzgen and I talked a bit about shared vs arch-specific ABI code and finding the right balance here. Originally we had copied-and-pasted abi.rs for both backends at the time (x64 and aarch64) and keeping them in-sync was tedious and error-prone; #2128 and #2142 merged the two into a shared common "vanilla ABI" with hooks. While that made sense at the time, we've had a significant combinatorial complexity jump since then with (4 ISAs) x (more calling conventions) x (more features), and empirically the overlap is shrinking as we add more special cases and additional hook points. So instead perhaps it makes sense to have arch-specific code at the top level (called directly from arch-specific lowering rules) and then factor out common helpers where it makes sense. This PR is a first step toward that and we'll hopefully move further that way in the future.

@fitzgen fitzgen added this pull request to the merge queue Jun 28, 2023
Merged via the queue into bytecodealliance:main with commit 1191091 Jun 29, 2023
@fitzgen fitzgen deleted the refactor-lower-return-calls branch June 29, 2023 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants