[PATCH] D139398: [AMDGPU] Add bf16 storage support

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: clang/lib/Basic/Targets/AMDGPU.h:119
+  bool hasBFloat16Type() const override { return isAMDGCN(getTriple()); }
+  const char *getBFloat16Mangling() const override { return "u6__bf16"; };
+
----------------
arsenm wrote:
> Don't understand this mangling. What is u6?
Not sure; for that one I just copy-pasted the implementation of other targets. All other targets use that mangling scheme


================
Comment at: clang/test/SemaCUDA/amdgpu-bf16.cu:43
+  *out = bf16;
+}
+
----------------
arsenm wrote:
> check casts to different int and float types? Is construction of bf16 vectors allowed?
Added cast + vec sema test and vec assign codegen test too

No conversions are allowed apparently but I don't think it matters for the initial patch; if needed we can always add it later I think


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:4819-4831
+    // When we don't have 16 bit instructions, bf16 is illegal and gets
+    // softened to i16 for storage, with float being used for arithmetic.
+    //
+    // After softening, some i16 -> fp32 bf16_to_fp operations can be left over.
+    // Lower those to (f32 (fp_extend (f16 (bitconvert x))))
+    if (!Op->getValueType(0).isFloatingPoint() ||
+        Op->getOperand(0).getValueType() != MVT::i16)
----------------
arsenm wrote:
> The generic legalizer should have handled this?
It looks like those operations are not implemented in the generic legalizer, e.g. I get 
``` 
Do not know how to promote this operator's operand!
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139398