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

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

codemzs marked an inline comment as done.
codemzs added a comment.

In D149573#4337480 , @stuij wrote:

> I made a comment on the RFC 
> 
>  to understand if we really need/want a new bfloat16 type.

Thank you, @stuij, for your input. I will continue to follow the consensus on 
the RFC regarding the introduction of a new bfloat16 type. For context, my 
initial implementation transitioned the existing storage-only `__bf16` type 
into an arithmetic type. If the decision leans towards not introducing a new 
`bfloat16` type, I'm prepared to revert my changes to utilize the `__bf16` 
type. To ensure our collective efforts are effectively streamlined and avoid 
any potential duplication, I am committed to aligning my work with the 
community consensus and ongoing discussions.

@tahonermann, when you have a moment, your guidance would be greatly 
appreciated.


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

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

[Ties Stuij via Phabricator via cfe-commits]

stuij added a comment.

I made a comment on the RFC 

 to understand if we really need/want a new bfloat16 type.


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

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

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

codemzs marked an inline comment as done.
codemzs added a comment.

In D149573#4332549 , @tahonermann 
wrote:

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

@tahonermann, thank you for your review and highlighting the naming issues with 
`__bf16` and `std::bfloat16_t`. I agree that reversing the type names will 
improve readability and maintainability. I considered this while working on the 
code and appreciate your suggestion to address it in a separate patch before 
rebasing this one.




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.

tahonermann wrote:
> Is this for `__bf16` or for `std::bfloat16_t`?
Its for `std::bfloat16_t`, I don't believe `__bf16` has a literal suffix. 


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

