majnemer added inline comments.
================ Comment at: clang/lib/AST/ExprConstant.cpp:12452 + int Ilogb; + if (APFloat::opStatus St = ilogb(F, Ilogb); !isConstantOpStatus(St)) + return false; ---------------- 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? 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
