Bug 84949 - -ffast-math bugged with respect to NaNs
Summary: -ffast-math bugged with respect to NaNs
Status: SUSPENDED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 5.4.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
: 111759 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-03-19 11:26 UTC by Christoph Lipka
Modified: 2023-10-10 13:58 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-03-19 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Christoph Lipka 2018-03-19 11:26:53 UTC
When -ffast-math is specified, NaNs are broken in _one_ of the following ways:

(A) 
std::isnan() and std::fpclassify() are broken in that they fail to identify NaNs.

-OR-

(B)
std::numeric_limits<T>::has_quiet_NaN() is broken in that it claims that NaNs are supported.


Seen with both g++ 4.8.4 on Ubuntu 14.04, as well as g++ 5.4.0 on MS Windows Subsystem for Linux, using the following set of compiler flags in both cases:

-pipe -Wno-multichar -Wno-write-strings -fno-enforce-eh-specs -Wno-non-template-friend -s -O3 -ffast-math -march=native -pthread
Comment 1 Richard Biener 2018-03-19 11:35:44 UTC
-ffast-math implies -ffinite-math-only which causes GCC to assume no inputs to
isnan/fpclassify are NaN.  As a programmer using -ffast-math you give this
guarantee to the compiler, so what you report is a pilot error?

I'm not sure if std::numeric_limits is wrong with -ffinite-math-only given
NaNs are still "supported" as in

double nan = __builtin_nan("");
int main()
{
  __builtin_printf ("%f\n", nan);
  return __builtin_isnan (nan); 
}
> g++-7 t.C  -ffast-math -O2
> ./a.out 
nan
> echo $?
0

so NaNs are there.  But in the testcase I broke the contract with the compiler
with feeding a NaN into isnan().

I guess the compiler could have as well replaced the NaN generated by
__builtin_nan("") with zero.
Comment 2 Christoph Lipka 2018-03-19 15:32:42 UTC
Having dug a bit deeper, I notice another way in which NaNs are broken in -ffinite-math-only mode:

Normally, NaNs should always compare NON-EQUAL, even when compared to itself.

In -ffinite-math-only mode, NaNs ALWAYS compare EQUAL, even when compared to a totally different value (such as, say, 0.0).


I would disagree with the claim that "NaNs are there" in -ffinite-math-only mode - de facto they're not. What is there is one or more representations that cause all sorts of undefined behaviour, which just so _happens_ to include rendering as "nan" when converted to a string.

