[PATCH] D136568: [Clang] Support constexpr builtin ilogb

Thu Oct 27 11:20:52 PDT 2022

hubert.reinterpretcast added inline comments.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136568