[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:727
+  if (Subtarget->hasBF16())
+    addRegisterClass(MVT::bf16, &ARM::HPRRegClass);
+
----------------
labrinea wrote:
> dmgreen wrote:
> > 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.
> Maybe for now it'd be better to add the bfloat type if both fullfp16 and bf16 are present. Makes sense?
We will need to fix bfloat + nofullfp16 anyway, and so I'm not sure it will make a lot of difference in the short term to make this fullfp16.

My point is that there will be a lot of operations - like adding two bfloats, or extracting from a bfloat vector or concating two bfloat vectors. In the long run we are going to make sure all those things work and don't crash during legalization. At the moment I expect a lot of them to break. For this patch it is probably OK to ignore that for the moment. But it should be something we look at eventually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81373