[Clang] Support constexpr builtin fmax

[Izaron]

Summary

Support constexpr version of __builtin_fmax and its variations.

[Izaron]

This patch is derived from https://reviews.llvm.org/D134136 (which was abandoned due to poor tests and overcomplication)

The constexpr version of fmax matches the libc realization: the libc realization is here.

It is verified with test that matches libc test: the libc test is here.

If this patch is approved, I promise to do a similar patch for the fmin function 😀

[efriedma]

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.

[jcranmer-intel]

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

clang/lib/AST/ExprConstant.cpp

14033–14034

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

clang/test/Sema/constant-builtins-fmax.cpp

36–40

These tests aren't covering what the sign of zero is in these cases.

[efriedma]

We shouldn't be claiming that except maybe if we're using -fp-model=strict

Makes sense.

I'm wondering what needs to happen with that for regular floating-point operations that may trigger exceptions

In general, floating-point exceptions are affected by whether we're semantically required to do constant evaluation (InConstantContext in the code). If we're evaluating something that has to be a constant expression, we ignore exceptions. Otherwise, we're doing opportunistic evaluation, so we have to honor the rounding mode specified by flags/pragmas.

[Izaron]

clang/lib/AST/ExprConstant.cpp

14033–14034

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

clang/test/Sema/constant-builtins-fmax.cpp

36–40

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.

[Izaron]

Rebase and fix windows __float128 failure

[efriedma]

clang/test/Sema/constant-builtins-fmax.cpp

36–40

I think __builtin_copysign should be sufficient.

[Izaron]

Fix corner case with pos/neg zeroes

Fix tests for pos/neg zeroes with __builtin_copysign

Thanks to @efriedma and @jcranmer-intel!

[aaron.ballman]

Regarding Annex F requirements, do we document that we conform to Annex F and do we have any actual test coverage for it? This question has come up for me quite a bit as I've worked on our C status page and I've never found a satisfactory answer of what we intend to support.

In D134369#3822144, @jcranmer-intel wrote:

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.

I think FENV_ACCESS is relevant. [library.c]p3 says C++ constant expressions follow Annex F requirements, and C2x F.8.4p1 says that FENV_ACCESS impacts constant expression evaluation.

clang/lib/AST/ExprConstant.cpp

14034

[Izaron]

Fix comment wording.

Thanks to @aaron.ballman!

[jcranmer-intel]

clang/lib/AST/ExprConstant.cpp

14029–14030

I think you should add a comment saying // TODO: Handle sNaN, just so that we remember to revisit this later.

[Izaron]

Add TODO comment about sNaN. Thanks to @jcranmer-intel!

[Izaron]

I've got an email about "Buildbot failure" with this link: https://lab.llvm.org/buildbot/#/builders/216/builds/10955

Line 51: 'static_assert' with no message is a C++17 extension

