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: Emit argument location uses eagerly in gen_arg #8398

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

jameysharp
Copy link
Contributor

This reverts the key parts of e3a08d4 (#8151), because it turns out that we didn't need that abstraction.

Several changes in the last month have enabled this:

@jameysharp jameysharp requested a review from a team as a code owner April 18, 2024 06:42
@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:x64 Issues related to x64 codegen labels Apr 18, 2024
This reverts the key parts of e3a08d4
(bytecodealliance#8151), because it turns out that we didn't need that abstraction.

Several changes in the last month have enabled this:

- bytecodealliance#8292 and then bytecodealliance#8316 allow us to refer to either incoming or outgoing
  argument areas in a (mostly) consistent way

- bytecodealliance#8327, bytecodealliance#8377, and bytecodealliance#8383 demonstrate that we never need to delay
  writing stack arguments directly to their final location

prtest:full
Copy link
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

Awesome! Now that I'm thinking about it, I believe we added this specifically for GrowArgumentArea and ShrinkArgumentArea, both of which no longer exist 🎉

@jameysharp jameysharp added this pull request to the merge queue Apr 18, 2024
@jameysharp
Copy link
Contributor Author

Of the several variations on the theme that we've considered, this one was even earlier than the grow/shrink frame approach. We intended to use regalloc2's parallel move resolver to shuffle values between stack slots, registers, and the argument area, to capture all the work of restoring clobber-saves, putting arguments in the right places, and getting the return address in the right place too. That turned out to be complicated. Fortunately, the fact that Ulrich's suggestion (proactively resizing the argument area) works shows we will never have to do parallel moves.

Merged via the queue into bytecodealliance:main with commit 86806a8 Apr 18, 2024
48 checks passed
@jameysharp jameysharp deleted the undo-argloc-moves branch April 18, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants