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

Others definitely need to review this, but i'm about happy with it.



================
Comment at: clang/include/clang/Basic/LangOptions.def:468
 
+LANGOPT(ExperimentalNewItaniumMangling, 1, 0, "experimental new Itanium mangling")
+
----------------
codemzs wrote:
> 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.
Thanks, I think this is a good direction for this patch to take.


================
Comment at: clang/lib/AST/ASTContext.cpp:135
+    CXX23FloatingPointConversionRankMap = {
+        {{{FloatingRankCompareResult::FRCR_Equal,
+           FloatingRankCompareResult::FRCR_Unordered,
----------------
Just a suggestion here, feel free to toss it, but I suspect some comments to explain the 'grid' a bit are helpful.

Basically, split it up into the 5 groups, and comment which is which.


================
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
----------------
codemzs wrote:
> 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?
Urgh, I kinda hate that script, it always gives such difficult to read tests...

My understanding is that when compiled with 'strip symbol names', no names are included that aren't required for ABI purposes.  So labels all switch to %0, param names are removed entirely/etc.

Though, I guess I haven't seen that bot complain in a while, so perhaps it disappeared...


================
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}}
----------------
codemzs wrote:
> 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.
I looked it up too, and I don't see this being a problem in our AST output as it is with our LLVM output, so perhaps I'm incorrect here.  Feel free to leave them alone as is...

As far as the `|-` characters: I'd suggest just removing them entirely.  Just left-align so it'd be :

```
VarDecl {{.*}} f16_val_6 '_Float16' cinit
FloatingLiteral {{.*}} '_Float16' 1.000000e+00
```

It'll still pass, and not be sensitive to the AST-dump's way of outputting.


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

https://reviews.llvm.org/D149573