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

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.


================
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
----------------
stuij wrote:
> 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.
I think we first need agreement what -mfloat-abi=soft  with bf16 means and how it should behave, document this, and have some tests. Possibly document how we diverge from this.

I think I tend to disagree with this:

> I think we shouldn't support bfloat at all in combination with -mfloat-abi=soft.

why would it not be supported in some way (promotions to another type, or even library calls), like the other float-types? 

> It turns out cc1 will accept -mfloat-abi=soft with neon intrinsics, which will happily churn out neon instructions.

I would say the fact that there are other problems, shouldn't distract us too much from trying to get this right; I think at this point that is not yet a justification. 

> 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.

I think at this point I disagree with this, mainly because of my first point: the behaviour should be specified. I would also say that not doing this, could be a bit of bad precedent for adding a new C type. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76077