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

[Jun Sha via Phabricator via llvm-commits]

joshua-arch1 added a comment.

In D153234#4493739 <https://reviews.llvm.org/D153234#4493739>, @asb wrote:

> @joshua-arch1 are you still planning to work through the review feedback from Craig? Happy to help if useful.

@craig.topper Ping. 
@asb I have replied to Craig, but there is no feedback again.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1846
     IsLegalVT = Subtarget.hasStdExtZfhOrZfhminOrZhinxOrZhinxmin();
+  else if (VT == MVT::bf16)
+    IsLegalVT = Subtarget.hasStdExtZfbfmin();
----------------
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.


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


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

https://reviews.llvm.org/D153234