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

[Joshua Cranmer via Phabricator via cfe-commits]

jcranmer-intel added a comment.

So while trying to review this patch, I've discovered there's an annoying 
incompatibility between C and C++ here, in that C and C++ specify different 
rules on how to choose between `_Float64`, `double`, and `long double` if all 
are `binary64` (C says `_Float64` > `long double` > `double`, C++ says `long 
double` > `_Float64` > `double`), and I don't have enough knowledge of clang 
internal implementation to know if the changes being done can capture the 
different semantics there. (It's also not entirely clear to me that the 
incompatibility between C and C++ was intentional in the first place; it looks 
like the paper author intentionally chose the ordering for C++ and argued for 
the change to C, but the CFP working group soundly rejected the change, and 
personally I agree with the CFP decision here over C++).




Comment at: clang/lib/AST/ASTContext.cpp:136-138
+// another floating point type.
+constexpr std::array, 5>
+CXX23FloatingPointConversionRankMap = {

I don't like having a large table here of results. It just doesn't scale; if 
you take into account all of the putative types that might be supported, you've 
got 3 basic types + 4 interchange formats + 3 decimal types + 1 IEEE extended 
type + bfloat + 3 IBM hex floats, and that's just things already supported by 
LLVM or that I know someone's working on.

I think after special casing float/double/long double, you can handle all the 
other types by simply using a mixture of `fltSemantics::isRepresentableBy` and 
a subrank preference list.

In the event that support for `_Float32` and `_Float64` are added, this table 
will no longer be target-independent. We have a few targets where `double` is 
binary32 and several targets where `long double` is either binary32 or binary64 
(and others where it's neither). I think it's better to start writing this 
method in a way that can handle this mapping be target-dependent.


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

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

Just starting to look at this. Don't we need a RN for this?


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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

Apologies once again for the delayed response. I reviewed some today and will 
resume reviewing on Monday.

In addition to the inline suggestions:

`clang/docs/ReleaseNotes.rst` will need to be updated to reflect that the core 
language changes for P1467R9 have been implemented for `std::float16_t` and 
`std::bfloat16_t`.

Given the confusing array of 16-bit floating-point types, how about modifying 
`clang/include/clang/AST/BuiltinTypes.def` to be more explicit regarding which 
is which?

  // 'half' in OpenCL, '__fp16' in ARM NEON.
  FLOATING_TYPE(Half, HalfTy)
  ...
  // '_Float16', 'std::float16_t'
  FLOATING_TYPE(Float16, HalfTy)
  
  // '__bf16', 'std::bfloat16_t'
  FLOATING_TYPE(BFloat16, BFloat16Ty)

Hmm, having pasted the above, I now noticed that the `Half` and `Float16` types 
share the `HalfTy` singleton. Unless I'm mistaken, the changes being made will 
cause divergent behavior. Do these now need to be split?




Comment at: clang/include/clang/AST/ASTContext.h:56
+// Conversion ranks introduced in C++23 6.8.6p2 [conv.rank]
+enum FloatingRankCompareResult {
+  FRCR_Unordered,

Naming suggestion: `FloatConvRankCompareResult`.



Comment at: clang/include/clang/AST/ASTContext.h:1119
+  CanQualType BFloat16Ty; // [C++23 6.8.3p5][basic.extended.fp]
+  CanQualType Float16Ty; // C11 extension ISO/IEC TS 18661-3 and [C++23 
6.8.3p5][basic.extended.fp]
   CanQualType VoidPtrTy, NullPtrTy;

I think the comment should reference http://eel.is/c++draft/basic.extended.fp#1 
for `std::float16_t`. p5 is for `std::bfloat16_t`.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8931
 >;
+def err_cxx23_invalid_implicit_floating_point_cast : Error<"floating point 
cast results in loss of precision">;
 def err_typecheck_expect_scalar_operand : Error<

The diagnostic text here looks more like the text of a warning. Since this is 
an error, I think it makes more sense to model the text on other similar errors 
and use "narrowing" or "implicit cast" terminology. For examples, see 
`err_spaceship_argument_narrowing` and `err_impcast_complex_scalar`

 Additionally, it would be helpful to include the relevant types in the message.

Finally, line length should be kept to 80 characters or less per 
https://llvm.org/docs/CodingStandards.html#source-code-width.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h:779-788
+FloatingRankCompareResult order = Ctx.getFloatingTypeOrder(LTy, RTy);
+if ((order == FRCR_Greater) || (order == FRCR_Equal_Greater_Subrank)) {
   RHS = (*doCast)(Solver, RHS, LTy, LBitWidth, RTy, RBitWidth);
   RTy = LTy;
-} else if (order == 0) {
+} else if ((order == FRCR_Equal) || (order == FRCR_Equal_Lesser_Subrank)) {
   LHS = (*doCast)(Solver, LHS, RTy, RBitWidth, LTy, LBitWidth);
   LTy = RTy;

This looks like a pre-existing bug since, for standard floating-point types, 
narrowing implicit conversions are allowed. I think the intent was to add a 
cast on which ever side is needed, but the existing code instead adds a cast on 
the RHS when the LHS has a higher rank, adds a cast on the LHS when the LHS and 
RHS have the same rank, and wanders into UB when the RHS has a higher rank.

The incorrect comparison goes back to when the code was introduced in commit 
08f943c5630d8ee31d6a93227171d2f05db59e62.

The introduction of unordered conversion ranks suggests additional changes are 
needed here, but it isn't clear to me what the right changes are. I added a 
suggested edit that adds an arbitrary cast (which probably suffices for the 
purposes of static analysis). Alternatively, an assert could be added for the 
unordered and equal cases.



Comment at: clang/lib/Sema/SemaChecking.cpp:10451
   return ICE->getCastKind() == CK_FloatingCast &&
- S.Context.getFloatingTypeOrder(From, To) < 0;
+ S.Context.getFloatingTypeOrder(From, To) == FRCR_Lesser;
 }

codemzs wrote:
> tahonermann wrote:
> > I'm not sure this is right. If I understand correctly, the C++23 extended 
> > FP types don't participate in argument promotions. Perhaps they should be 
> > excluded here.
> Rules for 
> [promotion](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1467r9.html#promotion)
>  are unchanged as per the proposal. This is just using the new enumeration to 
> represent a smaller conversion rank. 
I think this still produces an incorrect result in some cases though. According 
to http://eel.is/c++draft/conv.fpprom, the only floating-point promotion is 
from `float` to `double`.

Ah, I think the prior code was incorrect, but non-symptomatic because it is 
only currently used in one place (in `CheckPrintfHandler::checkFormatExpr()`) 
and that code only cares about the integer cases. I would prefer that we fix 
this rather than extend the issue 

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

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

codemzs updated this revision to Diff 558099.
codemzs added a comment.

Hi @tahonermann,

I've just pushed a new diff with tests for `va_arg` and `...`, ensuring 
promotion rules are intact.

Also, I've made sure `getFloatingTypeOrder` returns `FRCR_Unordered` only when 
we're dealing with both operands as C++23 extended floating point types and 
they are unordered. I've added tests like `_Float16 f16_val_1 = 1.0bf16; // 
expected-error {{cannot initialize a variable of type '_Float16' with an rvalue 
of type '__bf16'}}` to cover these cases. If you see any potential issues here, 
I'm all ears and ready to make adjustments.

Thanks a lot for reviewing this, especially with your busy schedule.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149573

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Lex/LiteralSupport.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Frontend/InitPreprocessor.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/SemaOverload.cpp
  clang/test/CodeGenCXX/cxx23-fp-ext-std-names-p1467r9.cpp
  clang/test/CodeGenCXX/cxx23-vector-bfloat16.cpp
  clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp

Index: clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
===
--- /dev/null
+++ clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
@@ -0,0 +1,505 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++23 -target-feature +fullbf16 -verify -ast-dump %s | FileCheck %s
+#include 
+_Float16 f16_val_1 = 1.0bf16; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type '__bf16'}}
+_Float16 f16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'float'}}
+_Float16 f16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'double'}}
+_Float16 f16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'long double'}}
+_Float16 f16_val_6 = 1.0f16;
+//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}}
+_Float16 f16_val_8 = static_cast<_Float16>(1.0f);
+//CHECK:  VarDecl {{.*}} f16_val_8 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'float' 1.00e+00
+_Float16 f16_val_9 = static_cast<_Float16>(1.0);
+//CHECK:  VarDecl {{.*}} f16_val_9 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'double' 1.00e+00
+_Float16 f16_val_10 = static_cast<_Float16>(1.0l);
+//CHECK:  VarDecl {{.*}} f16_val_10 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'long double' 1.00e+00
+_Float16 f16_val_11 = static_cast<_Float16>(1.0f16);
+//CHECK:  VarDecl {{.*}} f16_val_11 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} '_Float16' 1.00e+00
+
+decltype(0.0BF16) bf16_val_1 = 1.0f16; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type '_Float16'}}
+decltype(0.0BF16) bf16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'float'}}
+decltype(0.0BF16) bf16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'double'}}
+decltype(0.0BF16) bf16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'long double'}}
+decltype(0.0BF16) bf16_val_5 = 1.0bf16;
+//CHECK:  VarDecl {{.*}} bf16_val_5 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: FloatingLiteral {{.*}} '__bf16' 1.00e+00
+
+decltype(0.0BF16) bf16_val_6 = static_cast(1.0f16); // expected-error {{static_cast from '_Float16' to 'decltype(0.BF16)' (aka '__bf16') is not allowed}}
+decltype(0.0BF16) bf16_val_7 = static_cast(1.0f);
+//CHECK:  VarDecl {{.*}} bf16_val_7 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} 'decltype(0.BF16)':'__bf16' static_cast 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'float' 1.00e+00
+decltype(0.0BF16) bf16_val_8 = static_cast(1.0);

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

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

codemzs updated this revision to Diff 558083.
codemzs set the repository for this revision to rG LLVM Github Monorepo.
codemzs added a comment.

Synced to the main and resolved merge conflicts, updated tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149573

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Lex/LiteralSupport.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Frontend/InitPreprocessor.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/SemaOverload.cpp
  clang/test/CodeGenCXX/cxx23-fp-ext-std-names-p1467r9.cpp
  clang/test/CodeGenCXX/cxx23-vector-bfloat16.cpp
  clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp

Index: clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
===
--- /dev/null
+++ clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
@@ -0,0 +1,465 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++23 -target-feature +fullbf16 -verify -ast-dump %s | FileCheck %s
+
+_Float16 f16_val_1 = 1.0bf16; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type '__bf16'}}
+_Float16 f16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'float'}}
+_Float16 f16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'double'}}
+_Float16 f16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'long double'}}
+_Float16 f16_val_6 = 1.0f16;
+//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}}
+_Float16 f16_val_8 = static_cast<_Float16>(1.0f);
+//CHECK:  VarDecl {{.*}} f16_val_8 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'float' 1.00e+00
+_Float16 f16_val_9 = static_cast<_Float16>(1.0);
+//CHECK:  VarDecl {{.*}} f16_val_9 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'double' 1.00e+00
+_Float16 f16_val_10 = static_cast<_Float16>(1.0l);
+//CHECK:  VarDecl {{.*}} f16_val_10 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'long double' 1.00e+00
+_Float16 f16_val_11 = static_cast<_Float16>(1.0f16);
+//CHECK:  VarDecl {{.*}} f16_val_11 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} '_Float16' 1.00e+00
+
+decltype(0.0BF16) bf16_val_1 = 1.0f16; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type '_Float16'}}
+decltype(0.0BF16) bf16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'float'}}
+decltype(0.0BF16) bf16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'double'}}
+decltype(0.0BF16) bf16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'long double'}}
+decltype(0.0BF16) bf16_val_5 = 1.0bf16;
+//CHECK:  VarDecl {{.*}} bf16_val_5 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: FloatingLiteral {{.*}} '__bf16' 1.00e+00
+
+decltype(0.0BF16) bf16_val_6 = static_cast(1.0f16); // expected-error {{static_cast from '_Float16' to 'decltype(0.BF16)' (aka '__bf16') is not allowed}}
+decltype(0.0BF16) bf16_val_7 = static_cast(1.0f);
+//CHECK:  VarDecl {{.*}} bf16_val_7 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} 'decltype(0.BF16)':'__bf16' static_cast 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'float' 1.00e+00
+decltype(0.0BF16) bf16_val_8 = static_cast(1.0);
+//CHECK:  VarDecl {{.*}} bf16_val_8 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} 'decltype(0.BF16)':'__bf16' static_cast 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'double' 1.00e+00
+decltype(0.0BF16) bf16_val_9 = static_cast(1.0l);
+//CHECK:  VarDecl {{.*}} bf16_val_9 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} 'decltype(0.BF16)':'__bf16' static_cast 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'long double' 1.00e+00
+decltype(0.0BF16) 

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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

@codemzs, I'm sorry for the very long delay following up on this review. I've 
been struggling to keep up, but expect to be able to devote some time to this 
next week. I'm committed to helping to ensure we land this before Phabricator 
stops accepting new diffs (proposed for November 15th).


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

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

codemzs added a comment.

@tahonermann I would like to understand your concern better on unordered 
floating point types as the callers of `getFloatingTypeOrder` handle this 
result as per the C++23 proposal, for example there is a test case that 
exercises this scenario: _Float16 f16_val_1 = 1.0bf16; // expected-error 
{{cannot initialize a variable of type '_Float16' with an rvalue of type 
'__bf16'}}

Perhaps if you can please give me an example that further illustrates your 
concerns it would help me understand and assuage your concerns. Thank you.




Comment at: clang/lib/AST/ASTContext.cpp:191
+   FloatingRankCompareResult::FRCR_Greater,
+   FloatingRankCompareResult::FRCR_Equal;
+

tahonermann wrote:
> I just realized that none of these comparisons results in 
> `FRCR_Equal_Lesser_Subrank` or `FRCR_Equal_Greater_Subrank`. I guess those 
> won't actually be used until support for `std::float32_t` and 
> `std::float64_t` are added. That means that we don't have a way to test code 
> that performs subrank checks though.
That is correct, my plan was to add support for these types after this change 
is committed. 



Comment at: clang/lib/Frontend/InitPreprocessor.cpp:461
+  Builder.defineMacro("__STDCPP_FLOAT16_T__", "1");
+  Builder.defineMacro("__STDCPP_BFLOAT16_T__", "1");
+}

tahonermann wrote:
> Should the definition of `__STDCPP_BFLOAT16_T__` be conditional on something 
> like `Ctx.getTargetInfo().hasFullBFloat16Type()`?
Based on [our 
discussion](https://discourse.llvm.org/t/rfc-c-23-p1467r9-extended-floating-point-types-and-standard-names/70033/34?u=codemzs)
 on the RFC it was determined that representing `bfloat16` using `fp32` is ok 
as per the author of the proposal, in that case do you believe we should be 
making this macro conditional on the target natively supporting `bfloat16`?



Comment at: clang/lib/Sema/SemaChecking.cpp:10451
   return ICE->getCastKind() == CK_FloatingCast &&
- S.Context.getFloatingTypeOrder(From, To) < 0;
+ S.Context.getFloatingTypeOrder(From, To) == FRCR_Lesser;
 }

tahonermann wrote:
> I'm not sure this is right. If I understand correctly, the C++23 extended FP 
> types don't participate in argument promotions. Perhaps they should be 
> excluded here.
Rules for 
[promotion](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1467r9.html#promotion)
 are unchanged as per the proposal. This is just using the new enumeration to 
represent a smaller conversion rank. 



Comment at: clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp:3-6
+_Float16 f16_val_1 = 1.0bf16; // expected-error {{cannot initialize a variable 
of type '_Float16' with an rvalue of type '__bf16'}}
+_Float16 f16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of 
type '_Float16' with an rvalue of type 'float'}}
+_Float16 f16_val_3 = 1.0; // expected-error {{cannot initialize a variable of 
type '_Float16' with an rvalue of type 'double'}}
+_Float16 f16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of 
type '_Float16' with an rvalue of type 'long double'}}

tahonermann wrote:
> Are these errors correct? Since the source is a constant expression that 
> specifies a value that is within the representable range of the type, I think 
> these initializations are expected to be allowed.
I believe this should be an error unless my understanding is incorrect, below 
is what the proposal 
[says](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1467r9.html#implicit-constant)
 for constant expressions:

"A drawback of this part of the proposal is that constant values don’t get any 
special treatment. As a result, this code:

`std::float16_t x = 1.0;`

would be ill-formed. The constant 1.0 has type double, which cannot be 
implicitly converted to type std::float16_t, even though the value 1.0 can be 
represented exactly in both types. To compile, the code must have an explicit 
cast, or, preferably, use a literal suffix:
`std::float16_t x = 1.0f16;`
"



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

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

codemzs updated this revision to Diff 546100.
codemzs marked an inline comment as done.
codemzs added a comment.

Updated with feedback from @tahonermann


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

https://reviews.llvm.org/D149573

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Lex/LiteralSupport.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Frontend/InitPreprocessor.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/SemaOverload.cpp
  clang/test/CodeGenCXX/cxx23-fp-ext-std-names-p1467r9.cpp
  clang/test/CodeGenCXX/cxx23-vector-bfloat16.cpp
  clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp

Index: clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
===
--- /dev/null
+++ clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
@@ -0,0 +1,465 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++23 -target-feature +fullbf16 -verify -ast-dump %s | FileCheck %s
+
+_Float16 f16_val_1 = 1.0bf16; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type '__bf16'}}
+_Float16 f16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'float'}}
+_Float16 f16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'double'}}
+_Float16 f16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'long double'}}
+_Float16 f16_val_6 = 1.0f16;
+//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}}
+_Float16 f16_val_8 = static_cast<_Float16>(1.0f);
+//CHECK:  VarDecl {{.*}} f16_val_8 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'float' 1.00e+00
+_Float16 f16_val_9 = static_cast<_Float16>(1.0);
+//CHECK:  VarDecl {{.*}} f16_val_9 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'double' 1.00e+00
+_Float16 f16_val_10 = static_cast<_Float16>(1.0l);
+//CHECK:  VarDecl {{.*}} f16_val_10 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'long double' 1.00e+00
+_Float16 f16_val_11 = static_cast<_Float16>(1.0f16);
+//CHECK:  VarDecl {{.*}} f16_val_11 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} '_Float16' 1.00e+00
+
+decltype(0.0BF16) bf16_val_1 = 1.0f16; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type '_Float16'}}
+decltype(0.0BF16) bf16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'float'}}
+decltype(0.0BF16) bf16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'double'}}
+decltype(0.0BF16) bf16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'long double'}}
+decltype(0.0BF16) bf16_val_5 = 1.0bf16;
+//CHECK:  VarDecl {{.*}} bf16_val_5 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: FloatingLiteral {{.*}} '__bf16' 1.00e+00
+
+decltype(0.0BF16) bf16_val_6 = static_cast(1.0f16); // expected-error {{static_cast from '_Float16' to 'decltype(0.BF16)' (aka '__bf16') is not allowed}}
+decltype(0.0BF16) bf16_val_7 = static_cast(1.0f);
+//CHECK:  VarDecl {{.*}} bf16_val_7 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} 'decltype(0.BF16)':'__bf16' static_cast 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'float' 1.00e+00
+decltype(0.0BF16) bf16_val_8 = static_cast(1.0);
+//CHECK:  VarDecl {{.*}} bf16_val_8 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} 'decltype(0.BF16)':'__bf16' static_cast 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'double' 1.00e+00
+decltype(0.0BF16) bf16_val_9 = static_cast(1.0l);
+//CHECK:  VarDecl {{.*}} bf16_val_9 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} 'decltype(0.BF16)':'__bf16' static_cast 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'long double' 1.00e+00
+decltype(0.0BF16) bf16_val_10 = static_cast(1.0bf16);
+//CHECK:  VarDecl {{.*}} bf16_val_10 

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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann requested changes to this revision.
tahonermann added a comment.
This revision now requires changes to proceed.

Sorry for the long delay reviewing again. I noted some mostly minor issues. I'm 
on vacation next week, but I'll try to watch for updates.

I'm still concerned that the changes to the code that was already performing 
rank comparisons don't explicitly do anything for unordered cases. I think I 
would feel better if we at least asserted that the comparisons are not 
unordered.

I'd like to see tests added to exercise `va_arg` and passing the extended types 
through `...`; this will exercise the promotion related changes.




Comment at: clang/lib/AST/ASTContext.cpp:191
+   FloatingRankCompareResult::FRCR_Greater,
+   FloatingRankCompareResult::FRCR_Equal;
+

I just realized that none of these comparisons results in 
`FRCR_Equal_Lesser_Subrank` or `FRCR_Equal_Greater_Subrank`. I guess those 
won't actually be used until support for `std::float32_t` and `std::float64_t` 
are added. That means that we don't have a way to test code that performs 
subrank checks though.



Comment at: clang/lib/AST/ASTContext.cpp:7141-7145
+/// If LHS > RHS, return FRCR_Greater.  If LHS == RHS, return FRCR_Equal. If
+/// LHS < RHS, return FRCR_Lesser. If the values representedable by the two
+/// are not subset of each other, return FRCR_Unordered. If LHS == RHS but
+/// LHS has a higher subrank than RHS return FRCR_Equal_Greater_Subrank else
+/// return FRCR_Equal_Lesser_Subrank.





Comment at: clang/lib/Frontend/InitPreprocessor.cpp:461
+  Builder.defineMacro("__STDCPP_FLOAT16_T__", "1");
+  Builder.defineMacro("__STDCPP_BFLOAT16_T__", "1");
+}

Should the definition of `__STDCPP_BFLOAT16_T__` be conditional on something 
like `Ctx.getTargetInfo().hasFullBFloat16Type()`?



Comment at: clang/lib/Sema/SemaCast.cpp:1367
+  Self.Context.doCXX23ExtendedFpTypesRulesApply(DestType, SrcType)) {
+// Support for cast between fp16 and bf16 doesn't exist yet.
+if (!((DestType->isBFloat16Type() || DestType->isFloat16Type()) &&

codemzs wrote:
> tahonermann wrote:
> > Should this be a FIXME comment? What happens in this case? Should we 
> > perhaps assert on such attempted conversions?
> I have added the FIXME. There is a test case for this scenario:
> 
> ```_Float16 f16_val_7 = static_cast<_Float16>(1.0bf16); // expected-error 
> {{static_cast from '__bf16' to '_Float16' is not allowed}}```
> 
> If you still believe it might be better to add an assert I can do that, 
> please let me know.
If the condition can be legitimately hit, then we definitely want error 
handling rather than an assert. Thanks, this looks good.



Comment at: clang/lib/Sema/SemaChecking.cpp:10451
   return ICE->getCastKind() == CK_FloatingCast &&
- S.Context.getFloatingTypeOrder(From, To) < 0;
+ S.Context.getFloatingTypeOrder(From, To) == FRCR_Lesser;
 }

I'm not sure this is right. If I understand correctly, the C++23 extended FP 
types don't participate in argument promotions. Perhaps they should be excluded 
here.



Comment at: clang/lib/Sema/SemaChecking.cpp:14170-14195
   int Order = S.getASTContext().getFloatingTypeSemanticOrder(
   QualType(SourceBT, 0), QualType(TargetBT, 0));
-  if (Order > 0) {
+  if (Order == FRCR_Greater) {
 // Don't warn about float constants that are precisely
 // representable in the target type.
 Expr::EvalResult result;
 if (E->EvaluateAsRValue(result, S.Context)) {

Should `FRCR_Unordered` be handled in some way here? It seems like we should at 
least assert that the types are not unordered.



Comment at: clang/lib/Sema/SemaExpr.cpp:1244-1246
+assert(((order != FRCR_Equal) &&
+(order == FRCR_Lesser || order == FRCR_Equal_Lesser_Subrank)) &&
+   "illegal float comparison");

Since the assertion requires `order` to be one of `FRCR_Lesser` or 
`FRCE_Equal_Lesser_Subrank`, there is no need to check for `FRCR_Equal`.



Comment at: clang/lib/Sema/SemaExpr.cpp:1235-1237
+if (order == FRCR_Unordered) {
+  return QualType();
+}

codemzs wrote:
> tahonermann wrote:
> > Return of an invalid type is a new behavior for this function and it isn't 
> > clear to me that callers will handle it (or be expected to handle it) such 
> > that a diagnostic will be generated. Perhaps a diagnostic should be issued 
> > here? Or perhaps this should be an assertion?
> It results in an error, please see the below test case:
> 
> ```auto f16_bf16 = 1.0f16 + 1.0bf16; // expected-error {{invalid operands to 
> binary expression ('_Float16' and '__bf16')}}```
> 
> This function is only called by `UsualArithmeticConversions` which returns 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D149573#4418001 , @codemzs wrote:

> @tahonermann Gentle ping, please let me know if you have any questions.

FWIW, there are C++ standards meetings all week this week, and C standards 
meetings all next week (and into the following week), so @tahonermann (and 
other reviewers) might be slower to respond as a result.


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

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

codemzs added a comment.

@tahonermann Gentle ping, please let me know if you have any questions.


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

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

codemzs added inline comments.



Comment at: clang/lib/Sema/SemaCast.cpp:1367
+  Self.Context.doCXX23ExtendedFpTypesRulesApply(DestType, SrcType)) {
+// Support for cast between fp16 and bf16 doesn't exist yet.
+if (!((DestType->isBFloat16Type() || DestType->isFloat16Type()) &&

tahonermann wrote:
> Should this be a FIXME comment? What happens in this case? Should we perhaps 
> assert on such attempted conversions?
I have added the FIXME. There is a test case for this scenario:

```_Float16 f16_val_7 = static_cast<_Float16>(1.0bf16); // expected-error 
{{static_cast from '__bf16' to '_Float16' is not allowed}}```

If you still believe it might be better to add an assert I can do that, please 
let me know.



Comment at: clang/lib/Sema/SemaExpr.cpp:1235-1237
+if (order == FRCR_Unordered) {
+  return QualType();
+}

tahonermann wrote:
> Return of an invalid type is a new behavior for this function and it isn't 
> clear to me that callers will handle it (or be expected to handle it) such 
> that a diagnostic will be generated. Perhaps a diagnostic should be issued 
> here? Or perhaps this should be an assertion?
It results in an error, please see the below test case:

```auto f16_bf16 = 1.0f16 + 1.0bf16; // expected-error {{invalid operands to 
binary expression ('_Float16' and '__bf16')}}```

This function is only called by `UsualArithmeticConversions` which returns 
invalid type in several places, hence returning invalid type should be ok? If 
you still prefer an assertion or diagnostic, I will happily add one.



Comment at: clang/lib/Sema/SemaOverload.cpp:4202
+  if (S.Context.getFloatingTypeOrder(SCS2.getToType(1),
+ SCS1.getFromType()) < FRCR_Equal) 
{
+return ImplicitConversionSequence::Better;

tahonermann wrote:
> This comparison includes `FRCR_Unordered`, is that intended? (another case at 
> line 4225 below)
Yes, my understanding is we are checking if the second sequence's type's (T3) 
conversion rank is not equal to the conversion rank of FP2. Is that not correct?


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

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

codemzs updated this revision to Diff 529171.
codemzs marked 4 inline comments as done.
codemzs set the repository for this revision to rG LLVM Github Monorepo.
codemzs added a comment.

Hi @tahonermann,

Thank you for your insights on the patch. I concur with your perspective about 
the potential brittleness of comparison operations on the 
`FloatingRankCompareResult` enum type. As a remedy, I have incorporated a 
helper routine to handle such operations.

Regarding the `getFloatingTypeOrder()`method, it employs a 2-D grid to evaluate 
the conversion rank between two types. The grid index for a type is firmly 
established in the `CXX23FloatRankToIndex`, offering a stable indexing 
mechanism. Hence, it appears that this approach mitigates the brittleness 
concern associated with the comparison of `FloatingRankCompareResult` enum 
values.

I'm open to further insights if there are aspects I may have overlooked.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149573

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Lex/LiteralSupport.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Frontend/InitPreprocessor.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/SemaOverload.cpp
  clang/test/CodeGenCXX/cxx23-fp-ext-std-names-p1467r9.cpp
  clang/test/CodeGenCXX/cxx23-vector-bfloat16.cpp
  clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp

Index: clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
===
--- /dev/null
+++ clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
@@ -0,0 +1,465 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++23 -target-feature +fullbf16 -verify -ast-dump %s | FileCheck %s
+
+_Float16 f16_val_1 = 1.0bf16; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type '__bf16'}}
+_Float16 f16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'float'}}
+_Float16 f16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'double'}}
+_Float16 f16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'long double'}}
+_Float16 f16_val_6 = 1.0f16;
+//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}}
+_Float16 f16_val_8 = static_cast<_Float16>(1.0f);
+//CHECK:  VarDecl {{.*}} f16_val_8 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'float' 1.00e+00
+_Float16 f16_val_9 = static_cast<_Float16>(1.0);
+//CHECK:  VarDecl {{.*}} f16_val_9 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'double' 1.00e+00
+_Float16 f16_val_10 = static_cast<_Float16>(1.0l);
+//CHECK:  VarDecl {{.*}} f16_val_10 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'long double' 1.00e+00
+_Float16 f16_val_11 = static_cast<_Float16>(1.0f16);
+//CHECK:  VarDecl {{.*}} f16_val_11 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} '_Float16' 1.00e+00
+
+decltype(0.0BF16) bf16_val_1 = 1.0f16; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type '_Float16'}}
+decltype(0.0BF16) bf16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'float'}}
+decltype(0.0BF16) bf16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'double'}}
+decltype(0.0BF16) bf16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'long double'}}
+decltype(0.0BF16) bf16_val_5 = 1.0bf16;
+//CHECK:  VarDecl {{.*}} bf16_val_5 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: FloatingLiteral {{.*}} '__bf16' 1.00e+00
+
+decltype(0.0BF16) bf16_val_6 = static_cast(1.0f16); // expected-error {{static_cast from '_Float16' to 'decltype(0.BF16)' (aka '__bf16') is not allowed}}
+decltype(0.0BF16) bf16_val_7 = static_cast(1.0f);
+//CHECK:  VarDecl {{.*}} bf16_val_7 

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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

This is looking good to me. I added a few comments seeking clarification.

I'm concerned about the changes to the existing calls to 
`getFloatingTypeOrder()`. The changes look like they will preserve prior 
behavior for standard types just fine, but I'm not sure whether they implement 
the right behavior for the extended types. Defining comparison functions that 
are explicit in their handling of `FRCR_Unordered` and subrank comparisons 
would help me feel more confident about the changes, the intent, and my ability 
to audit for the desired behavior.




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.

codemzs wrote:
> 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. 
I guess it does now! :)



Comment at: clang/lib/AST/ASTContext.cpp:126-127
+return 4;
+  default:
+llvm_unreachable("Not a CXX23+ floating point builtin type");
+  }

Assuming there is no need to handle `__float128` and `__ibm128` here, how about 
a comment to state why they don't require support here?



Comment at: clang/lib/Sema/Sema.cpp:706
+auto conversionRank = Context.getFloatingTypeOrder(TypeTy, ExprTy);
+if (conversionRank < FRCR_Greater) {
+  Diag(E->getExprLoc(),

This comparison depends on the order of the enumerators in 
`FloatingRankCompareResult` and that seems brittle. I suggest either 
encompassing such comparisons in a function or, at least, adding a comment to 
the enumeration definition that explains the ordering requirements.



Comment at: clang/lib/Sema/SemaCast.cpp:1367
+  Self.Context.doCXX23ExtendedFpTypesRulesApply(DestType, SrcType)) {
+// Support for cast between fp16 and bf16 doesn't exist yet.
+if (!((DestType->isBFloat16Type() || DestType->isFloat16Type()) &&

Should this be a FIXME comment? What happens in this case? Should we perhaps 
assert on such attempted conversions?



Comment at: clang/lib/Sema/SemaExpr.cpp:1235-1237
+if (order == FRCR_Unordered) {
+  return QualType();
+}

Return of an invalid type is a new behavior for this function and it isn't 
clear to me that callers will handle it (or be expected to handle it) such that 
a diagnostic will be generated. Perhaps a diagnostic should be issued here? Or 
perhaps this should be an assertion?



Comment at: clang/lib/Sema/SemaOverload.cpp:4202
+  if (S.Context.getFloatingTypeOrder(SCS2.getToType(1),
+ SCS1.getFromType()) < FRCR_Equal) 
{
+return ImplicitConversionSequence::Better;

This comparison includes `FRCR_Unordered`, is that intended? (another case at 
line 4225 below)


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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> @tahonermann Just a quick note - I've made the changes you initially 
> suggested in the RFC. When you get a chance, could you have another look?

Yes, I hope to today. If not, I'll make it a priority for tomorrow.


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

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

codemzs added a comment.

In D149573#4395968 , @erichkeane 
wrote:

> This all LGTM, but I want @tahonermann to do a pass as well, he knows more 
> about FP than I do, so I don't want to accept without his run-through.

Thank you, @erichkeane for the review. @aaron.ballman if you can please also 
have a look when you get a chance it would be great!

@tahonermann Just a quick note - I've made the changes you initially suggested 
in the RFC 
.
 When you get a chance, could you have another look?


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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

This all LGTM, but I want @tahonermann to do a pass as well, he knows more 
about FP than I do, so I don't want to accept without his run-through.


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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D149573#4391083 , @codemzs wrote:

> In D149573#4391013 , @erichkeane 
> wrote:
>
>> In D149573#4390895 , @codemzs 
>> wrote:
>>
>>> In D149573#4390863 , @stuij wrote:
>>>
 This is going to be a very unhelpful comment. After looking through the 
 changes, I don't have any comments to make, but I also don't feel 
 comfortable to accept this revision as I don't feel to know enough about 
 the front-end.
>>>
>>> @stuij, I sincerely appreciate you taking the time to review the changes. 
>>> Your hesitation due to unfamiliarity with the front-end elements is 
>>> completely understandable, and I respect your candid feedback.
>>>
>>> @erichkeane, given your extensive contributions to the core `Sema`* files, 
>>> I believe your expertise and experience would be particularly valuable in 
>>> reviewing the changes I've made. I recall your initial informal approval 
>>> for the change, and since then, I've further refined it after incorporating 
>>> the outcomes of D150913 . I'd be most 
>>> appreciative if you could please review this revision once again.
>>>
>>> My intention is to ensure this revision aligns with our shared vision for 
>>> LLVM/Clang, and your reviews will greatly contribute to this goal. If there 
>>> are any other changes or improvements required for the successful landing 
>>> of this revision, please feel free to let me know.
>>
>> I'll put you on my list to re-review for early next week, though Aaron 
>> probably needs to do a look through this as well.
>
> Thank you, @erichkeane that would be great, are you referring to 
> @aaron.ballman ?

Yes.


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

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

codemzs added a subscriber: aaron.ballman.
codemzs added a comment.

In D149573#4391013 , @erichkeane 
wrote:

> In D149573#4390895 , @codemzs wrote:
>
>> In D149573#4390863 , @stuij wrote:
>>
>>> This is going to be a very unhelpful comment. After looking through the 
>>> changes, I don't have any comments to make, but I also don't feel 
>>> comfortable to accept this revision as I don't feel to know enough about 
>>> the front-end.
>>
>> @stuij, I sincerely appreciate you taking the time to review the changes. 
>> Your hesitation due to unfamiliarity with the front-end elements is 
>> completely understandable, and I respect your candid feedback.
>>
>> @erichkeane, given your extensive contributions to the core `Sema`* files, I 
>> believe your expertise and experience would be particularly valuable in 
>> reviewing the changes I've made. I recall your initial informal approval for 
>> the change, and since then, I've further refined it after incorporating the 
>> outcomes of D150913 . I'd be most 
>> appreciative if you could please review this revision once again.
>>
>> My intention is to ensure this revision aligns with our shared vision for 
>> LLVM/Clang, and your reviews will greatly contribute to this goal. If there 
>> are any other changes or improvements required for the successful landing of 
>> this revision, please feel free to let me know.
>
> I'll put you on my list to re-review for early next week, though Aaron 
> probably needs to do a look through this as well.

Thank you, @erichkeane that would be great, are you referring to @aaron.ballman 
?


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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D149573#4390895 , @codemzs wrote:

> In D149573#4390863 , @stuij wrote:
>
>> This is going to be a very unhelpful comment. After looking through the 
>> changes, I don't have any comments to make, but I also don't feel 
>> comfortable to accept this revision as I don't feel to know enough about the 
>> front-end.
>
> @stuij, I sincerely appreciate you taking the time to review the changes. 
> Your hesitation due to unfamiliarity with the front-end elements is 
> completely understandable, and I respect your candid feedback.
>
> @erichkeane, given your extensive contributions to the core `Sema`* files, I 
> believe your expertise and experience would be particularly valuable in 
> reviewing the changes I've made. I recall your initial informal approval for 
> the change, and since then, I've further refined it after incorporating the 
> outcomes of D150913 . I'd be most 
> appreciative if you could please review this revision once again.
>
> My intention is to ensure this revision aligns with our shared vision for 
> LLVM/Clang, and your reviews will greatly contribute to this goal. If there 
> are any other changes or improvements required for the successful landing of 
> this revision, please feel free to let me know.

I'll put you on my list to re-review for early next week, though Aaron probably 
needs to do a look through this as well.


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

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

codemzs added a comment.

In D149573#4390863 , @stuij wrote:

> This is going to be a very unhelpful comment. After looking through the 
> changes, I don't have any comments to make, but I also don't feel comfortable 
> to accept this revision as I don't feel to know enough about the front-end.

@stuij, I sincerely appreciate you taking the time to review the changes. Your 
hesitation due to unfamiliarity with the front-end elements is completely 
understandable, and I respect your candid feedback.

@erichkeane, given your extensive contributions to the core `Sema`* files, I 
believe your expertise and experience would be particularly valuable in 
reviewing the changes I've made. I recall your initial informal approval for 
the change, and since then, I've further refined it after incorporating the 
outcomes of D150913 . I'd be most 
appreciative if you could please review this revision once again.

My intention is to ensure this revision aligns with our shared vision for 
LLVM/Clang, and your reviews will greatly contribute to this goal. If there are 
any other changes or improvements required for the successful landing of this 
revision, please feel free to let me know.


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

[Ties Stuij via Phabricator via cfe-commits]

stuij added a comment.

This is going to be a very unhelpful comment. After looking through the 
changes, I don't have any comments to make, but I also don't feel comfortable 
to accept this revision as I don't feel to know enough about the front-end.


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

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

codemzs updated this revision to Diff 526524.
codemzs added a comment.

Remove unused header file.


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

https://reviews.llvm.org/D149573

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Lex/LiteralSupport.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Frontend/InitPreprocessor.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/SemaOverload.cpp
  clang/test/CodeGenCXX/cxx23-fp-ext-std-names-p1467r9.cpp
  clang/test/CodeGenCXX/cxx23-vector-bfloat16.cpp
  clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp

Index: clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
===
--- /dev/null
+++ clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
@@ -0,0 +1,465 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++23 -target-feature +fullbf16 -verify -ast-dump %s | FileCheck %s
+
+_Float16 f16_val_1 = 1.0bf16; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type '__bf16'}}
+_Float16 f16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'float'}}
+_Float16 f16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'double'}}
+_Float16 f16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'long double'}}
+_Float16 f16_val_6 = 1.0f16;
+//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}}
+_Float16 f16_val_8 = static_cast<_Float16>(1.0f);
+//CHECK:  VarDecl {{.*}} f16_val_8 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'float' 1.00e+00
+_Float16 f16_val_9 = static_cast<_Float16>(1.0);
+//CHECK:  VarDecl {{.*}} f16_val_9 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'double' 1.00e+00
+_Float16 f16_val_10 = static_cast<_Float16>(1.0l);
+//CHECK:  VarDecl {{.*}} f16_val_10 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'long double' 1.00e+00
+_Float16 f16_val_11 = static_cast<_Float16>(1.0f16);
+//CHECK:  VarDecl {{.*}} f16_val_11 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} '_Float16' 1.00e+00
+
+decltype(0.0BF16) bf16_val_1 = 1.0f16; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type '_Float16'}}
+decltype(0.0BF16) bf16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'float'}}
+decltype(0.0BF16) bf16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'double'}}
+decltype(0.0BF16) bf16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'long double'}}
+decltype(0.0BF16) bf16_val_5 = 1.0bf16;
+//CHECK:  VarDecl {{.*}} bf16_val_5 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: FloatingLiteral {{.*}} '__bf16' 1.00e+00
+
+decltype(0.0BF16) bf16_val_6 = static_cast(1.0f16); // expected-error {{static_cast from '_Float16' to 'decltype(0.BF16)' (aka '__bf16') is not allowed}}
+decltype(0.0BF16) bf16_val_7 = static_cast(1.0f);
+//CHECK:  VarDecl {{.*}} bf16_val_7 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} 'decltype(0.BF16)':'__bf16' static_cast 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'float' 1.00e+00
+decltype(0.0BF16) bf16_val_8 = static_cast(1.0);
+//CHECK:  VarDecl {{.*}} bf16_val_8 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} 'decltype(0.BF16)':'__bf16' static_cast 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'double' 1.00e+00
+decltype(0.0BF16) bf16_val_9 = static_cast(1.0l);
+//CHECK:  VarDecl {{.*}} bf16_val_9 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} 'decltype(0.BF16)':'__bf16' static_cast 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'long double' 1.00e+00
+decltype(0.0BF16) bf16_val_10 = static_cast(1.0bf16);
+//CHECK:  VarDecl {{.*}} bf16_val_10 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} 

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

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

codemzs updated this revision to Diff 526519.
codemzs retitled this revision from "[Clang][C++23] Implement core language 
changes from P1467R9 extended floating-point types and standard names and adapt 
__bf16 to be arithmetic type" to "[Clang][C++23] Implement core language 
changes from P1467R9 extended floating-point types and standard names".
codemzs edited the summary of this revision.
codemzs set the repository for this revision to rG LLVM Github Monorepo.
codemzs added a comment.

Hi @tahonermann / @erichkeane ,

This change has been simplified and rebased onto D150913 
, which is now in LLVM main. I hope this 
makes the review process more straightforward.

CC: @rjmccall / @pengfei


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149573

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Lex/LiteralSupport.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Frontend/InitPreprocessor.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/SemaOverload.cpp
  clang/test/CodeGenCXX/cxx23-fp-ext-std-names-p1467r9.cpp
  clang/test/CodeGenCXX/cxx23-vector-bfloat16.cpp
  clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp

Index: clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
===
--- /dev/null
+++ clang/test/Sema/cxx23-fp-ext-std-names-p1467r9.cpp
@@ -0,0 +1,465 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++23 -target-feature +fullbf16 -verify -ast-dump %s | FileCheck %s
+
+_Float16 f16_val_1 = 1.0bf16; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type '__bf16'}}
+_Float16 f16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'float'}}
+_Float16 f16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'double'}}
+_Float16 f16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type '_Float16' with an rvalue of type 'long double'}}
+_Float16 f16_val_6 = 1.0f16;
+//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}}
+_Float16 f16_val_8 = static_cast<_Float16>(1.0f);
+//CHECK:  VarDecl {{.*}} f16_val_8 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'float' 1.00e+00
+_Float16 f16_val_9 = static_cast<_Float16>(1.0);
+//CHECK:  VarDecl {{.*}} f16_val_9 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'double' 1.00e+00
+_Float16 f16_val_10 = static_cast<_Float16>(1.0l);
+//CHECK:  VarDecl {{.*}} f16_val_10 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'long double' 1.00e+00
+_Float16 f16_val_11 = static_cast<_Float16>(1.0f16);
+//CHECK:  VarDecl {{.*}} f16_val_11 '_Float16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} '_Float16' static_cast<_Float16> 
+//CHECK-NEXT: FloatingLiteral {{.*}} '_Float16' 1.00e+00
+
+decltype(0.0BF16) bf16_val_1 = 1.0f16; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type '_Float16'}}
+decltype(0.0BF16) bf16_val_2 = 1.0f; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'float'}}
+decltype(0.0BF16) bf16_val_3 = 1.0; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'double'}}
+decltype(0.0BF16) bf16_val_4 = 1.0l; // expected-error {{cannot initialize a variable of type 'decltype(0.BF16)' (aka '__bf16') with an rvalue of type 'long double'}}
+decltype(0.0BF16) bf16_val_5 = 1.0bf16;
+//CHECK:  VarDecl {{.*}} bf16_val_5 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: FloatingLiteral {{.*}} '__bf16' 1.00e+00
+
+decltype(0.0BF16) bf16_val_6 = static_cast(1.0f16); // expected-error {{static_cast from '_Float16' to 'decltype(0.BF16)' (aka '__bf16') is not allowed}}
+decltype(0.0BF16) bf16_val_7 = static_cast(1.0f);
+//CHECK:  VarDecl {{.*}} bf16_val_7 'decltype(0.BF16)':'__bf16' cinit
+//CHECK-NEXT: CXXStaticCastExpr {{.*}} 'decltype(0.BF16)':'__bf16' static_cast 
+//CHECK-NEXT: FloatingLiteral {{.*}} 'float' 1.00e+00
+decltype(0.0BF16) bf16_val_8 = 

[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names and adapt __bf16 to be arithmetic type

[Ties Stuij via Phabricator via cfe-commits]

stuij added a comment.

In D149573#4341323 , @codemzs wrote:

> @stuij, as you initially introduced `__bf16` as a storage-type, your review 
> on this adjustment would be greatly appreciated.

Sorry for the late reaction, was on holiday. Yes, I'll have a look. I've put it 
on my todo.


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 adapt __bf16 to be arithmetic type

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

codemzs added a comment.

I have created a separate change to upgrade existing `__bf16` to arithmetic 
type at D150913 




Comment at: clang/lib/AST/Type.cpp:
+  return (BT->getKind() >= BuiltinType::Bool &&
+  BT->getKind() <= BuiltinType::Ibm128 &&
+  (BT->getKind() != BuiltinType::BFloat16 ||

Remove



Comment at: clang/lib/Basic/Targets/AMDGPU.h:122
+  const char *getBFloat16Mangling() const override { return "DF16b"; };
+  
   std::string_view getClobbers() const override { return ""; }

Remove


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