[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Timm Bäder via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc14c34de4571: [clang][Interp] Implement __builtin_fmin 
(authored by tbaeder).

Changed prior to commit:
  https://reviews.llvm.org/D155546?vs=544691=545228#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155546

Files:
  clang/lib/AST/Interp/InterpBuiltin.cpp
  clang/test/AST/Interp/builtin-functions.cpp


Index: clang/test/AST/Interp/builtin-functions.cpp
===
--- clang/test/AST/Interp/builtin-functions.cpp
+++ clang/test/AST/Interp/builtin-functions.cpp
@@ -60,3 +60,15 @@
  // 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(), "");
+}
Index: clang/lib/AST/Interp/InterpBuiltin.cpp
===
--- clang/lib/AST/Interp/InterpBuiltin.cpp
+++ clang/lib/AST/Interp/InterpBuiltin.cpp
@@ -143,6 +143,25 @@
   return true;
 }
 
+static bool interp__builtin_fmin(InterpState , CodePtr OpPC,
+ const InterpFrame *Frame, const Function *F) {
+  const Floating  = getParam(Frame, 0);
+  const Floating  = getParam(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(Result);
+  return true;
+}
+
 bool InterpretBuiltin(InterpState , CodePtr OpPC, const Function *F) {
   InterpFrame *Frame = S.Current;
   APValue Dummy;
@@ -195,6 +214,15 @@
   return Ret(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(S, OpPC, Dummy);
+break;
+
   default:
 return false;
   }


Index: clang/test/AST/Interp/builtin-functions.cpp
===
--- clang/test/AST/Interp/builtin-functions.cpp
+++ clang/test/AST/Interp/builtin-functions.cpp
@@ -60,3 +60,15 @@
  // 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(), "");
+}
Index: clang/lib/AST/Interp/InterpBuiltin.cpp
===
--- clang/lib/AST/Interp/InterpBuiltin.cpp
+++ clang/lib/AST/Interp/InterpBuiltin.cpp
@@ -143,6 +143,25 @@
   return true;
 }
 
+static bool interp__builtin_fmin(InterpState , CodePtr OpPC,
+ const InterpFrame *Frame, const Function *F) {
+  const Floating  = getParam(Frame, 0);
+  const Floating  = getParam(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(Result);
+  return true;
+}
+
 bool InterpretBuiltin(InterpState , CodePtr OpPC, const Function *F) {
   InterpFrame *Frame = S.Current;
   APValue Dummy;
@@ -195,6 +214,15 @@
   return Ret(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(S, OpPC, Dummy);
+break;
+
   default:
 return false;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

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


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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

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.


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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

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.


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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Joshua Cranmer via Phabricator via cfe-commits]

jcranmer-intel added inline comments.



Comment at: clang/test/AST/Interp/builtin-functions.cpp:65-73
+  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, "");

aaron.ballman wrote:
> jcranmer-intel wrote:
> > tbaeder wrote:
> > > aaron.ballman wrote:
> > > > Can you add a test using `__builtin_nans` to show that it results in an 
> > > > invalid constant expression because of the `FE_INVALID` signal?
> > > 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?
> > 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.)
> 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.
> not regular floating-point exceptions.

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


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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Joshua Cranmer via Phabricator via cfe-commits]

jcranmer-intel added inline comments.



Comment at: clang/test/AST/Interp/builtin-functions.cpp:65-73
+  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, "");

tbaeder wrote:
> aaron.ballman wrote:
> > Can you add a test using `__builtin_nans` to show that it results in an 
> > invalid constant expression because of the `FE_INVALID` signal?
> 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?
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.)


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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/AST/Interp/builtin-functions.cpp:65-73
+  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, "");

tbaeder wrote:
> aaron.ballman wrote:
> > Can you add a test using `__builtin_nans` to show that it results in an 
> > invalid constant expression because of the `FE_INVALID` signal?
> 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?
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.


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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/test/AST/Interp/builtin-functions.cpp:65-73
+  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, "");

aaron.ballman wrote:
> Can you add a test using `__builtin_nans` to show that it results in an 
> invalid constant expression because of the `FE_INVALID` signal?
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?


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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D155546#4532839 , @jcranmer-intel 
wrote:

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

That sounds reasonable to me.




Comment at: clang/test/AST/Interp/builtin-functions.cpp:65-73
+  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, "");

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


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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 544691.

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

https://reviews.llvm.org/D155546

Files:
  clang/lib/AST/Interp/InterpBuiltin.cpp
  clang/test/AST/Interp/builtin-functions.cpp


