[PATCH] D134369: [Clang] Support constexpr builtin fmax

[Joshua Cranmer via Phabricator via cfe-commits]

jcranmer-intel added a comment.

In D134369#3821949 <https://reviews.llvm.org/D134369#3821949>, @efriedma wrote:

> I think __builtin_fmax can raise a floating-point exception; in that case, it wouldn't be constant, I think?  Not sure how consistent we are about handling that sort of thing in constant evaluation at the moment.

https://eel.is/c++draft/library.c#3 says that a floating-point exception other than `FE_INEXACT` causes it to not be a constant expression, if I have constant expression wording right. `FE_INVALID` can be raised if the inputs are signalling NaNs, but even if we're claiming implementation of Annex F, it's okay to treat sNaN as qNaN unless we're claiming `FE_SNANS_ALWAYS_SIGNAL` (this is new in C2x, I think). We shouldn't be claiming that except maybe if we're using `-fp-model=strict`. I don't think `#pragma STDC FENV_ACCESS ON` bears any relevance here, but now I'm wondering what needs to happen with that for regular floating-point operations that may trigger exceptions.



================
Comment at: clang/lib/AST/ExprConstant.cpp:14033-14034
+        !EvaluateFloat(E->getArg(1), RHS, Info))
+      return false;
+    if (Result.isNaN() || RHS > Result)
+      Result = RHS;
----------------
If I'm reading APFloat's source correctly, this doesn't suffice to consistently make __builtin_fmax(±0.0, ±0.0) return +0.0 if one of the zeroes is positive. (We're not required to return +0.0 in this case, but most libm implementations seem to endeavor to return +0.0 in this situation, even if the LLVM intrinsic we lower to doesn't).


================
Comment at: clang/test/Sema/constant-builtins-fmax.cpp:35-39
+#define FMAX_TEST_BOTH_ZERO(T, FUNC)       \
+    static_assert(0.0 == FUNC(0.0, 0.0));  \
+    static_assert(0.0 == FUNC(-0.0, 0.0)); \
+    static_assert(0.0 == FUNC(0.0, -0.0)); \
+    static_assert(0.0 == FUNC(-0.0, -0.0));
----------------
These tests aren't covering what the sign of zero is in these cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134369