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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

As I wrote in one of the inlined comments, I am relatively unhappy that we have both bfloat and bfloat16 as names. But that looks like a complain for another patch, and not really this one.



================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:74
+  BFloat16Width = BFloat16Align = 16;
+  BFloat16Format = &llvm::APFloat::BFloat();
+
----------------
stuij wrote:
> 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.
ah, I couldn't remember on which patch I commented on the bfloat16 vs bfloat naming.
Since everything is called bfloat16, from the architecture extension to the source language type, I find this a bit of a missed opportunity to get the names consistent. And I wouldn't say that naming of types is bikeshedding. But maybe that's just me...


================
Comment at: clang/lib/Basic/Targets/ARM.cpp:80
     DoubleAlign = LongLongAlign = LongDoubleAlign = SuitableAlign = 32;
+  BFloat16Width = BFloat16Align = 16;
+  BFloat16Format = &llvm::APFloat::BFloat();
----------------
Is this tested? Can it be tested?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4483
+                                     bool V1Ty=false,
+                                     bool HasBFloat16Type=true) {
   int IsQuad = TypeFlags.isQuad();
----------------
nit: the linter says some spaces here.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5514
 
-  llvm::VectorType *VTy = GetNeonType(this, Type, HasLegalHalfType);
+  llvm::VectorType *VTy = GetNeonType(this, Type, HasLegalHalfType, false, HasBFloat16Type);
   llvm::Type *Ty = VTy;
----------------
nit: looks like the hinter right to be unhappy about the formatting


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:6270
 
+  // __bf16  get passed using the bfloat ir type, or using i32 but
+  // with the top 16 bits unspecified.
----------------
nit: ir -> IR


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:6471
+    // FP16/BF16 vectors should be converted to integer vectors
+    // This check is similar  to isIllegalVectorType - refactor?
+    if ((!getTarget().hasLegalHalfType() &&
----------------
nit: perhaps add TODO, or just remove this.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:1874
+
+    // conversions between bfloat and other floats are not permitted
+    if (FromType == S.Context.BFloat16Ty || ToType == S.Context.BFloat16Ty)
----------------
Nit: conversions -> Conversions,  permitted -> permitted.

Same on line no. 1895.


================
Comment at: clang/test/CodeGen/arm-bf16-params-returns.c:5
+// RUN: %clang_cc1 -triple aarch64-arm-none-eabi -target-abi aapcs -mfloat-abi softfp -target-feature +bf16 -target-feature +neon -emit-llvm -O2 -o - %s | opt -S -mem2reg -sroa | FileCheck %s --check-prefix=CHECK64-SOFTFP
+
+// function return types
----------------
what happens with `-mfloat-abi=soft`. Does that deserve a test?


================
Comment at: clang/test/CodeGen/arm-mangle-16bit-float.cpp:4
+
+// CHECK64: define {{.*}}void @_Z3foou6__bf16(half %b)
+// CHECK32: define {{.*}}void @_Z3foou6__bf16(i32 %b.coerce)
----------------
LukeGeeson wrote:
> SjoerdMeijer wrote:
> > LukeGeeson wrote:
> > > craig.topper wrote:
> > > > How can bfloat16 be passed as half? Don't they have a different format?
> > > see the above comment about soft-abis
> > Probably I am bit confused too now... are the three possible types that we are expecing not bfloat, i32, or i16? 
> Hi Sjoerd, Good spot here I forgot to add a default case somewhere, which means AArch32 didn't observe `-mfloat-abi softfp` by default - and hence used bfloat where i32 was expected. 
> 
> I have a local patch for this and will send to Ties to merge into this one :) 
> 
> 
> We're not sure what you mean by i16 however?
Ah, as I said, got confused. Now I see we use i16 for targets that don't support bfloat16. So we probably need a test for that.


================
Comment at: clang/test/CodeGen/arm-mangle-bf16.cpp:3
+// RUN: %clang_cc1 -triple arm-arm-none-eabi -target-feature +bf16 -mfloat-abi hard -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK32-HARD
+// RUN: %clang_cc1 -triple arm-arm-none-eabi -target-feature +bf16 -mfloat-abi softfp -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK32-SOFTFP
+
----------------
nit: if the name mangling is Independent of the float ABI, then you might as well one runline and the abi option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76077