[PATCH] D153234: [RISCV] Add codegen for Zfbfmin instructions

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1846
     IsLegalVT = Subtarget.hasStdExtZfhOrZfhminOrZhinxOrZhinxmin();
+  else if (VT == MVT::bf16)
+    IsLegalVT = Subtarget.hasStdExtZfbfmin();
----------------
joshua-arch1 wrote:
> craig.topper wrote:
> > Is this change tested?
> define bfloat @bfloat_imm() nounwind {
> ; CHECKIZFBFMIN-LABEL: bfloat_imm:
> ; CHECKIZFBFMIN:       # %bb.0:
> ; CHECKIZFBFMIN-NEXT:    lui a0, %hi(.LCPI6_0)
> ; CHECKIZFBFMIN-NEXT:    flh fa0, %lo(.LCPI6_0)(a0)
> ; CHECKIZFBFMIN-NEXT:    ret
>   ret bfloat 3.0
> }
> 
> In this case, we need this change to address bf16 constant.
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?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:245
 
+def BFPR16 : RegisterClass<"RISCV", [bf16], 16, (add
+    (sequence "F%u_H", 15, 10), // fa5-fa0
----------------
joshua-arch1 wrote:
> craig.topper wrote:
> > 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.
> 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.
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153234/new/

https://reviews.llvm.org/D153234