stuij marked 9 inline comments as done.
stuij added inline comments.

================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:74
+  BFloat16Width = BFloat16Align = 16;
+  BFloat16Format = &llvm::APFloat::BFloat();
+
----------------
SjoerdMeijer wrote:
> 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...
In general naming isn't bikeshedding. And I appreciate discussions on naming. 
I'm just afraid it might turn into bikeshedding. I do agree that such a remark 
isn't a very constructive to a conversation. Sorry.


================
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
----------------
SjoerdMeijer wrote:
> what happens with `-mfloat-abi=soft`. Does that deserve a test?
Yes, this one is interesting. I think we shouldn't support bfloat at all in 
combination with -mfloat-abi=soft. We don't support software emulation of 
bfloat instructions and all operations on bfloat are simd instructions.

It turns out cc1 will accept -mfloat-abi=soft with neon intrinsics, which will 
happily churn out neon instructions. This doesn't sound very soft. The driver 
will ignore -mfloat-abi=soft in certain combinations of cmdline instructions, 
but I haven't delved deep enough to know what's what.

GCC doesn't allow soft+neon combination. Unfortunately it will actually crash 
for just a bfloat type by itself, which is quite useless without intrinsics. 
The Arm GCC folks will raise a ticket on this with as proposed solution to not 
allow this combination.

As this issue seems bigger than just bfloat, and potentially there's driver 
code involved as well I thought it'd make sense to handle this in a separate 
patch.


================
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)
----------------
SjoerdMeijer wrote:
> 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.
As I wrote above, I think we currently should not allow it as long as there are 
no targets that support bfloat and have use for this combination.


================
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
+
----------------
SjoerdMeijer wrote:
> SjoerdMeijer wrote:
> > nit: if the name mangling is Independent of the float ABI, then you might 
> > as well one runline and the abi option.
> -> ...  then you might as well have only one runline and remove the abi 
> option.
yes, I thought it's better to be defensive here, in case the implementation 
changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76077



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to