[clang][Interp] Implement __builtin_fmin

[tbaeder]

Summary

Split out from https://reviews.llvm.org/D155368

[cor3ntin]

Looks fine but where are the tests?

[tbaeder]

clang/test/Sema/constant-builtins-fmin.cpp actually needed quite a lot of builtins before it could succeed, so it's enabled in https://reviews.llvm.org/D155369

[aaron.ballman]

I'd like to see test coverage for treatment of NaNs. According to the C23 standard, a quiet NaN is treated as missing data for fmax and fmin; so if there's a quiet NaN and a numeric value, the numeric value it's what's returned.

It's not yet clear to me what happens when any of these functions encounter a signaling NaN at compile time. CC @hubert.reinterpretcast @jcranmer-intel @rsmith

[tbaeder]

In D155546#4510691, @aaron.ballman wrote:

I'd like to see test coverage for treatment of NaNs. According to the C23 standard, a quiet NaN is treated as missing data for fmax and fmin; so if there's a quiet NaN and a numeric value, the numeric value it's what's returned.

Isn't this always the case?

constexpr float qNan = __builtin_nan("");

constexpr float min = __builtin_fmin(qNan, 1);
static_assert(min == 1);
constexpr float min2 = __builtin_fmin(1, qNan);
static_assert(min2 == 1);

works already.

[aaron.ballman]

In D155546#4521383, @tbaeder wrote:

In D155546#4510691, @aaron.ballman wrote:

I'd like to see test coverage for treatment of NaNs. According to the C23 standard, a quiet NaN is treated as missing data for fmax and fmin; so if there's a quiet NaN and a numeric value, the numeric value it's what's returned.

Isn't this always the case?

constexpr float qNan = __builtin_nan("");

constexpr float min = __builtin_fmin(qNan, 1);
static_assert(min == 1);
constexpr float min2 = __builtin_fmin(1, qNan);
static_assert(min2 == 1);

works already.

I believe it generally is always true, but it's good to have the test coverage.

It's not yet clear to me what happens when any of these functions encounter a signaling NaN at compile time. CC @hubert.reinterpretcast @jcranmer-intel @rsmith

I asked some members of the C floating point study group and it turns out signaling NaN is a bit weirder than I had realized. Basically, a signal should be raised during any "mathematical operation" unless the operation is explicitly defined to *not* raise the signal or the implementation defines that it doesn't signal under those circumstances. e.g.,

float some_float_nan(); // Produces a signaling NaN of float type

double d = some_float_nan(); // Subject to F.3p4, might signal, might not
float f = some_float_nan(); // Also subject to F.3p4
sizeof(some_float_nan()); // No signal, unevaluated operand
isnan(some_float_nan()); // No signal, allowed explicitly by standard
some_float_nan() + 12; // Signals
(void)some_float_nan(); // No signal, no mathematical operation
(int)some_float_nan(); // Subject to Annex F, might or might not signal

Also, translation-time initialization with SNAN macros don’t convert to a quiet NaN (which is the return value for the default handling of the signal due to a signaling NaN operand of a runtime conversion).

[jcranmer-intel]

In D155546#4510691, @aaron.ballman wrote:

It's not yet clear to me what happens when any of these functions encounter a signaling NaN at compile time. CC @hubert.reinterpretcast @jcranmer-intel @rsmith

fmin(sNaN, x) signals FE_INVALID, which would be a compile-time error per [library.c]p3 (every FP exception save FE_INEXACT is a compile-time error). Except sNaN raising FE_INVALID is only a recommended practice, so it's really unclear what the C++ standard attempts to say on this matter.

Treating sNaN as always signaling FE_INVALID is probably the safer option.

[aaron.ballman]

In D155546#4532839, @jcranmer-intel wrote:

Treating sNaN as always signaling FE_INVALID is probably the safer option.

That sounds reasonable to me.

clang/test/AST/Interp/builtin-functions.cpp

65–73

Can you add a test using __builtin_nans to show that it results in an invalid constant expression because of the FE_INVALID signal?

[tbaeder]

clang/test/AST/Interp/builtin-functions.cpp

65–73

It doesn't currently result in an invalid constant expression in clang (both new and current interpreter). Where should that signal occur? Or do I need to check for signaling nans whenever I compute a floating value?

[aaron.ballman]

clang/test/AST/Interp/builtin-functions.cpp

65–73

That's the way that I would approach it (to check for a signaling NaN any time you need its value) and I think that's the same as the suggestion from @jcranmer-intel.

[jcranmer-intel]

clang/test/AST/Interp/builtin-functions.cpp

65–73

Most, but not all, floating-point operations with an sNaN signal an exception. The complete list of exceptions is, I believe:

• C2x 7.12.3 classification macros (e.g., isnan, fpclassify)
• totalorder, totalordermag
• fneg, fabs, copysign, "copy" (basically anything that could do an SSA copy of the value)
• conversion to/from strings, maybe (IEEE 754 has some "should"s in here)

The best place to do the checking for sNaNs is where you're handling the inputs for a function.

