[PATCH] D81373: [ARM] Basic bfloat support

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5908
+    if ((DstVT == MVT::f16 && !Subtarget->hasFullFP16()) ||
+        (DstVT == MVT::bf16 && !Subtarget->hasBF16()))
       return SDValue();
----------------
labrinea wrote:
> dmgreen wrote:
> > According to D81411, you can have bf16 without having fp16. And so you don't have any of the instructions like VMOV.f16 (which a VMOVrh will turn into).
> > 
> > Same goes for the vldr.16 int he test below. Because +fp16 isn't specified, we might have to awkwardly use some other set of instructions. It will be more efficient to use vmov.16 and vldr.16 if they are available, but if they are not we might have to fall back to something else.
> > 
> > Or we say that combination isn't supported, but it seems that fp16 is still optional and bf16 is mandatory in 8.6.
> Good point. I am going to alter these checks to only guard fullfp16 for now. As the title suggests this is basic support, so I think it's fair to only support the bf16+fullfp16 combination in this revision. I will make sure it is explicitly stated in the commit message.
Sounds fair. I would expect this to be the most common combination, so is good to tackle first.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:727
+  if (Subtarget->hasBF16())
+    addRegisterClass(MVT::bf16, &ARM::HPRRegClass);
+
----------------
We only add the FP16 regclass if we have HasFullFP16. Not if we just have the vcvt instructions (HasFP16). I agree it is probably good to make bf16 a legal type if we can, but a lot of operations will not be supported. As far as I understand they are not expected to work, and there will be nothing that can promote them at the moment?

I'm not sure how the AArch64 backend handled it, but do we need something like setAllExpand(..)? Honestly I would expect it to still break if you tried to add two bfloats in IR, but it might be more future-proof.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5081
 
 bool ARMTargetLowering::isUnsupportedFloatingType(EVT VT) const {
   if (VT == MVT::f32)
----------------
This controls some things like setcc lowering. I wouldn't expect them to be relevant for bfloat if it is essentially just a storage type.


================
Comment at: llvm/lib/Target/ARM/ARMInstrVFP.td:166
 
+def : Pat<(f16 (alignedload16 addrmode5fp16:$addr)),
+          (VLDRH addrmode5fp16:$addr)>;
----------------
I think these patterns should still have `let Predicates = [HasFPRegs16] in` around them, like we do for all the NEON or MVE patterns.


================
Comment at: llvm/test/CodeGen/ARM/bfloat.ll:9
+define bfloat @load_scalar_bf(bfloat* %addr) {
+; CHECK-LABEL: load_scalar_bf:
+; CHECK:       @ %bb.0: @ %entry
----------------
These CHECK lines are left over.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81373