The implementation in https://reviews.llvm.org/D151313 is done for the circumstance without Zfbfmin. This patch adds codegen support for the 6 instructions provided in Zfbfmin extension.
Details
Diff Detail
Event Timeline
Rebase, as well as change the operand type in riscv_fpround_bf16/riscv_fpextend_bf16 from f16 to bf16.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
1846 | Is this change tested? | |
llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td | ||
436 | These patterns duplicate patterns from HasStdExtZfhOrZfhmin, can we share them by using a new predicate? | |
llvm/lib/Target/RISCV/RISCVRegisterInfo.td | ||
245 | Can we just add bf16 to the type list for FPR16? If the only reason to have a separate register class is for tablegen type inference, I don't thinks that a good reason. |
@joshua-arch1 are you still planning to work through the review feedback from Craig? Happy to help if useful.
@craig.topper Ping.
@asb Sorry for the delay! I have replied to Craig, but there is no feedback again.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
1846 | define bfloat @bfloat_imm() nounwind { ret bfloat 3.0 } In this case, we need this change to address bf16 constant. | |
llvm/lib/Target/RISCV/RISCVRegisterInfo.td | ||
245 | I have tried to add bf16 to the type list for FPR16, but that needs to modify almost all the patterns with f16 for type inference, which will be heavy work. Also, bf16 is used only in some special cases. I think addressing this type in an individual part will be better. Mixing bf16 and f16 together will make fp16 instructions hard to maintain. I'm wondering why you don't think this is a good reason. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
1846 | I thought this function is supposed to return true for constants that don't need a constant pool. Your test for 3.0 is using a constant pool, which I think means this function is returning false for 3.0. Wouldn't that be the behavior you got without this change? | |
llvm/lib/Target/RISCV/RISCVRegisterInfo.td | ||
245 | This ends up creating two register classes that are circularly subclasses of each other. I think RISCVInstrInfo::loadRegFromStackSlot for BFPR16 only works because RISCV::FPR16RegClass.hasSubClassEq(RISCV::BFPR16) returns true? I'm not sure RISCVInstrInfo::copyPhysReg works at all unless Zfh or Zfhmin is also enabled? Is that tested? |
llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td | ||
---|---|---|
420 | The patterns need to be duplicated for bf16. I think it accidentally works now because the type type profile says they only take f16 types so tablegen optimized out the checks. We need to relax the types in the type constraints and add new patterns. def SDT_RISCVFMV_H_X : SDTypeProfile<1, 1, [SDTCisVT<0, f16>, SDTCisVT<1, XLenVT>]>; def SDT_RISCVFMV_X_EXTH : SDTypeProfile<1, 1, [SDTCisVT<0, XLenVT>, SDTCisVT<1, f16>]>; |
llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td | ||
---|---|---|
403 | Can we put these patterns in the RISCVInstrInfoZfbfmin.td? If the include order doesn't work, I hope we can just fix the include order so that Zfh is included first. |
llvm/lib/Target/RISCV/RISCVInstrInfoZfbfmin.td | ||
---|---|---|
34 | There's only supposed to be one SDNode declaration per RISCVISD opcode name. The SDTypeProfile for the existing version needs to be changed to SDTCisFP instead of SDTCisVT<1, f16>. The semantics of the opcode changed, so the type description needs to change accordingly. |
Relax the types in the type constraints for SDT_RISCVFMV_H_X and SDT_RISCV_FMV_X_EXTH
Is this change tested?