(As a brief aside, C++23 only discusses making FP exceptions compile-time errors for calling C library functions, not regular floating-point exceptions.)

[jcranmer-intel]

clang/test/AST/Interp/builtin-functions.cpp

65–73

not regular floating-point exceptions.

That should say "regular floating-point operations" (e.g., a + b).

[tbaeder]

Thanks for the info @jcranmer-intel.

I see that the current intepreter doesn't check this for __builtin_fmin(GCC does) either, but it does error out for e.g. __builtin_nan("") + 1 (GCC doesn't).

@aaron.ballman I'd like to handle that in a separate patch where I also handle regular floating point operations.

[tbaeder]

As for signaling vs. quiet NaNs, it seems like both GCC and Clang only check for NaNs, and don't care if it's signaling or not.

[aaron.ballman]

In D155546#4541144, @tbaeder wrote:

Thanks for the info @jcranmer-intel.

I see that the current intepreter doesn't check this for __builtin_fmin(GCC does) either, but it does error out for e.g. __builtin_nan("") + 1 (GCC doesn't).

@aaron.ballman I'd like to handle that in a separate patch where I also handle regular floating point operations.

I think that's reasonable.

In D155546#4541249, @tbaeder wrote:

As for signaling vs. quiet NaNs, it seems like both GCC and Clang only check for NaNs, and don't care if it's signaling or not.

Yeah, I'm not too surprised that we missed these kinds of "edge" cases.

[aaron.ballman]

LGTM, we can handle NaN behavior in a follow-up.

clang/lib/AST/Interp/InterpBuiltin.cpp

static bool interp__builtin_copysign(InterpState &S, CodePtr OpPC,
 
   APFloat Copy = Arg1.getAPFloat();
   Copy.copySign(Arg2.getAPFloat());
   S.Stk.push<Floating>(Floating(Copy));
 
   return true;
 }
 
+static bool interp__builtin_fmin(InterpState &S, CodePtr OpPC,
+                                 const InterpFrame *Frame, const Function *F) {
+  const Floating &LHS = getParam<Floating>(Frame, 0);
+  const Floating &RHS = getParam<Floating>(Frame, 1);
+
+  Floating Result;
+
+  // When comparing zeroes, return -0.0 if one of the zeroes is negative.
+  if (LHS.isZero() && RHS.isZero() && RHS.isNegative())
+    Result = RHS;
+  else if (LHS.isNan() || RHS < LHS)
+    Result = RHS;
+  else
+    Result = LHS;
+
+  S.Stk.push<Floating>(Result);
+  return true;
+}
+
 bool InterpretBuiltin(InterpState &S, CodePtr OpPC, const Function *F) {
   InterpFrame *Frame = S.Current;
   APValue Dummy;
 
   switch (F->getBuiltinID()) {
   case Builtin::BI__builtin_is_constant_evaluated:
     S.Stk.push<Boolean>(Boolean::from(S.inConstantContext()));
     return Ret<PT_Bool>(S, OpPC, Dummy);
bool InterpretBuiltin(InterpState &S, CodePtr OpPC, const Function *F) {
   case Builtin::BI__builtin_copysign:
   case Builtin::BI__builtin_copysignf:
   case Builtin::BI__builtin_copysignl:
   case Builtin::BI__builtin_copysignf128:
     if (interp__builtin_copysign(S, OpPC, Frame, F))
       return Ret<PT_Float>(S, OpPC, Dummy);
     break;
 
+  case Builtin::BI__builtin_fmin:
+  case Builtin::BI__builtin_fminf:
+  case Builtin::BI__builtin_fminl:
+  case Builtin::BI__builtin_fminf16:
+  case Builtin::BI__builtin_fminf128:
+    if (interp__builtin_fmin(S, OpPC, Frame, F))
+      return Ret<PT_Float>(S, OpPC, Dummy);
+    break;
 
   default:
     return false;
   }
 
   return false;
 }
 
 } // namespace interp
 } // namespace clang

clang/test/AST/Interp/builtin-functions.cpp

namespace nan {
 
   /// FIXME: This should be accepted by the current interpreter as well.
   constexpr char f2[] = {'0', 'x', 'A', 'E'}; /// No trailing 0 byte.
   constexpr double NaN7 = __builtin_nan(f2); // ref-error {{must be initialized by a constant expression}} \
                                              // expected-error {{must be initialized by a constant expression}} \
                                              // expected-note {{read of dereferenced one-past-the-end pointer}} \
                                              // expected-note {{in call to}}
 }
+
+namespace fmin {
+  constexpr float f1 = __builtin_fmin(1.0, 2.0f);
+  static_assert(f1 == 1.0f, "");
+
+  constexpr float min = __builtin_fmin(__builtin_nan(""), 1);
+  static_assert(min == 1, "");
+  constexpr float min2 = __builtin_fmin(1, __builtin_nan(""));
+  static_assert(min2 == 1, "");
+  constexpr float min3 = __builtin_fmin(__builtin_inf(), __builtin_nan(""));
+  static_assert(min3 == __builtin_inf(), "");
+}