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

Mon Nov 20 12:56:13 PST 2023

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<std::array<FloatingRankCompareResult, 5>, 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