tahonermann requested changes to this revision.
tahonermann added a comment.
This revision now requires changes to proceed.
I reviewed about a third of this, but then stopped due to the `__bf16` vs
`std::bfloat16_t` naming issues. I think the existing names that use "bfloat16"
to support the `__bf16` type should be renamed, in a separate patch, and this
patch rebased on top of it. We are sure to make mistakes if this confusing
situation is not resolved.
================
Comment at: clang/include/clang/AST/ASTContext.h:1113-1114
CanQualType HalfTy; // [OpenCL 6.1.1.1], ARM NEON
CanQualType BFloat16Ty;
+ CanQualType BF16Ty; // [C++23 6.8.3p5], ISO/IEC/IEEE 60559.
CanQualType Float16Ty; // C11 extension ISO/IEC TS 18661-3
----------------
This seems wrong. C++23 introduces `std::bfloat16_t` and Clang already
supports `__bf16`. It seems to me that the `BFloat16Ty` name should be used for
the former and `BF16Ty` used for the latter. I get that the inconsistency is
preexisting, but I think we should fix that (in another patch before landing
this one; there aren't very many references).
================
Comment at: clang/include/clang/AST/BuiltinTypes.def:215-219
+// 'Bfloat16' arithmetic type to represent std::bfloat16_t.
+FLOATING_TYPE(BF16, BFloat16Ty)
+
// '__bf16'
FLOATING_TYPE(BFloat16, BFloat16Ty)
----------------
Here again, the type names are the exact opposite of what one would intuitively
expect (I do appreciate that the comment makes the intent clear, but it still
looks like a copy/paste error or similar).
================
Comment at: clang/include/clang/AST/Type.h:2152-2162
+ bool isCXX23FloatingPointType()
+ const; // C++23 6.8.2p12 [basic.fundamental] (standard floating point +
+ // extended floating point)
+ bool isCXX23StandardFloatingPointType()
+ const; // C++23 6.8.2p12 [basic.fundamental] (standard floating point)
+ bool isCXX23ExtendedFloatingPointType()
+ const; // C++23 6.8.2p12 [basic.fundamental] (extended floating point)
----------------
Precedent elsewhere in this file is to place multi-line comments ahead of the
function they appertain to.
================
Comment at: clang/include/clang/AST/Type.h:7259-7265
inline bool Type::isBFloat16Type() const {
return isSpecificBuiltinType(BuiltinType::BFloat16);
}
+inline bool Type::isBF16Type() const {
+ return isSpecificBuiltinType(BuiltinType::BF16);
+}
----------------
This is a great example of `BFloat16` vs `BF16` confusion; here the names are
*not* reversed. It is really hard to know if this code is wrong or for callers
to know which one they should use.
================
Comment at: clang/include/clang/Driver/Options.td:6418-6420
+def fnative_bfloat16_type: Flag<["-"], "fnative-bfloat16-type">,
+ HelpText<"Enable bfloat16 arithmetic type in C++23 for targets with native
support.">,
+ MarshallingInfoFlag<LangOpts<"NativeBFloat16Type">>;
----------------
Since this message is specific to C++ for now (pending the addition of a
`_BFloat16` type in some future C standard, I think the message should
reference `std::bfloat16_t` and skip explicit mention of C++23. I know it is
kind of awkward to refer to the standard library name for the type, but since
WG21 decided not to provide a keyword name (I wish they would just use the C
names for these and get over it; they can still provide pretty library names!),
there isn't another more explicit name to use. Alternatively, we could say
something like "Enable the underlying type of std::bfloat16_t in C++ ...".
================
Comment at: clang/include/clang/Lex/LiteralSupport.h:75
bool isBitInt : 1; // 1wb, 1uwb (C2x)
+ bool isBF16 : 1; // 1.0bf
uint8_t MicrosoftInteger; // Microsoft suffix extension i8, i16, i32, or i64.
----------------
Is this for `__bf16` or for `std::bfloat16_t`?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149573/new/
https://reviews.llvm.org/D149573
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
