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