Why is it failing if pre-merge checks has been passed? =(

[efriedma]

You might need to explicitly specify -std=c++17 in the RUN line. (That bot has a different target triple, and I think with that triple the default C++ standard isn't C++17?)

[Izaron]

In D134369#3843824, @efriedma wrote:

You might need to explicitly specify -std=c++17 in the RUN line. (That bot has a different target triple, and I think with that triple the default C++ standard isn't C++17?)

Thanks @efriedma! Could you please take a look at my fix with added -std=c++17? https://reviews.llvm.org/D135486

Sorry for inconvenience, it was a sad surprise =(

clang/docs/LanguageExtensions.rst

 * ``__builtin_clzs``
 * ``__builtin_ctz``
 * ``__builtin_ctzl``
 * ``__builtin_ctzll``
 * ``__builtin_ctzs``
 * ``__builtin_ffs``
 * ``__builtin_ffsl``
 * ``__builtin_ffsll``
+* ``__builtin_fmax``
 * ``__builtin_fpclassify``
 * ``__builtin_inf``
 * ``__builtin_isinf``
 * ``__builtin_isinf_sign``
 * ``__builtin_isfinite``
 * ``__builtin_isnan``
 * ``__builtin_isnormal``
 * ``__builtin_nan``

clang/lib/AST/ExprConstant.cpp

bool FloatExprEvaluator::VisitCallExpr(const CallExpr *E) {
   case Builtin::BI__builtin_copysignf128: {
     APFloat RHS(0.);
     if (!EvaluateFloat(E->getArg(0), Result, Info) ||
         !EvaluateFloat(E->getArg(1), RHS, Info))
       return false;
     Result.copySign(RHS);
     return true;
   }
+
+  case Builtin::BI__builtin_fmax:
+  case Builtin::BI__builtin_fmaxf:
+  case Builtin::BI__builtin_fmaxl:
+  case Builtin::BI__builtin_fmaxf16:
+  case Builtin::BI__builtin_fmaxf128: {
+    // TODO: Handle sNaN.
+    APFloat RHS(0.);
+    if (!EvaluateFloat(E->getArg(0), Result, Info) ||
+        !EvaluateFloat(E->getArg(1), RHS, Info))
+      return false;
+    // When comparing zeroes, return +0.0 if one of the zeroes is positive.
+    if (Result.isZero() && RHS.isZero() && Result.isNegative())
+      Result = RHS;
+    else if (Result.isNaN() || RHS > Result)
+      Result = RHS;
+    return true;
+  }
   }
 }
 
 bool FloatExprEvaluator::VisitUnaryReal(const UnaryOperator *E) {
   if (E->getSubExpr()->getType()->isAnyComplexType()) {
     ComplexValue CV;
     if (!EvaluateComplex(E->getSubExpr(), CV, Info))
       return false;

clang/test/Sema/constant-builtins-fmax.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+constexpr double NaN = __builtin_nan("");
+constexpr double Inf = __builtin_inf();
+constexpr double NegInf = -__builtin_inf();
+
+#define FMAX_TEST_SIMPLE(T, FUNC)                           \
+    static_assert(T(6.7890) == FUNC(T(1.2345), T(6.7890))); \
+    static_assert(T(6.7890) == FUNC(T(6.7890), T(1.2345)));
+
+#define FMAX_TEST_NAN(T, FUNC)                          \
+    static_assert(Inf == FUNC(NaN, Inf));               \
+    static_assert(NegInf == FUNC(NegInf, NaN));         \
+    static_assert(0.0 == FUNC(NaN, 0.0));               \
+    static_assert(-0.0 == FUNC(-0.0, NaN));             \
+    static_assert(T(-1.2345) == FUNC(NaN, T(-1.2345))); \
+    static_assert(T(1.2345) == FUNC(T(1.2345), NaN));   \
+    static_assert(__builtin_isnan(FUNC(NaN, NaN)));
+
+#define FMAX_TEST_INF(T, FUNC)                  \
+    static_assert(Inf == FUNC(NegInf, Inf));    \
+    static_assert(Inf == FUNC(Inf, 0.0));       \
+    static_assert(Inf == FUNC(-0.0, Inf));      \
+    static_assert(Inf == FUNC(Inf, T(1.2345))); \
+    static_assert(Inf == FUNC(T(-1.2345), Inf));
+
+#define FMAX_TEST_NEG_INF(T, FUNC)                         \
+    static_assert(Inf == FUNC(Inf, NegInf));               \
+    static_assert(0.0 == FUNC(NegInf, 0.0));               \
+    static_assert(-0.0 == FUNC(-0.0, NegInf));             \
+    static_assert(T(-1.2345) == FUNC(NegInf, T(-1.2345))); \
+    static_assert(T(1.2345) == FUNC(T(1.2345), NegInf));
+
+#define FMAX_TEST_BOTH_ZERO(T, FUNC)       \
+    static_assert(__builtin_copysign(1.0, FUNC(0.0, 0.0)) == 1.0);  \
+    static_assert(__builtin_copysign(1.0, FUNC(-0.0, 0.0)) == 1.0);  \
+    static_assert(__builtin_copysign(1.0, FUNC(0.0, -0.0)) == 1.0);  \
+    static_assert(__builtin_copysign(1.0, FUNC(-0.0, -0.0)) == -1.0);
+
+#define LIST_FMAX_TESTS(T, FUNC) \
+    FMAX_TEST_SIMPLE(T, FUNC)    \
+    FMAX_TEST_NAN(T, FUNC)       \
+    FMAX_TEST_INF(T, FUNC)       \
+    FMAX_TEST_NEG_INF(T, FUNC)   \
+    FMAX_TEST_BOTH_ZERO(T, FUNC)
+
+LIST_FMAX_TESTS(double, __builtin_fmax)
+LIST_FMAX_TESTS(float, __builtin_fmaxf)
+LIST_FMAX_TESTS((long double), __builtin_fmaxl)
+LIST_FMAX_TESTS(__fp16, __builtin_fmaxf16)
+#ifdef __FLOAT128__
+LIST_FMAX_TESTS(__float128, __builtin_fmaxf128)
+#endif