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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:14033-14034
+        !EvaluateFloat(E->getArg(1), RHS, Info))
+      return false;
+    if (Result.isNaN() || RHS > Result)
+      Result = RHS;
----------------
jcranmer-intel wrote:
> 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).
These cases are covered in tests (in a wrong manner though as I see), I was "checking" that we have `+0.0` if one of the zeroes is positive


================
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));
----------------
jcranmer-intel wrote:
> These tests aren't covering what the sign of zero is in these cases.
Cool! Thank you, today I learned that this compiles:
```
static_assert(0.0 == 0.0);
static_assert(0.0 == -0.0);
static_assert(0.0 == +0.0);
```

So I think I need to test the `+0.0` cases with the `__builtin_signbit` (I didn't find another way), BUT this function is not constexpr yet. So I need to implement a constexpr `__builtin_signbit` first... In a new pull request.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134369