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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a subscriber: hubert.reinterpretcast.
aaron.ballman added a comment.

In D136568#3880711 <https://reviews.llvm.org/D136568#3880711>, @jcranmer-intel wrote:

> 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.

+1 to this point; the edge cases are pretty tricky and the standard is the source of truth for how we should be implementing this stuff.



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


================
Comment at: llvm/include/llvm/ADT/APFloat.h:450-454
   /// For special APFloat values, this returns special error codes:
   ///
   ///   NaN -> \c IEK_NaN
   ///   0   -> \c IEK_Zero
   ///   Inf -> \c IEK_Inf
----------------
Is this part of the comment still accurate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136568