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

Mon Oct 24 14:39:37 PDT 2022

jcranmer-intel added a comment.

In D136568#3878104 <https://reviews.llvm.org/D136568#3878104>, @Izaron wrote:

> The online documentation (https://en.cppreference.com/w/cpp/numeric/math/ilogb) says:
>
>   1. If the correct result is greater than INT_MAX or smaller than INT_MIN, FE_INVALID is raised.
>   2. If arg is ±0, ±∞, or NaN, FE_INVALID is raised.
>   3. In all other cases, the result is exact (FE_INEXACT is never raised) and the current rounding mode is ignored
>
> The first point seemingly never occur, because llvm's `ilogb` return type is `int`.
> The second point is handled as expected (`APFloatTest.cpp` checks it)

FWIW, I would be slightly wary of relying on cppreference as definitive for niche semantic issues like this, although it is correct in this case. C2x is actually pretty well-organized in Annex F, and enumerates most of the corner cases specifically in every function.

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

================
Comment at: clang/test/Sema/constant-builtins-ilogb.cpp:13
+    static_assert(!__builtin_constant_p(FUNC(T(Inf))));    \
+    static_assert(!__builtin_constant_p(FUNC(T(NegInf))));
+
----------------
Test of smallest subnormal and largest finite number would also be useful.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136568