Index: clang/test/AST/Interp/builtin-functions.cpp
===
--- clang/test/AST/Interp/builtin-functions.cpp
+++ clang/test/AST/Interp/builtin-functions.cpp
@@ -60,3 +60,15 @@
  // 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(), "");
+}
Index: clang/lib/AST/Interp/InterpBuiltin.cpp
===
--- clang/lib/AST/Interp/InterpBuiltin.cpp
+++ clang/lib/AST/Interp/InterpBuiltin.cpp
@@ -143,6 +143,25 @@
   return true;
 }
 
+static bool interp__builtin_fmin(InterpState , CodePtr OpPC,
+ const InterpFrame *Frame, const Function *F) {
+  const Floating  = getParam(Frame, 0);
+  const Floating  = getParam(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(Result);
+  return true;
+}
+
 bool InterpretBuiltin(InterpState , CodePtr OpPC, const Function *F) {
   InterpFrame *Frame = S.Current;
   APValue Dummy;
@@ -195,6 +214,14 @@
   return Ret(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(S, OpPC, Dummy);
+break;
 
   default:
 return false;


Index: clang/test/AST/Interp/builtin-functions.cpp
===
--- clang/test/AST/Interp/builtin-functions.cpp
+++ clang/test/AST/Interp/builtin-functions.cpp
@@ -60,3 +60,15 @@
  // 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(), "");
+}
Index: clang/lib/AST/Interp/InterpBuiltin.cpp
===
--- clang/lib/AST/Interp/InterpBuiltin.cpp
+++ clang/lib/AST/Interp/InterpBuiltin.cpp
@@ -143,6 +143,25 @@
   return true;
 }
 
+static bool interp__builtin_fmin(InterpState , CodePtr OpPC,
+ const InterpFrame *Frame, const Function *F) {
+  const Floating  = getParam(Frame, 0);
+  const Floating  = getParam(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(Result);
+  return true;
+}
+
 bool InterpretBuiltin(InterpState , CodePtr OpPC, const Function *F) {
   InterpFrame *Frame = S.Current;
   APValue Dummy;
@@ -195,6 +214,14 @@
   return Ret(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(S, OpPC, Dummy);
+break;
 
   default:
 return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Joshua Cranmer via Phabricator via cfe-commits]

jcranmer-intel added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: jcranmer-intel, hubert.reinterpretcast, rsmith.
aaron.ballman added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

`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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

Looks fine but where are the tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155546: [clang][Interp] Implement __builtin_fmin

[Timm Bäder via Phabricator via cfe-commits]

tbaeder created this revision.
tbaeder added reviewers: aaron.ballman, erichkeane, shafik, cor3ntin.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155546

Files:
  clang/lib/AST/Interp/InterpBuiltin.cpp


Index: clang/lib/AST/Interp/InterpBuiltin.cpp
===
--- clang/lib/AST/Interp/InterpBuiltin.cpp
+++ clang/lib/AST/Interp/InterpBuiltin.cpp
@@ -139,6 +139,25 @@
   return true;
 }
 
+static bool interp__builtin_fmin(InterpState , CodePtr OpPC,
+ const InterpFrame *Frame, const Function *F) {
+  const Floating  = getParam(Frame, 0);
+  const Floating  = getParam(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(Result);
+  return true;
+}
+
 bool InterpretBuiltin(InterpState , CodePtr OpPC, const Function *F) {
   InterpFrame *Frame = S.Current;
   APValue Dummy;
@@ -191,6 +210,14 @@
   return Ret(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(S, OpPC, Dummy);
+break;
 
   default:
 return false;


Index: clang/lib/AST/Interp/InterpBuiltin.cpp
===
--- clang/lib/AST/Interp/InterpBuiltin.cpp
+++ clang/lib/AST/Interp/InterpBuiltin.cpp
@@ -139,6 +139,25 @@
   return true;
 }
 
+static bool interp__builtin_fmin(InterpState , CodePtr OpPC,
+ const InterpFrame *Frame, const Function *F) {
+  const Floating  = getParam(Frame, 0);
+  const Floating  = getParam(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(Result);
+  return true;
+}
+
 bool InterpretBuiltin(InterpState , CodePtr OpPC, const Function *F) {
   InterpFrame *Frame = S.Current;
   APValue Dummy;
@@ -191,6 +210,14 @@
   return Ret(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(S, OpPC, Dummy);
+break;
 
   default:
 return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits