Skip to content

cranelift: Simplify spill/reload emission#8296

Merged
jameysharp merged 1 commit intobytecodealliance:mainfrom
jameysharp:simplify-spill-reload
Apr 3, 2024
Merged

cranelift: Simplify spill/reload emission#8296
jameysharp merged 1 commit intobytecodealliance:mainfrom
jameysharp:simplify-spill-reload

Conversation

@jameysharp
Copy link
Contributor

The gen_spill and gen_reload methods on Callee are used to emit appropriate moves between registers and the stack, as directed by the register allocator.

These moves always apply to a single register at a time, even if that register was originally part of a group of registers. For example, when an I128 is represented using two 64-bit registers, either of those registers may be spilled independently.

As a result, the load_spillslot/store_spillslot helpers were more general than necessary, which in turn required extra complexity in the gen_load_stack_multi/gen_store_stack_multi helpers. None of these helpers were used in any other context, so all that complexity was unnecessary.

Inlining all four helpers and then simplifying eliminates a lot of code without changing the output of the compiler.

These helpers were also the only uses of StackAMode::offset, so I've deleted that. While I was there, I also deleted StackAMode::get_type, which was introduced in #8151 and became unused again in #8246.

The `gen_spill` and `gen_reload` methods on `Callee` are used to emit
appropriate moves between registers and the stack, as directed by the
register allocator.

These moves always apply to a single register at a time, even if that
register was originally part of a group of registers. For example, when
an I128 is represented using two 64-bit registers, either of those
registers may be spilled independently.

As a result, the `load_spillslot`/`store_spillslot` helpers were more
general than necessary, which in turn required extra complexity in the
`gen_load_stack_multi`/`gen_store_stack_multi` helpers. None of these
helpers were used in any other context, so all that complexity was
unnecessary.

Inlining all four helpers and then simplifying eliminates a lot of code
without changing the output of the compiler.

These helpers were also the only uses of `StackAMode::offset`, so I've
deleted that. While I was there, I also deleted `StackAMode::get_type`,
which was introduced in bytecodealliance#8151 and became unused again in bytecodealliance#8246.
@jameysharp jameysharp requested a review from a team as a code owner April 3, 2024 18:21
@jameysharp jameysharp requested review from cfallin and removed request for a team April 3, 2024 18:21
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.

Good catch! I think this is leftover generality from a previous version of multi-reg handling; indeed we don't seem to need it anymore. I'm having a little trouble remembering whether the _multi variants were useful for arm32 previously (e.g., stack args); I suspect the rest of the ABI machinery should work without this though, so let's clean up and cross other bridges later :-)

@jameysharp jameysharp enabled auto-merge April 3, 2024 18:34
@jameysharp jameysharp added this pull request to the merge queue Apr 3, 2024
@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. labels Apr 3, 2024
Merged via the queue into bytecodealliance:main with commit 646169e Apr 3, 2024
@jameysharp jameysharp deleted the simplify-spill-reload branch April 3, 2024 19:22
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 Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants