Page MenuHomePhabricator

[RISCV] Add intrinsic for Zbt extension
ClosedPublic

Authored by Chenbing.Zheng on Jan 17 2022, 5:05 AM.

Details

Summary

RV32: fsl, fsr, fsri

RV64: fsl, fsr, fsri, fslw, fsrw, fsriw

Diff Detail

Event Timeline

Chenbing.Zheng created this revision.Jan 17 2022, 5:05 AM
Chenbing.Zheng requested review of this revision.Jan 17 2022, 5:05 AM
craig.topper added inline comments.Jan 17 2022, 9:08 AM
clang/include/clang/Basic/BuiltinsRISCV.def
65

cmov and cmix don't builtins don't seem very useful. The compiler can already match them from IR operations that can be expressed in C.

clang/lib/CodeGen/CGBuiltin.cpp
18967

We shouldn't have an fsri builtin. The fsr builtin is sufficient. And riscv_fsri looks unhandled in the backend.

llvm/lib/Target/RISCV/RISCVInstrInfoZb.td
907 ↗(On Diff #400496)

Need to also handle fsl with a constant argument.

Chenbing.Zheng edited the summary of this revision. (Show Details)

del cmov,cmix,fsri in clang

Chenbing.Zheng marked 2 inline comments as done.Jan 18 2022, 1:34 AM
Chenbing.Zheng added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoZb.td
907 ↗(On Diff #400496)

Emm.... but there is no fsli in zbt.

craig.topper added inline comments.Jan 18 2022, 6:55 AM
llvm/lib/Target/RISCV/RISCVInstrInfoZb.td
907 ↗(On Diff #400496)

Yes because the compiler can change the constant to turn fsli into fsri. We do this on line 903.

If the user uses fsl with a constant we should give them good code by turning it into fsri.

craig.topper added inline comments.Jan 18 2022, 8:50 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4262

The operand order for RISCVISD::FSL/FSR match llvm.fshl and llvm.fshr rather than rs1, rs2, rs3 order. I think the operand order you want for riscv.fsl and riscv.fsr should be rs1, rs2, rs3.

And the assembly printing for fsl/fsr prints $rd, $rs1, $rs3, $rs2 makes this even more confusing.

I'll put up a patch to change RISCVISD::FSL/FSR and RISCVISD::FSLW/FSRW order to be in rs1, rs2, rs3 order.

craig.topper added inline comments.Jan 18 2022, 9:45 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4262

i just noticed the proposed intrinsic order here https://github.com/riscv/riscv-bitmanip/blob/main-history/cproofs/rvintrin.h uses rs1, rs3, rs2/shamt ordering. Which is different than the order used in the spec pseudo code. But rs1, rs3, rs2/shamt matches how the instructions are printed.

So I guess we should use rs1, rs3, rs2/shamt order. I'll update the RISCVISD opcodes accordingly.

Out of curiosity, what is your interest in Zbt? Do you work for a company that is implementing this extension in hardware?

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4262

I pushed a patch to change the operand order of RISCVISD::FSL/FSR

llvm/lib/Target/RISCV/RISCVInstrInfoZb.td
900 ↗(On Diff #400768)

I made this change in the repo and handled the riscv_fsl with constant case.

Out of curiosity, what is your interest in Zbt? Do you work for a company that is implementing this extension in hardware?

My company has not implementing this extension,but may be in the future. I'm just doing a pre-research.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4262

I think change swap op2 and op3 in Pat, so shall i keep the order op1, op2, op3 here?

def : Pat<(riscv_fsl GPR:$rs1, GPR:$rs3, GPR:$rs2),

(FSL GPR:$rs1, GPR:$rs2, GPR:$rs3)>;

def : Pat<(riscv_fsr GPR:$rs1, GPR:$rs3, GPR:$rs2),

(FSR GPR:$rs1, GPR:$rs2, GPR:$rs3)>;
craig.topper added inline comments.Jan 19 2022, 8:31 AM
llvm/test/CodeGen/RISCV/rv32zbt-intrinsic.ll
22

This should have the same register order as the fsl test case. I think the test just needs to be re-generated.

28

Please add an fsli test case using llvm.riscv.fsl.i32 and a constant shift amount

31

This should be fsri a0, a0, a1, 5

llvm/test/CodeGen/RISCV/rv64zbt-intrinsic.ll
28

Please add an fsli test case.

59

Please add a fsri test case.

craig.topper added a comment.EditedJan 19 2022, 10:28 AM

Out of curiosity, what is your interest in Zbt? Do you work for a company that is implementing this extension in hardware?

My company has not implementing this extension,but may be in the future. I'm just doing a pre-research.

The Zbt extensions and the other un-ratified extension from the old Bitmanip spec have not seen any active discussion for at least the last 6-9 months. If that continues, it starts to raise questions about whether llvm should continue to support and extension that does not appear to be on a path to ratification. I've asked for a discussion on this topic at the next RISCV LLVM bi-weekly sync meeting tomorrow morning US time. calendar link here https://llvm.org/docs/GettingInvolved.html#online-sync-ups

Chenbing.Zheng marked 8 inline comments as done.Jan 19 2022, 6:18 PM

Out of curiosity, what is your interest in Zbt? Do you work for a company that is implementing this extension in hardware?

My company has not implementing this extension,but may be in the future. I'm just doing a pre-research.

The Zbt extensions and the other un-ratified extension from the old Bitmanip spec have not seen any active discussion for at least the last 6-9 months. If that continues, it starts to raise questions about whether llvm should continue to support and extension that does not appear to be on a path to ratification. I've asked for a discussion on this topic at the next RISCV LLVM bi-weekly sync meeting tomorrow morning US time. calendar link here https://llvm.org/docs/GettingInvolved.html#online-sync-ups

Thanks a lot, I will keep watching it.

This revision is now accepted and ready to land.Jan 19 2022, 11:14 PM
This revision was automatically updated to reflect the committed changes.