Skip to content

riscv64: Add Zba extension instructions #6087

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

Merged
merged 18 commits into from
Mar 23, 2023

Conversation

afonso360
Copy link
Contributor

👋 Hey,

This PR adds the instructions present in the Zba extension to the RISC-V 64 backend. The Zba extension contains instructions for address generation.

Here's a quick summary:

Mnemonic Description
add.uw rd, rs1, rs2 Add unsigned word
sh1add rd, rs1, rs2 Shift left by 1 and add
sh1add.uw rd, rs1, rs2 Shift unsigned word left by 1 and add
sh2add rd, rs1, rs2 Shift left by 2 and add
sh2add.uw rd, rs1, rs2 Shift unsigned word left by 2 and add
sh3add rd, rs1, rs2 Shift left by 3 and add
sh3add.uw rd, rs1, rs2 Shift unsigned word left by 3 and add
slli.uw rd, rs1, imm Shift-left unsigned word (Immediate)

Besides directly matching the above, we also add the zext.w mnemnoic which is essentially add.uw rd, rs, zero.

I've left this fuzzing overnight on a riscv64 machine and all seems ok so far.

@afonso360 afonso360 added the cranelift:area:riscv64 Issues related to the RISC-V 64 backend. label Mar 22, 2023
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Mar 22, 2023
Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

I have a few requests but for the most part I think this looks correct!

@afonso360
Copy link
Contributor Author

Thanks for reviewing this! I've also added a bunch more rules to the sextend+arithmetic pattern, those are all in the last commit. Let me know if you'd prefer that split into it's own PR.

Even with all those rules, I think they can still be generalized further. Especially the shifts with const imm, but I noticed that I could also cleanup the regular shift rules in the same way, so I've left those improvements for a future PR.

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! One tiny whitespace fix but I think this is good to go.

A note for future work: the riscv64 implementation of Imm12::maybe_from_u64 has issues similar to the aarch64 version with assuming that the high bits of an iconst are sign-extended, when we want them to be zero-extended instead. I don't think this is a correctness issue, just the immediate-operand rules may not always match when you expect them to.

@afonso360
Copy link
Contributor Author

afonso360 commented Mar 23, 2023

I found another issue with the new shift rules, since they can have a i128 RHS we need to explicitly access the value regs. I've updated that commit, but I'm going to run the icache fuzzer for a bit to doublecheck that I haven't missed any other cases.

A note for future work: the riscv64 implementation of Imm12::maybe_from_u64 has issues similar to the aarch64 version with assuming that the high bits of an iconst are sign-extended, when we want them to be zero-extended instead. I don't think this is a correctness issue, just the immediate-operand rules may not always match when you expect them to.

🤔 Yeah, I should look into that.

@afonso360 afonso360 added this pull request to the merge queue Mar 23, 2023
Merged via the queue into bytecodealliance:main with commit 602ff71 Mar 23, 2023
@afonso360 afonso360 deleted the riscv-zba branch March 23, 2023 20:36
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 Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants