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

[Tom Honermann via Phabricator via libcxx-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 into extended FP types. One way to fix it would be to introduce `isPromotableFloatingType()` and `getPromotedFloatingType()` functions to match the existing ones integers used above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149573