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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
