[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:
> Pierre-vh wrote:
> > Pierre-vh wrote:
> > > 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
> > Ah I remember now, it's just C++ mangling. I don't quite understand the lowercase "u" but a quick search in Clang tells me it's vendor-extended types.
> > So it's just u6 -> vendor extended type, 6 characters following + __bf16 (name of the type).
> Do we really need an override for this? I'd expect a reasonable default. Plus I think a virtual function for something that's only a parameterless, static string is a bit ridiculous
Default impl asserts if not implemented. I think it's to make sure targets are all aware of what it takes to support bfloat and they don't end up partially implementing it?
```
  /// Return the mangled code of bfloat.
  virtual const char *getBFloat16Mangling() const {
    llvm_unreachable("bfloat not implemented on this target");
  }
```

 I'd say let's stick to the current pattern in this diff; I created D139608 to change it


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCallingConv.td:49
+  CCIfType<[bf16], CCBitConvertToType<i16>>,
+  CCIfType<[v2bf16], CCBitConvertToType<i32>>,
   CCIfNotInReg<CCIfType<[f32, i32, f16, i16, v2i16, v2f16] , CCAssignToReg<[
----------------
arsenm wrote:
> Without being added to a register class, all the tablegen changes should not do anything
bf16 ones seem to not be needed but if I don't have the v2bf16 ones I get "cannot allocate arguments" in "test_arg_store_v2bf16"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139398