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

[Joshua Cranmer via Phabricator via cfe-commits]

jcranmer-intel 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:
> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136568