[PATCH] D76077: [ARM] Add __bf16 as new Bfloat16 C Type

[Ties Stuij via Phabricator via cfe-commits]

stuij marked 4 inline comments as done.
stuij added a comment.

@LukeGeeson: you're already mentioned :) See the commits tab on this review. But Simon Tatham needs a mention as well. I shall add him.



================
Comment at: clang/docs/LanguageExtensions.rst:518
+Clang supports three half-precision (16-bit) floating point types: ``__fp16``,
+``_Float16`` and ``__bf16``.  These types are supported in all language modes.
 
----------------
SjoerdMeijer wrote:
> Not my field of expertise, and sorry if I've missed this somewhere, but was wondering if this (i.e. all language modes) means we need C++ name mangling support for bfloat? In the AArch64 backend I saw this:
> 
>   getBFloat16Mangling() const override { return "u6__bf16"; }
> 
> But that's something else I guess, and I was guessing a 2 letter mangling needs to be added here?
> 
> https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling-builtin
Yes, we need name-mangling support, and yes that method you mentioned does that. There's one for AArch32 as well. And yes we have documented it in the 'C++ ABI for the latest Arm Architecture' docs:

aarch32: https://developer.arm.com/docs/ihi0041/latest
aarch64: https://developer.arm.com/docs/ihi0059/latest


================
Comment at: clang/include/clang-c/Index.h:3254
   CXType_FirstBuiltin = CXType_Void,
   CXType_LastBuiltin = CXType_ULongAccum,
 
----------------
majnemer wrote:
> Should this be:
>   CXType_LastBuiltin = CXType_BFloat16,
> 
thanks


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3186
+    case BuiltinType::Half:      EltName = "float16_t"; break;
+    case BuiltinType::BFloat16:  EltName = "bfloat16x1_t"; break;
     default:
----------------
majnemer wrote:
> Why is this x1?
Yes, that does look odd. The original author of this code has left the company, but I'll ask around.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:1873-1874
     // We of course allow this conversion if long double is really double.
+    if (FromType == S.Context.BFloat16Ty || ToType == S.Context.BFloat16Ty)
+      return false;
     if (&S.Context.getFloatTypeSemantics(FromType) !=
----------------
majnemer wrote:
> This probably needs an explanation.
done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76077