[Bug libquadmath/109758] New: quadmath abs

[Bug libquadmath/109758] New: quadmath abs

[g.peterhoff@t-online.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

            Bug ID: 109758
           Summary: quadmath abs
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libquadmath
          Assignee: unassigned at gcc dot gnu.org
          Reporter: g.peterhoff@t-online.de
  Target Milestone: ---

Hello gcc-team,
Problem:
#include <boost/cstdfloat.hpp>
#include <iostream>
#include <limits>
#include <cmath>

using T = __float128;

int main()
{
    const T 
        neg_nan_v = -std::numeric_limits<T>::quiet_NaN();

    std::cout << neg_nan_v << std::endl;

    std::cout << "std::abs " << std::abs(neg_nan_v) << std::endl;
    std::cout << "fabsq " << fabsq(neg_nan_v) << std::endl;
    std::cout << "builtin " << __builtin_fabsf128(neg_nan_v) << std::endl;
}

-nan
std::abs -nan
fabsq nan
builtin nan

The problem can be found in bits/std_abs.h:
#if !defined(__STRICT_ANSI__) && defined(_GLIBCXX_USE_FLOAT128)
  __extension__ inline _GLIBCXX_CONSTEXPR
  __float128
  abs(__float128 __x)
  {
   return __x < 0 ? -__x : __x;
  }
#endif

Is this actually correct? If I compile with -U__STRICT_ANSI__ or remove/comment
abs from bits/std_abs.h abs falls back to fabsq, which then also works.
With std::abs(float/double/...) this problem does not occur.

Wouldn't it make sense in principle to also provide a C++ header
(quadmath.hpp)?
#include <quadmath.h>
namespace std
{
math-functions
to_string/to_wstring
to_chars/from_chars
operator<<
operator>>
...
}

thx
Gero

[Bug libstdc++/109758] quadmath abs

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
libstdc++ doesn't depend on libquadmath and the __float128 support is there
very limited.
Use std::float128_t instead (in GCC 13.1)?

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-05-06
            Summary|quadmath abs                |std::abs(__float128)
                   |                            |doesn't support NaN

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
We should fix std::abs(__float128) to handle NaN but I don't think we want to
add __float128 support for everything in <cmath> and elsewhere in std.

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[g.peterhoff@t-online.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

--- Comment #3 from g.peterhoff@t-online.de ---
>> libstdc++ doesn't depend on libquadmath and the __float128 support is there very limited.
Yes, exactly. There should be nothing of quadmath in the std implementations of
C/C++. But in bits/std_abs.h this is the case.

>> Use std::float128_t instead (in GCC 13.1)?
std::float128_t can only be used from C++23 on, but quadmath can also be used
with older standard/compiler versions.

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to g.peterhoff from comment #3)
> >> libstdc++ doesn't depend on libquadmath and the __float128 support is there very limited.
> Yes, exactly. There should be nothing of quadmath in the std implementations
> of C/C++. But in bits/std_abs.h this is the case.

I'm not sure what you mean. There is nothing from libquadmath there, what are
you referring to as "quadmath"? Just any use of 128-bit floating-point types?


> >> Use std::float128_t instead (in GCC 13.1)?
> std::float128_t can only be used from C++23 on, but quadmath can also be
> used with older standard/compiler versions.

Again, what do you mean by "quadmath"?

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[g.peterhoff@t-online.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

--- Comment #5 from g.peterhoff@t-online.de ---
>> Again, what do you mean by "quadmath"?

__float128 https://github.com/gcc-mirror/gcc/tree/master/libquadmath
This is not to be confused with C++23 std::float128_t.

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to g.peterhoff from comment #5)
> >> Again, what do you mean by "quadmath"?
> 
> __float128 https://github.com/gcc-mirror/gcc/tree/master/libquadmath

Well that's even more confusing/confused :-)

Those are two separate things. __float128 is a built-in type provided by the
compiler, and libquadmath is a library. The __float128 type can be used
completely independently of libquadmath.

Libstdc++ has some support for the __float128 type, but it does not use
libquadmath.

There is no "quadmath" in <bits/std_abs.h> or anywhere else in libstdc++, just
plain uses of the __float128 type.

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[g.peterhoff@t-online.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

--- Comment #7 from g.peterhoff@t-online.de ---
1) Can you please still submit a proposal to the STD/ISO committee so that abs
(besides copysign/signbit) ALWAYS works ?
2) What do you think about my proposal for a C++ interface quadmath.hpp ?

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
What does the committee have to do with the GCC implementation?

I guess what we could do is:
--- libstdc++-v3/include/bits/std_abs.h 2023-01-16 11:52:16.917721774 +0100
+++ libstdc++-v3/include/bits/std_abs.h 2023-05-07 12:01:03.716627026 +0200
@@ -135,7 +135,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   __extension__ inline _GLIBCXX_CONSTEXPR
   __float128
   abs(__float128 __x)
-  { return __x < 0 ? -__x : __x; }
+  {
+#if __has_builtin(__builtin_fabsf128)
+    return  __builtin_fabsf128(__x);
+#else
+    return __x < 0 ? -__x : __x;
+#endif
+  }
 #endif

 _GLIBCXX_END_NAMESPACE_VERSION
If the builtin isn't supported, perhaps we could use bit_cast if supported to
toggle the sign bit if we know where it is, but if that isn't supported either,
there is nothing we can do, as the function needs to be constexpr.

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to g.peterhoff from comment #7)
> 1) Can you please still submit a proposal to the STD/ISO committee so that
> abs (besides copysign/signbit) ALWAYS works ?

What does that even mean?

Should abs("") work?

Why should the committee say anything about the behaviour of a non-standard
__float128 type?

So no, I won't submit anything related to this to the committee. This is just a
bug in a libstdc++ function, like I already said.


> 2) What do you think about my proposal for a C++ interface quadmath.hpp ?