[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 cfe-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>;

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

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

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

codemzs added a comment.

Hi @rjmccall and @erichkeane,

I would like to inquire about the necessity of implementing excess precision 
support for `std::bfloat16_t` in this patch. In reference to the discussion on 
https://reviews.llvm.org/D136919, it was mentioned that introducing 
`std::bfloat16_t` in Clang requires proper mangling, semantics, and excess 
precision support.

However, this change includes a front-end flag that enables `bfloat16` 
arithmetic only when the target natively supports `bfloat16` operations. Given 
this restriction, is it still essential to provide excess precision support for 
`std::bfloat16_t` in this patch?

Thank you for your guidance.


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

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

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

codemzs marked an inline comment as done.
codemzs added a comment.

Hi @rjmccall and @erichkeane,

I would like to inquire about the necessity of implementing excess precision 
support for `std::bfloat16_t` in this patch. In reference to the discussion on 
https://reviews.llvm.org/D136919, it was mentioned that introducing 
`std::bfloat16_t` in Clang requires proper mangling, semantics, and excess 
precision support.

However, this change includes a front-end flag that enables `bfloat16` 
arithmetic only when the target natively supports `bfloat16` operations. Given 
this restriction, is it still essential to provide excess precision support for 
`std::bfloat16_t` in this patch?

Thank you for your guidance.


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

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

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

codemzs marked 2 inline comments as done.
codemzs added inline comments.



Comment at: clang/include/clang/Basic/LangOptions.def:468
 
+LANGOPT(ExperimentalNewItaniumMangling, 1, 0, "experimental new Itanium 
mangling")
+

erichkeane wrote:
> 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.
@erichkeane @tahonermann Itanium mangling is now finalized as `DF16b`and [[ 
https://github.com/itanium-cxx-abi/cxx-abi/pull/147#issuecomment-1540692344 | 
merged  ]]into the github repository. 


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

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

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

codemzs added a comment.

Thank you @erichkeane for your insightful review. I have addressed the feedback 
from your previous review.


Repository:
  rG LLVM Github Monorepo

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

[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 cfe-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.00e+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.00e+00
```

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


CHANGES SINCE LAST ACTION
  

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

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

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()->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, 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 

[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 cfe-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;

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()->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, 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.00e+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 

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

[Nikolas Klauser via Phabricator via cfe-commits]

philnik removed a reviewer: libc++abi.
philnik added a comment.

In D149573#433 , @codemzs wrote:

> In D149573#4310601 , @philnik wrote:
>
>> In D149573#4310009 , @codemzs 
>> wrote:
>>
>>> My change to libcxxabi/test/test_demangle.pass.cpp (last file in the 
>>> change) is only one line i.e 
>>> {"_ZNK5clang4Type16isArithmeticTypeERNS_10ASTContextE", 
>>> "clang::Type::isArithmeticType(clang::ASTContext&) const"}, but running 
>>> clang-format on it changes bunch of lines.
>>
>> Please don't clang-format the test then. There is no need to do it, and the 
>> changes look rather confusing. I don't really understand why you add it at 
>> all TBH. This doesn't look like it tests anything from this patch.
>
> @philnik Thank you for your feedback on the changes to 
> `libcxxabi/test/test_demangle.pass.cpp`. I apologize for any confusion my 
> initial approach may have caused. I intended to keep the test file consistent 
> with the updated function prototype, but I understand your concern about 
> clang-format's impact on readability.
>
> I will revert the changes to the test file as you suggested, ensuring it does 
> not affect the current patch's functionality. I appreciate your guidance and 
> will consider these aspects in future updates.

No worries :D. The clang-format guidelines are a bit confusing, and there is 
nothing wrong with not knowing all the conventions of the different subprojects.


Repository:
  rG LLVM Github Monorepo

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

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

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

codemzs added a comment.

In D149573#4310601 , @philnik wrote:

> In D149573#4310009 , @codemzs wrote:
>
>> My change to libcxxabi/test/test_demangle.pass.cpp (last file in the change) 
>> is only one line i.e {"_ZNK5clang4Type16isArithmeticTypeERNS_10ASTContextE", 
>> "clang::Type::isArithmeticType(clang::ASTContext&) const"}, but running 
>> clang-format on it changes bunch of lines.
>
> Please don't clang-format the test then. There is no need to do it, and the 
> changes look rather confusing. I don't really understand why you add it at 
> all TBH. This doesn't look like it tests anything from this patch.

@philnik Thank you for your feedback on the changes to 
`libcxxabi/test/test_demangle.pass.cpp`. I apologize for any confusion my 
initial approach may have caused. I intended to keep the test file consistent 
with the updated function prototype, but I understand your concern about 
clang-format's impact on readability.

I will revert the changes to the test file as you suggested, ensuring it does 
not affect the current patch's functionality. I appreciate your guidance and 
will consider these aspects in future updates.


Repository:
  rG LLVM Github Monorepo

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

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

[Nikolas Klauser via Phabricator via cfe-commits]

philnik added a comment.

In D149573#4310009 , @codemzs wrote:

> My change to libcxxabi/test/test_demangle.pass.cpp (last file in the change) 
> is only one line i.e {"_ZNK5clang4Type16isArithmeticTypeERNS_10ASTContextE", 
> "clang::Type::isArithmeticType(clang::ASTContext&) const"}, but running 
> clang-format on it changes bunch of lines.

Please don't clang-format the test then. There is no need to do it, and the 
changes look rather confusing. I don't really understand why you add it at all 
TBH. This doesn't look like it tests anything from this patch.


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

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

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

codemzs added a comment.

My change to libcxxabi/test/test_demangle.pass.cpp (last file in the change) is 
one is only one line i.e 
{"_ZNK5clang4Type16isArithmeticTypeERNS_10ASTContextE", 
"clang::Type::isArithmeticType(clang::ASTContext&) const"}, but running 
clang-format on it changes bunch of lines.


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

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

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

codemzs updated this revision to Diff 518468.
codemzs added a comment.
Herald added subscribers: mstorsjo, mgrang, fedor.sergeev.

Fix Clang format failures.


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

https://reviews.llvm.org/D149573

Files:
  clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
  clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/BuiltinTypes.def
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/LiteralSupport.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/AST/NSAPI.cpp
  clang/lib/AST/PrintfFormatString.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/Type.cpp
  clang/lib/AST/TypeLoc.cpp
  clang/lib/Basic/Targets/AArch64.h
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/ARM.h
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Index/USRGeneration.cpp
  clang/lib/Lex/LiteralSupport.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaObjCProperty.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Serialization/ASTCommon.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/CodeGenCXX/cxx2b-fp-ext-std-names-p1467r9.cpp
  clang/test/Sema/cxx2b-fp-ext-std-names-p1467r9.cpp
  libcxxabi/test/test_demangle.pass.cpp

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

codemzs added inline comments.



Comment at: libcxxabi/test/test_demangle.pass.cpp:10921
 {"_ZNK5clang4Type10isRealTypeEv", "clang::Type::isRealType() const"},
-{"_ZNK5clang4Type16isArithmeticTypeEv", "clang::Type::isArithmeticType() 
const"},
+{"_ZNK5clang4Type16isArithmeticTypeEv", 
"clang::Type::isArithmeticType(clang::ASTContext&) const"},
 {"_ZNK5clang4Type12isScalarTypeEv", "clang::Type::isScalarType() const"},

Forgot to update the mangled name as these tests don't seem to run locally with 
cmake check-all 


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

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

[Thorsten via Phabricator via cfe-commits]

tschuett added a comment.

Agreed.


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

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

[M. Zeeshan Siddiqui via Phabricator via cfe-commits]

codemzs added a comment.

In D149573#4309378 , @tschuett wrote:

> I don't believe that there is NativeBFloat16Type. AArch64 learned bf16 with 
> ARMv8.6-A, but very limited operations and only in SIMD. X86 supports bf16 
> since Cooperlake, but very limited operations and only in SIMD.
>
> Maybe GPUs?



In D149573#4309378 , @tschuett wrote:

> I don't believe that there is NativeBFloat16Type. AArch64 learned bf16 with 
> ARMv8.6-A, but very limited operations and only in SIMD. X86 supports bf16 
> since Cooperlake, but very limited operations and only in SIMD.
>
> Maybe GPUs?

Dear @tschuett,

Thank you for your input. It seems that NativeBFloat16Type is indeed limited in 
availability. As you mentioned, AArch64 and X86 offer some bf16 support, but 
only in SIMD and with limited operations. This flag is intended to guard 
changes for targets with full bf16 support, as discussed in this RFC: 
https://discourse.llvm.org/t/rfc-c-23-p1467r9-extended-floating-point-types-and-standard-names/70033/12.
 While few public targets currently have native bf16 support, this still serves 
as a useful precaution.


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

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

[Thorsten via Phabricator via cfe-commits]

tschuett added a comment.

I don't believe that there is NativeBFloat16Type. AArch64 learned bf16 with 
ARMv8.6-A, but very limited operations and only in SIMD. X86 supports bf16 
since Cooperlake, but very limited operations and only in SIMD.

Maybe GPUs?


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