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

Tue May 2 23:45:13 PDT 2023

codemzs marked 10 inline comments as done.
codemzs added a comment.

Addressing @erichkeane 's review comments and pushing out the updated patch.

================
Comment at: clang/include/clang/Basic/LangOptions.def:468

+LANGOPT(ExperimentalNewItaniumMangling, 1, 0, "experimental new Itanium mangling")
+
----------------
erichkeane wrote:
> 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.
Thank you for your valuable feedback. Based on your suggestion and considering that GCC has already implemented the mangling as `DF16b`, I agree that if we have reasonable confidence in the direction the mangling will take, it would be better to implement it directly instead of guarding it with an experimental flag. Therefore, I will be removing the flag. I initially added the flag since @tahonermann was on board with implementing the bf16 arithmetic type, assuming the mangling was finalized.

However, I understand your concerns and would appreciate further input from both @tahonermann and @rjmccall on this matter. My intention is to avoid stalling the progress of this change due to mangling finalization.

I'm open to further discussion and collaboration to ensure we make the right decision for our project while maintaining the momentum of the review process.

================
Comment at: clang/lib/AST/ASTContext.cpp:7152
+      RHSKind = RHS->castAs<BuiltinType>()->getKind();
+    auto comp = CXX23FloatingPointConversionRankMap[LHSKind][RHSKind];
+    return comp;
----------------
erichkeane wrote:
> 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.
Thank you for your insightful suggestion! I agree that using a 2D array along with the `FloatRankToIndex` function is a more efficient and cleaner approach. I've updated the implementation accordingly. I appreciate your guidance on this.

================
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
----------------
erichkeane wrote:
> `entry` isn't a stable name here, so you shouldn't use this label.
I have removed the "entry" label as you suggested.

Just to let you know, this label was automatically generated by the script `utils/update_cc_test_checks.py`, which is used to update the expected output of the test cases. I noticed that this label is present in other tests in the codebase as well.

Out of curiosity, why is this label not considered stable?

================
Comment at: clang/test/CodeGenCXX/cxx2b-fp-ext-std-names-p1467r9.cpp:101
+
+// CHECK-LABEL: @_Z4testv(
+// CHECK-NEXT:  entry:
----------------
erichkeane wrote:
> 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.
I understand your preference for interlacing `CHECK-NEXT` commands with the code to improve readability. Unfortunately, the update_cc_test_checks.py script doesn't seem have an option to interlace `CHECK-NEXT` statements, and it can be challenging to achieve this in certain scenarios, such as when we have multiple loads followed by stores.

To address your concern, I've refactored the test code into smaller functions, which should make it easier to read and understand the relationship between the code and the corresponding `CHECK` statements. I hope this improves the readability and addresses your concerns. If you have any further suggestions or improvements, please don't hesitate to let me know. I'm open to making any necessary changes to ensure the code is clear and maintainable.

================
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}}
----------------
erichkeane wrote:
> 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.  
I've observed similar floating-point representations in other tests, but I acknowledge your concerns about potential inconsistencies and stability. To address this, would replacing them with `{{.*}}` be a suitable solution? I'm also considering dividing the test into two parts: one for error checks in the current location and another for AST checks under `test/AST`. Would this resolve your concerns about the use of `|-` and `| ` characters? Furthermore, I'd like to clarify whether your comment about avoiding `|-` applies specifically to tests in the `test/Sema` directory or more generally. My intention is to seek clarification and ensure the test code adheres to best practices.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149573