hubert.reinterpretcast requested changes to this revision. hubert.reinterpretcast added inline comments. This revision now requires changes to proceed.
================ Comment at: clang/lib/AST/ExprConstant.cpp:12452 + int Ilogb; + if (APFloat::opStatus St = ilogb(F, Ilogb); !isConstantOpStatus(St)) + return false; ---------------- Izaron wrote: > hubert.reinterpretcast wrote: > > hubert.reinterpretcast wrote: > > > hubert.reinterpretcast wrote: > > > > Izaron wrote: > > > > > majnemer wrote: > > > > > > Izaron wrote: > > > > > > > jcranmer-intel wrote: > > > > > > > > Izaron wrote: > > > > > > > > > aaron.ballman wrote: > > > > > > > > > > jcranmer-intel wrote: > > > > > > > > > > > `long double` is `ppc_fp128` on at least some PPC > > > > > > > > > > > targets, and while I'm not entirely certain of what > > > > > > > > > > > `ilogb` properly returns in the corner cases of the > > > > > > > > > > > `ppc_fp128`, I'm not entirely confident that the > > > > > > > > > > > implementation of `APFloat` is correct in those cases. > > > > > > > > > > > I'd like someone with PPC background to comment in here. > > > > > > > > > > Paging @hubert.reinterpretcast for help finding a good > > > > > > > > > > person to comment on the PPC questions. > > > > > > > > > @jcranmer-intel constexpr evaluation is quite > > > > > > > > > machine-/target-independent. Clang evaluates it based on its > > > > > > > > > **internal** representation of float variables. > > > > > > > > > > > > > > > > > > [[ > > > > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/include/llvm/ADT/APFloat.h#L1256 > > > > > > > > > | int ilogb ]] uses `Arg.getIEEE()`, that [[ > > > > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/include/llvm/ADT/APFloat.h#L819-L825 > > > > > > > > > | returns Clang's internal float representation ]]. > > > > > > > > > > > > > > > > > > Whichever float semantics is being used, [[ > > > > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/llvm/lib/Support/APFloat.cpp#L54-L61 > > > > > > > > > | minExponent and maxExponent are representable as > > > > > > > > > APFloatBase::ExponentType ]], which is `int32_t`: > > > > > > > > > ``` > > > > > > > > > /// A signed type to represent a floating point numbers > > > > > > > > > unbiased exponent. > > > > > > > > > typedef int32_t ExponentType; > > > > > > > > > ``` > > > > > > > > > > > > > > > > > > We already use `int ilogb` in some constexpr evaluation code: > > > > > > > > > [[ > > > > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/clang/lib/AST/ExprConstant.cpp#L14592 > > > > > > > > > | link ]], it is working correct because it is working on > > > > > > > > > Clang's float representations. > > > > > > > > > We already use `int ilogb` in some constexpr evaluation code: > > > > > > > > > [[ > > > > > > > > > https://github.com/llvm/llvm-project/blob/2e5bf4da99a2f8d3d4bb4f1a4d1ed968a01e8f02/clang/lib/AST/ExprConstant.cpp#L14592 > > > > > > > > > | link ]], it is working correct because it is working on > > > > > > > > > Clang's float representations. > > > > > > > > > > > > > > > > `APFloat::getIEEE()`, if I'm following it correctly, only > > > > > > > > returns the details of the high double in `ppc_fp128` floats, > > > > > > > > and I'm not sufficiently well-versed in the `ppc_fp128` format > > > > > > > > to establish whether or not the low double comes into play > > > > > > > > here. glibc seems to think that the low double comes into play > > > > > > > > in at least one case: > > > > > > > > https://github.com/bminor/glibc/blob/30891f35fa7da832b66d80d0807610df361851f3/sysdeps/ieee754/ldbl-128ibm/e_ilogbl.c > > > > > > > Thanks for the link to the glibc code! It helped me to understand > > > > > > > the IEEE754 standard better. > > > > > > > > > > > > > > I did some research and it seems like AST supports a fixed set of > > > > > > > float types, each working good with `ilogb`: > > > > > > > ``` > > > > > > > half (__fp16, only for OpenCL), float16, float, double, long > > > > > > > double, float128 > > > > > > > ``` > > > > > > > [[ > > > > > > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/lib/Sema/SemaExpr.cpp#L3911-L3931 > > > > > > > | link to SemaExpr.cpp ]] > > > > > > > > > > > > > > It means that the constant evaluator doesn't deal with other > > > > > > > float types including `ppc_fp128`. > > > > > > > If `ppc_fp128` was supported on the AST level, it would anyway > > > > > > > come through type casting, and `__builtin_ilog<SUFFIX>` would > > > > > > > deal with a value of a known type. > > > > > > > > > > > > > > I checked the list of builtins - each builtin argument of float > > > > > > > type also supports only common float types: > > > > > > > [[ > > > > > > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/include/clang/Basic/Builtins.def#L27-L31 > > > > > > > | link to Builtins.def 1 ]] > > > > > > > [[ > > > > > > > https://github.com/llvm/llvm-project/blob/7846d590033e8d661198f4c00f56f46a4993c526/clang/include/clang/Basic/Builtins.def#L53-L54 > > > > > > > | link to Builtins.def 2 ]] > > > > > > Won't long double map to ppc_fp128 for some targets? > > > > > Hi! It will map, but only **after** all the constant (constexpr) > > > > > calculations are done (that is, after the AST parsing stage) - in the > > > > > Codegen stage. > > > > > > > > > > The Clang's constant evaluator itself never deals with ppc_fp128 and > > > > > doesn't care about the target. > > > > > While parsing the AST, the constant evaluator works on the same level > > > > > with it, providing calculated values to the AST being built > > > > > on-the-fly. At the moment AST is built, constant evaluation is over. > > > > > The evaluator is target-independent and uses the internal > > > > > representation for `long double`, in the form of emulated **80-bit > > > > > (x86) format**. > > > > > > > > > > The Codegen can map the AST's `long double` to `ppc_fp128` on some > > > > > targets. > > > > > It doesn't cause problems because x87 80-bit float is convertible to > > > > > ppc_fp128 without precision loss. > > > > > But the constexpr `long double` values itself were calculated using > > > > > the Clang's 80-bit format emulation, before the Codegen stage. > > > > > > > > > > I'm sorry if I'm not describing it clearly. It's important to me that > > > > > everyone understands what the trick is =) > > > > > So, the constant evaluator does everything with 80-bit floats and at > > > > > the end they can be mapped on ppc_fp128 floats if the target requires > > > > > it. > > > > That's kind-of terrible, but at least that means that what ilogb can do > > > > within the scope of this patch is very clear. > > > > I am surprised to find that ilogb({2, -0x1p-64}) seems to return 1 on > > > > ppc64le Linux. > > > > > > > Of course I should have used `ilogbl`... > > > So, yes, ilogbl({2, -0x1p-64}) returns 0 on ppc64le Linux. > > @Izaron, > > > > > The evaluator is target-independent and uses the internal representation > > > for long double, in the form of emulated 80-bit (x86) format. > > > > This the internal representation still 80-bit for platforms where long > > double uses the IEEE binary 128-bit format? > > ilogbl({2, -0x1p-64}) returns 0 on ppc64le Linux. > > How did you use `ilogbl`? I built local Clang with PowerPC support, this > worked fine to me: > ``` > static_assert(__builtin_ilogbl(-0x1p-64) == -64); > static_assert(__builtin_ilogbl(-0x1p-1074L) == -1074); > // smaller exponents (< -1074) don't work, too small number > ``` > I would be glad for any example where constant ilogb would work incorrectly > but runtime ilogb correctly. Unfortunately I can't make up such number > because I navigate not so good enough in IEEE754 corner cases =( > > > This the internal representation still 80-bit for platforms where long > > double uses the IEEE binary 128-bit format? > > I just checked with the debugger, the `-target ppc64le` parameter actually > makes the constant evaluator use internal **128**-bit format for long double. > Yeah, this contradicts to some points what I wrote earlier... =( > I would be glad for any example where constant ilogb would work incorrectly > but runtime ilogb correctly. Unfortunately I can't make up such number > because I navigate not so good enough in IEEE754 corner cases =( By `ilogbl({2, -0x1p-64})`, I meant `ilogbl(2.L + -0x1p-64)`. The answer I got at runtime is `0` (which is what I expected). It seems the `ilogbl` implementation at compile time does //not// return `0` though. Here, I try the largest number in the format that is smaller than 2: ``` $ cat ilogb.cc #include <math.h> #include <stdio.h> constexpr int comptime = __builtin_ilogbl(2.L - __DBL_DENORM_MIN__); constinit volatile long double rtinput = 2.L - __DBL_DENORM_MIN__; int runtime; int main(void) { runtime = ilogbl(rtinput); printf("comptime(%d)\nruntime(%d)\n", comptime, runtime); } $ clang++ -target powerpc64le-unknown-linux-gnu -std=c++20 ilogb.cc && ./a.out comptime(1) runtime(0) ``` ================ Comment at: clang/test/Sema/constant-builtins-ilogb.cpp:1 +// RUN: %clang_cc1 -fsyntax-only -verify %s +// expected-no-diagnostics ---------------- There seems to be no C language test in the patch (although the builtin presumably is okay at least as part of arithmetic constant expressions). @aaron.ballman, what are your thoughts re: integer constant expression contexts? For example: ``` struct C { int x : __builtin_ilogb(1. + 1.); }; ``` ================ Comment at: clang/test/Sema/constant-builtins-ilogb.cpp:60 + +#if !defined(_WIN32) && !defined(_WIN64) // in MSVC sizeof(long double) == sizeof(double) +static_assert(__builtin_ilogbl(3.64519953188247460253E-4951L) == -16445); ---------------- This test also fails for PPC double-double (which does not increase the range of exponents). With C++, it is presumably possible for you to verify using predefined floating point constants with platform-specific values, e.g., `__FLT_DENORM_MIN__`, and checking the answer by dividing or multiplying the number by 2 (or more generally, the base) in a constexpr function repeatedly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136568/new/ https://reviews.llvm.org/D136568 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
