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

riscv64: Improve base select lowering #8703

Merged
merged 1 commit into from
May 31, 2024

Conversation

afonso360
Copy link
Contributor

👋 Hey,

This PR is an attempt at improving the base lowering for the select instruction in the RISC-V backend. It reduces one instruction from the current lowering.

Currently our selects have the following instructions:

b{cond} rc, 0xc
mv rd, rx
j 8
mv rd, ry

With this change we now produce the following:

mv rd, rx
b{cond} rc, 8
mv rd, ry

So, we lose the jump instruction, but one of the moves becomes unconditional. This to me feels like it's worth it, but I have no data to prove one way or the other, and I think if i try to measure it with sightglass it probably won't detect this change.

One of the benefits of this lowering is that now if one of the input registers is the same as the destination register we can do a two instruction select, like so (assuming rx = rd):

b{cond} rc, 8
mv rd, ry

I've had to do some regalloc surgery to make that possible which makes some other cases get worse regalloc.

This is built on top of #8695, but only the last commit is relevant.

@afonso360 afonso360 added cranelift Issues related to the Cranelift code generator cranelift:area:riscv64 Issues related to the RISC-V 64 backend. labels May 29, 2024
@afonso360 afonso360 requested review from a team as code owners May 29, 2024 13:55
@afonso360 afonso360 requested review from abrown and removed request for a team May 29, 2024 13:55
Comment on lines +93 to +94
; mv a4, a0
; mv a0, a4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now sometimes get this funky output from regalloc. I think I understand why it's doing this, but maybe there's some way to avoid this?


As a side note, maybe we could force the destination register to always be the same as one of the inputs. We would always emit the 2 instruction select and let regalloc emit the move beforehand if it finds it beneficial.

I'm not sure if this is better or not.

@github-actions github-actions bot added cranelift:meta Everything related to the meta-language. isle Related to the ISLE domain-specific language labels May 29, 2024
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "cranelift:area:riscv64", "cranelift:meta", "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.

@abrown
Copy link
Contributor

abrown commented May 29, 2024

I'm positive toward this change but someone more familiar with RISC-V should probably review this?

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.

Last commit LGTM, thanks! Happy to land once #8695 does and this rebases.

@afonso360 afonso360 enabled auto-merge May 31, 2024 16:36
@afonso360 afonso360 added this pull request to the merge queue May 31, 2024
Merged via the queue into bytecodealliance:main with commit d65ccd8 May 31, 2024
36 checks passed
@afonso360 afonso360 deleted the riscv-better-select branch May 31, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:riscv64 Issues related to the RISC-V 64 backend. cranelift:meta Everything related to the meta-language. 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.

3 participants