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

[Ties Stuij via Phabricator via cfe-commits]

stuij added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8111
   "pointer cannot be cast to type %0">;
+def err_cast_to_bfloat : Error<"cannot type-cast to __bf16">;
+def err_cast_from_bfloat : Error<"cannot type-cast from __bf16">;
----------------
SjoerdMeijer wrote:
> Nit: was wondering if `err_cast_to_bfloat16` would be more consistent
agree


================
Comment at: clang/lib/AST/ASTContext.cpp:2052
+      Width = Target->getBFloat16Width();
+      Align = Target->getBFloat16Align();
     case BuiltinType::Float16:
----------------
SjoerdMeijer wrote:
> Is a `break` missing here?
tnx


================
Comment at: clang/lib/AST/ASTContext.cpp:6007
     case Float16Rank:
     case HalfRank: llvm_unreachable("Complex half is not supported");
     case FloatRank:      return FloatComplexTy;
----------------
SjoerdMeijer wrote:
> nit: perhaps this error message is not entirely accurate for bfloat16?
tnx


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3186
+    case BuiltinType::Half:      EltName = "float16_t"; break;
+    case BuiltinType::BFloat16:  EltName = "bfloat16x1_t"; break;
     default:
----------------
stuij wrote:
> 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.
I'm not sure it's 100% wrong, but it's not consistent with GCC. Changing to `bfloat16_t`.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:74
+  BFloat16Width = BFloat16Align = 16;
+  BFloat16Format = &llvm::APFloat::BFloat();
+
----------------
SjoerdMeijer wrote:
> Nit: we use bfloat16 everywhere, would `llvm::APFloat::BFloat16()` be better for consistency?
In the IR world bfloat is consistently called bfloat, without the 16. I think this might turn into bikeshedding to justify if either one or both sides of the divide should or should not include the '16'. I can think of arguments to support the various options.

As I think there's no clear winner, I'd like to take the route of less effort and keep things as is.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:115
   HalfTy = llvm::Type::getHalfTy(LLVMContext);
+  BFloatTy = llvm::Type::getBFloatTy(LLVMContext);
   FloatTy = llvm::Type::getFloatTy(LLVMContext);
----------------
SjoerdMeijer wrote:
> nit: perhaps `getBFloat16Ty()`
see previous C - IR divide comment


================
Comment at: clang/lib/CodeGen/CodeGenTypeCache.h:39
+  /// half, bfloat, float, double
+  llvm::Type *HalfTy, *BFloatTy, *FloatTy, *DoubleTy;
 
----------------
SjoerdMeijer wrote:
> And so here too, BFloat16Ty?
see previous C - IR divide comment


================
Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:307
+    else
+      return llvm::Type::getInt16Ty(VMContext);
+  }
----------------
SjoerdMeijer wrote:
> Is this covered in tests?
Thanks. That code doesn't make sense to me. We are not expecting to emulate bfloat operations, and we shouldn't conflate bfloat with half. I'll remove the UseNativeHalf clause, and the one call-site of this fn.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5844
   ABIKind Kind;
+  bool IsSoftFloatABI;
 
----------------
SjoerdMeijer wrote:
> Nit: to distinguish the unfortunate float ABI names, I was wondering whether `IsFloatABISoftFP` is clearer, or something along those lines, just to make clear it is not the "soft" but the "softfp" variant
Yes, good idea. Done.


================
Comment at: clang/test/CodeGen/arm-mangle-16bit-float.cpp:2
+// RUN: %clang_cc1 -triple aarch64-arm-none-eabi -fallow-half-arguments-and-returns -target-feature +bf16 -target-feature +fullfp16 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK64
+// RUN: %clang_cc1 -triple arm-arm-none-eabi     -fallow-half-arguments-and-returns -target-feature +bf16 -target-feature +fullfp16 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK32
+
----------------
SjoerdMeijer wrote:
> Do you need to pass `+fullfp16`?
I'm splitting out bf16 and fp16 tests. I don't think we should be testing with the union of cmdline options to accommodate testing multiple types in one file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76077