So my point stands that std::numeric_limits<T>::has_quiet_NaN() should return false when -ffinite-math-only is active.
Comment 3 Jonathan Wakely 2018-03-19 17:21:12 UTC
(In reply to Christoph Lipka from comment #0)
> (B)
> std::numeric_limits<T>::has_quiet_NaN() is broken in that it claims that
> NaNs are supported.

Agreed. 
 
> Seen with both g++ 4.8.4 on Ubuntu 14.04, as well as g++ 5.4.0 on MS Windows
> Subsystem for Linux

Neither of those compilers is still supported by the GCC project.
Comment 4 Jonathan Wakely 2018-05-03 12:24:34 UTC
std::numeric_limits<float> defines:

      static _GLIBCXX_USE_CONSTEXPR bool has_infinity = __FLT_HAS_INFINITY__;
      static _GLIBCXX_USE_CONSTEXPR bool has_quiet_NaN = __FLT_HAS_QUIET_NAN__;
      static _GLIBCXX_USE_CONSTEXPR bool has_signaling_NaN = has_quiet_NaN;
      static _GLIBCXX_USE_CONSTEXPR float_denorm_style has_denorm
	= bool(__FLT_HAS_DENORM__) ? denorm_present : denorm_absent;
      //...
      static _GLIBCXX_USE_CONSTEXPR bool is_iec559
	= has_infinity && has_quiet_NaN && has_denorm == denorm_present;

And that seems to be the right thing to do. If the compiler tells us the type has infinities and NaNs then we expose that through std::numeric_limits.

I don't think we want the C++ library to be inconsistent with the compiler here. So maybe any change should be in the compiler not libstdc++.
Comment 5 rguenther@suse.de 2018-05-08 07:35:08 UTC
On Thu, 3 May 2018, redi at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949
> 
> --- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> std::numeric_limits<float> defines:
> 
>       static _GLIBCXX_USE_CONSTEXPR bool has_infinity = __FLT_HAS_INFINITY__;
>       static _GLIBCXX_USE_CONSTEXPR bool has_quiet_NaN = __FLT_HAS_QUIET_NAN__;
>       static _GLIBCXX_USE_CONSTEXPR bool has_signaling_NaN = has_quiet_NaN;
>       static _GLIBCXX_USE_CONSTEXPR float_denorm_style has_denorm
>         = bool(__FLT_HAS_DENORM__) ? denorm_present : denorm_absent;
>       //...
>       static _GLIBCXX_USE_CONSTEXPR bool is_iec559
>         = has_infinity && has_quiet_NaN && has_denorm == denorm_present;

^^^

note that this really can't work in a way to reflect the state
of flags like -ffinite-math-only because those can be different
_per function_.  Of course then the preprocessor macros do not
reflect reality either...

bool __attribute__((optimize("finite-math-only")))
isnan1 (double x, int &a)
{
  a = __FINITE_MATH_ONLY__;
  return __builtin_isnan(x);
}
bool __attribute__((optimize("no-finite-math-only")))
isnan2 (double x, int &a)
{
  a = __FINITE_MATH_ONLY__;
  return __builtin_isnan(x);
}
int main()
{
  int a;
  __builtin_printf ("%d %d\n", isnan1(__builtin_nan(""), a), a);
  __builtin_printf ("%d %d\n", isnan2(__builtin_nan(""), a), a);
}

> g++-7 t.c
> ./a.out 
0 0
1 0
> g++-7 t.c -ffinite-math-only
> ./a.out 
0 0
1 1

note that __FLT_HAS_QUIET_NAN__ is not affected by flags.

> And that seems to be the right thing to do. If the compiler tells us the type
> has infinities and NaNs then we expose that through std::numeric_limits.
> 
> I don't think we want the C++ library to be inconsistent with the compiler
> here. So maybe any change should be in the compiler not libstdc++.

So my points in comment#1 still stand.  It might be unfortunate
(even more so since you can't actually rely on __FINITE_MATH_ONLY__)
but it's not likely to change (people have repeatedly requested
that at least isnan() continues to return true for NaNs for example).
Comment 6 Matthias Kretz (Vir) 2018-12-11 11:36:52 UTC
I'd like to make a case for numeric_limits<floating-point>::is_iec559 to follow __STDC_IEC_559__. I.e. the following patch:

--- a/libstdc++-v3/include/std/limits
+++ b/libstdc++-v3/include/std/limits
@@ -1649,7 +1649,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       denorm_min() _GLIBCXX_USE_NOEXCEPT { return __FLT_DENORM_MIN__; }
 
       static _GLIBCXX_USE_CONSTEXPR bool is_iec559
+#ifdef __STDC_IEC_559__
   = has_infinity && has_quiet_NaN && has_denorm == denorm_present;
+#else
+  = false;
+#endif
       static _GLIBCXX_USE_CONSTEXPR bool is_bounded = true;
       static _GLIBCXX_USE_CONSTEXPR bool is_modulo = false;
 
@@ -1724,7 +1728,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       denorm_min() _GLIBCXX_USE_NOEXCEPT { return __DBL_DENORM_MIN__; }
 
       static _GLIBCXX_USE_CONSTEXPR bool is_iec559
+#ifdef __STDC_IEC_559__
   = has_infinity && has_quiet_NaN && has_denorm == denorm_present;
+#else
+  = false;
+#endif
       static _GLIBCXX_USE_CONSTEXPR bool is_bounded = true;
       static _GLIBCXX_USE_CONSTEXPR bool is_modulo = false;
 
@@ -1799,7 +1807,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       denorm_min() _GLIBCXX_USE_NOEXCEPT { return __LDBL_DENORM_MIN__; }
 
       static _GLIBCXX_USE_CONSTEXPR bool is_iec559
+#ifdef __STDC_IEC_559__
   = has_infinity && has_quiet_NaN && has_denorm == denorm_present;
+#else
+  = false;
+#endif
       static _GLIBCXX_USE_CONSTEXPR bool is_bounded = true;
       static _GLIBCXX_USE_CONSTEXPR bool is_modulo = false;


The __STDC_IEC_559__ macro is not defined when -ffast-math is active. is_iec559 requires "true if and only if the type adheres to ISO/IEC/IEEE 60559". I don't have IEC559 at hand, but I believe assuming no NaN, inf, or -0 can occur makes the floating point types not adhere to IEC559.
And IIUC, if __STDC_IEC_559__ is defined, then has_infinity, has_quiet_NaN and has_denorm must all be true. So
+#ifdef __STDC_IEC_559__
+  = true;
+#else
+  = false;
+#endif
should be correct.
Comment 7 Matthias Kretz (Vir) 2018-12-11 11:41:10 UTC
Example showing the discrepancy: https://godbolt.org/z/D15m71

Also PR83875 is relevant wrt. giving different answers depending on function attributes.
Comment 8 Matthias Kretz (Vir) 2020-09-18 07:09:10 UTC
I've been doing a lot of research into the numeric_limits intent/meaning recently. I also implemented and used alternative interpretations of "has NaN" and "is IEC559". My conclusion: std::numeric_limits means "has NaN bitpattern" and "has IEC559 bit layout" not "has NaNs with NaN behavior" and "has IEC559 behavior". The former are still useful even if the latter don't hold. The C++ standard could be clearer on this matter though.
Thus, it seems the status quo is working as intended. It's just that we're missing a standard interface to ask for behavior conformance.

Any progress on this issue must go via WG21.

Besides: my patch in #6 was not accepted (ABI concerns: specifically ABI breakage from using different -f... flags), so should this PR be closed?
Comment 9 Richard Biener 2021-09-21 06:41:45 UTC
I think this is a good bug to dup to since it contains good information, so lets suspend it.
Comment 10 Andrew Pinski 2023-10-10 13:58:38 UTC
*** Bug 111759 has been marked as a duplicate of this bug. ***