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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits