[libcxx-commits] [PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names and introduce Bfloat16 arithmetic type.

[Tom Honermann via Phabricator via libcxx-commits]

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