I already said we don't want to do that. If you want full support in the
standard library, use standard types.

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #8)
> If the builtin isn't supported, perhaps we could use bit_cast if supported
> to toggle the sign bit if we know where it is, but if that isn't supported
> either, there is nothing we can do, as the function needs to be constexpr.

The implementation of fabsq in libquadmath just assumes the sign bit is the MSB
so it seems reasonable to do that here too. I already have a patch doing that
which I'll finish testing when back at work.

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
We'd need to bitcast with
  struct
  {
#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
    uint64_t __high;
    uint64_t __low;
#else
    uint64_t __low;
    uint64_t __high;
#endif
  } __words64;
};
and back (we can't use __uint128_t unconditionally).

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:af595613acbd9863198ae69c7b1c9e856bca9e4f

commit r14-700-gaf595613acbd9863198ae69c7b1c9e856bca9e4f
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed May 10 12:20:58 2023 +0100

    libstdc++: Fix std::abs(__float128) for -NaN and -0.0 [PR109758]

    The current implementation of this non-standard overload of std::abs
    incorrectly returns a negative value for negative NaNs and negative
    zero, because x < 0 is false in both cases.

    Use fabsl(long double) or fabsf128(_Float128) if those do the right
    thing.  Otherwise, use __builtin_signbit(x) instead of x < 0 to detect
    negative inputs. This assumes that __builtin_signbit handles __float128
    correctly, but that seems to be true for all of GCC, clang and icc.

    libstdc++-v3/ChangeLog:

            PR libstdc++/109758
            * include/bits/std_abs.h (abs(__float128)): Handle negative NaN
            and negative zero correctly.
            * testsuite/26_numerics/headers/cmath/109758.cc: New test.

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.2
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for 13.2 (but could be backported later).

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.2                        |14.0

--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #13)
> Fixed for 13.2 (but could be backported later).

Oops, I meant 14, it's not backported to gcc-13.

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:099d469df67d8454aa393d036f4791912364bd4d

commit r13-7406-g099d469df67d8454aa393d036f4791912364bd4d
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed May 10 12:20:58 2023 +0100

    libstdc++: Fix std::abs(__float128) for -NaN and -0.0 [PR109758]

    The current implementation of this non-standard overload of std::abs
    incorrectly returns a negative value for negative NaNs and negative
    zero, because x < 0 is false in both cases.

    Use fabsl(long double) or fabsf128(_Float128) if those do the right
    thing.  Otherwise, use __builtin_signbit(x) instead of x < 0 to detect
    negative inputs. This assumes that __builtin_signbit handles __float128
    correctly, but that seems to be true for all of GCC, clang and icc.

    libstdc++-v3/ChangeLog:

            PR libstdc++/109758
            * include/bits/std_abs.h (abs(__float128)): Handle negative NaN
            and negative zero correctly.
            * testsuite/26_numerics/headers/cmath/109758.cc: New test.

    (cherry picked from commit af595613acbd9863198ae69c7b1c9e856bca9e4f)

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|14.0                        |13.2

--- Comment #16 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Backported for 13.2 now

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

--- Comment #17 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:9f381ebb7211c1359f5de87760148096fcba3357

commit r12-10244-g9f381ebb7211c1359f5de87760148096fcba3357
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed May 10 12:20:58 2023 +0100

    libstdc++: Fix std::abs(__float128) for -NaN and -0.0 [PR109758]

    The current implementation of this non-standard overload of std::abs
    incorrectly returns a negative value for negative NaNs and negative
    zero, because x < 0 is false in both cases.

    Use fabsl(long double) or fabsf128(_Float128) if those do the right
    thing.  Otherwise, use __builtin_signbit(x) instead of x < 0 to detect
    negative inputs. This assumes that __builtin_signbit handles __float128
    correctly, but that seems to be true for all of GCC, clang and icc.

    libstdc++-v3/ChangeLog:

            PR libstdc++/109758
            * include/bits/std_abs.h (abs(__float128)): Handle negative NaN
            and negative zero correctly.
            * testsuite/26_numerics/headers/cmath/109758.cc: New test.

    (cherry picked from commit af595613acbd9863198ae69c7b1c9e856bca9e4f)

[Bug libstdc++/109758] std::abs(__float128) doesn't support NaN

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109758

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.2                        |12.4

--- Comment #18 from Jonathan Wakely <redi at gcc dot gnu.org> ---
And 12.4