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

[Erich Keane via Phabricator via libcxx-commits]

erichkeane added a subscriber: rjmccall.
erichkeane added a comment.

I



================
Comment at: clang/include/clang/AST/ASTContext.h:28
 #include "clang/AST/TemplateName.h"
+#include "clang/AST/Type.h"
 #include "clang/Basic/IdentifierTable.h"
----------------
What is this needed for?  


================
Comment at: clang/include/clang/AST/ASTContext.h:2808
+  /// are unordered, return FRCR_Unordered. If \p LHS and \p RHS are equal but
+  /// subrank of \p LHS is greater than \p RHS, return
+  /// FRCR_Equal_Greater_Subrank. If \p LHS and \p RHS are equal but subrank of
----------------



================
Comment at: clang/include/clang/AST/ASTContext.h:2809
+  /// subrank of \p LHS is greater than \p RHS, return
+  /// FRCR_Equal_Greater_Subrank. If \p LHS and \p RHS are equal but subrank of
+  /// \p LHS is less than \p RHS, return FRCR_Equal_Lesser_Subrank. Subrank and
----------------



================
Comment at: clang/include/clang/AST/ASTContext.h:2811
+  /// \p LHS is less than \p RHS, return FRCR_Equal_Lesser_Subrank. Subrank and
+  /// Unordered comparision was introduced in C++23.
+  FloatingRankCompareResult getFloatingTypeOrder(QualType LHS,
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8745
 def err_cast_from_bfloat16 : Error<"cannot type-cast from __bf16">;
+def err_cxx2b_invalid_implicit_floating_point_cast : Error<"floating point cast results in loss of precision.">;
 def err_typecheck_expect_scalar_operand : Error<
----------------



================
Comment at: clang/include/clang/Basic/LangOptions.def:468
 
+LANGOPT(ExperimentalNewItaniumMangling, 1, 0, "experimental new Itanium mangling")
+
----------------
I'm not sure I like the implications or maintainability of this.  This is something that is likely to get out of hand too quickly.  We probably need to just figure out what the itanium mangling is GOING to be (perhaps @rjmccall has some  insight?), and just implement that.

Also, this isn't really a 'Language Option', so much as a codegen option.


================
Comment at: clang/lib/AST/ASTContext.cpp:116
+// C++23 6.8.6p2 [conv.rank]
+using RankMap =
+    std::unordered_map<clang::BuiltinType::Kind, FloatingRankCompareResult>;
----------------
I don't quite see what this is used for yet, but nested unordered-maps for what is all compile-time constants seems like a waste of compile-time.  I wonder if there is a good way to use a table for this comparison (perhaps with some level of adjustment on the type-kinds to make them non-sparse).


================
Comment at: clang/lib/AST/ASTContext.cpp:7152
+      RHSKind = RHS->castAs<BuiltinType>()->getKind();
+    auto comp = CXX23FloatingPointConversionRankMap[LHSKind][RHSKind];
+    return comp;
----------------
by coding standard, you can't use 'auto' here.  Also, variables are capital. I probably just would return it without the intermediary variable.

That said, this is exactly what I feared the unordered-map table above was for.  We need to come up with a better storage medium for that.

My thought is to introduce something like:

```
constexpr unsigned FloatRankToIndex(clang::BuiltinType::Kind) {
  switch(Kind) {
    case clang::BuiltinType::Float16: return 0;  
    case clang::BuiltinType::BF16: return 1;
    case clang::BuiltinType::Float: return 2;
    case clang::BuiltinType::Double: return 3;
    case clang::BuiltinType::LongDouble: return 4;
    default: llvm_unreachable("Not a floating builtin type");
 }
```
And just run that on `LHSKind` and `RHSKind` here.  Then, the `CXX23FloatingPointConversionRankMap` can be just a 2d pair of arrays:



```
std::array<std::array<FloatingRankCompareResult, 5>, 5> CXX23FloatingPointConversionRankMap = 
```

We get the same results with very small runtime cost (for the 2 switches per operation), but save two `unordered_map` lookups, AND save the runtime construction of the `unordered_map`s.


================
Comment at: clang/test/CodeGenCXX/cxx2b-fp-ext-std-names-p1467r9.cpp:5
+// CHECK-LABEL: @_Z1fDF16b(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[V_ADDR:%.*]] = alloca bfloat, align 2
----------------
`entry` isn't a stable name here, so you shouldn't use this label.


================
Comment at: clang/test/CodeGenCXX/cxx2b-fp-ext-std-names-p1467r9.cpp:101
+
+// CHECK-LABEL: @_Z4testv(
+// CHECK-NEXT:  entry:
----------------
I'd vastly prefer these check-next commands be interlaced in teh code, so it is easy to see which line is supposed to associate with which.


================
Comment at: clang/test/Sema/cxx2b-fp-ext-std-names-p1467r9.cpp:9
+//CHECK:      |-VarDecl {{.*}} f16_val_6 '_Float16' cinit
+//CHECK-NEXT: | `-FloatingLiteral {{.*}} '_Float16' 1.000000e+00
+_Float16 f16_val_7 = static_cast<_Float16>(1.0bf16); // expected-error {{static_cast from 'BF16' to '_Float16' is not allowed}}
----------------
How we emit the result of the floats is inconsistent, at least in IR, so I'm not sure how stable it'll be here.

Also, don't use the `|-` and `| \`-`` in the check-lines, those get messy when something changes.  


================
Comment at: clang/test/Sema/cxx2b-fp-ext-std-names-p1467r9.cpp:466
+float float_val_2(user_defined_val_3); // expected-error {{conversion from 'S2' to 'float' is ambiguous}}
\ No newline at end of file

----------------
Definitely need the newline here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149573