diff mbox series

[risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578]

Message ID 20240507135019.3821031-1-jwakely@redhat.com
State New
Headers show
Series [risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] | expand

Commit Message

Jonathan Wakely May 7, 2024, 1:49 p.m. UTC 
Do we want this change for RISC-V, to fix PR113578?

I haven't tested it on RISC-V, only on x86_64-linux (where it doesn't do
anything).

-- >8 --

libstdc++-v3/ChangeLog:

	PR libstdc++/113578
	* include/std/ostream (operator<<(basic_ostream&, float)):
	Restore signbit after converting to double.
---
 libstdc++-v3/include/std/ostream | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Jeff Law May 7, 2024, 1:57 p.m. UTC | #1 
On 5/7/24 7:49 AM, Jonathan Wakely wrote:
> Do we want this change for RISC-V, to fix PR113578?
> 
> I haven't tested it on RISC-V, only on x86_64-linux (where it doesn't do
> anything).
> 
> -- >8 --
> 
> libstdc++-v3/ChangeLog:
> 
> 	PR libstdc++/113578
> 	* include/std/ostream (operator<<(basic_ostream&, float)):
> 	Restore signbit after converting to double.
No strong opinion.     One could argue that the existence of a 
conditional like that inherently implies the generic code is dependent 
on specific processor behavior which probably is unwise.  But again, no 
strong opinion.

jeff
Jonathan Wakely May 7, 2024, 2:06 p.m. UTC | #2 
On Tue, 7 May 2024 at 14:57, Jeff Law wrote:
>
>
>
> On 5/7/24 7:49 AM, Jonathan Wakely wrote:
> > Do we want this change for RISC-V, to fix PR113578?
> >
> > I haven't tested it on RISC-V, only on x86_64-linux (where it doesn't do
> > anything).
> >
> > -- >8 --
> >
> > libstdc++-v3/ChangeLog:
> >
> >       PR libstdc++/113578
> >       * include/std/ostream (operator<<(basic_ostream&, float)):
> >       Restore signbit after converting to double.
> No strong opinion.     One could argue that the existence of a
> conditional like that inherently implies the generic code is dependent
> on specific processor behavior which probably is unwise.  But again, no
> strong opinion.

Yes, but I'm not aware of any other processors that lose the signbit
like this, so in practice it's always worked fine to cast the float to
double.
Jonathan Wakely May 7, 2024, 2:11 p.m. UTC | #3 
On Tue, 7 May 2024 at 15:06, Jonathan Wakely wrote:
>
> On Tue, 7 May 2024 at 14:57, Jeff Law wrote:
> >
> >
> >
> > On 5/7/24 7:49 AM, Jonathan Wakely wrote:
> > > Do we want this change for RISC-V, to fix PR113578?
> > >
> > > I haven't tested it on RISC-V, only on x86_64-linux (where it doesn't do
> > > anything).
> > >
> > > -- >8 --
> > >
> > > libstdc++-v3/ChangeLog:
> > >
> > >       PR libstdc++/113578
> > >       * include/std/ostream (operator<<(basic_ostream&, float)):
> > >       Restore signbit after converting to double.
> > No strong opinion.     One could argue that the existence of a
> > conditional like that inherently implies the generic code is dependent
> > on specific processor behavior which probably is unwise.  But again, no
> > strong opinion.
>
> Yes, but I'm not aware of any other processors that lose the signbit
> like this, so in practice it's always worked fine to cast the float to
> double.

The similar glibc fix for strfrom is specific to RISC-V:
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=0cc0033ef19bd3378445c2b851e53d7255cb1b1e

My patch uses copysign unconditionally, to avoid branching on isnan. I
don't know if that's the right choice.
Jeff Law May 7, 2024, 3:25 p.m. UTC | #4 
On 5/7/24 8:06 AM, Jonathan Wakely wrote:
> On Tue, 7 May 2024 at 14:57, Jeff Law wrote:
>>
>>
>>
>> On 5/7/24 7:49 AM, Jonathan Wakely wrote:
>>> Do we want this change for RISC-V, to fix PR113578?
>>>
>>> I haven't tested it on RISC-V, only on x86_64-linux (where it doesn't do
>>> anything).
>>>
>>> -- >8 --
>>>
>>> libstdc++-v3/ChangeLog:
>>>
>>>        PR libstdc++/113578
>>>        * include/std/ostream (operator<<(basic_ostream&, float)):
>>>        Restore signbit after converting to double.
>> No strong opinion.     One could argue that the existence of a
>> conditional like that inherently implies the generic code is dependent
>> on specific processor behavior which probably is unwise.  But again, no
>> strong opinion.
> 
> Yes, but I'm not aware of any other processors that lose the signbit
> like this, so in practice it's always worked fine to cast the float to
> double.
We kicked it around a bit in our meeting today and the thinking is that 
while RISC-V implementation is IEEE 754 compliant, it does differ from 
other implementations.

So do we want to be stuck explaining this corner of IEEE 754 compliance 
to end users?  If not, then we probably want to go with your fix.

Similarly if there's a reasonable chance a standard higher in the 
software stacks mandates the behavior that everyone else has, then we'd 
want to go with your fix as well.

So after further review, I'd lean towards fixing this in libstdc++ by 
whatever means you think is cleanest.

jeff
Andreas Schwab May 7, 2024, 3:36 p.m. UTC | #5 
On Mai 07 2024, Jonathan Wakely wrote:

> +#ifdef __riscv
> +	return _M_insert(__builtin_copysign((double)__f,
> +					    (double)-__builtin_signbit(__f));

Should this use static_cast<double>?
Palmer Dabbelt May 7, 2024, 4:24 p.m. UTC | #6 
[+Adhemerval and Letu, who handled the glibc side of things, in case 
they have any more context.]

On Tue, 07 May 2024 07:11:08 PDT (-0700), jwakely@redhat.com wrote:
> On Tue, 7 May 2024 at 15:06, Jonathan Wakely wrote:
>>
>> On Tue, 7 May 2024 at 14:57, Jeff Law wrote:
>> >
>> >
>> >
>> > On 5/7/24 7:49 AM, Jonathan Wakely wrote:
>> > > Do we want this change for RISC-V, to fix PR113578?
>> > >
>> > > I haven't tested it on RISC-V, only on x86_64-linux (where it doesn't do
>> > > anything).
>> > >
>> > > -- >8 --
>> > >
>> > > libstdc++-v3/ChangeLog:
>> > >
>> > >       PR libstdc++/113578
>> > >       * include/std/ostream (operator<<(basic_ostream&, float)):
>> > >       Restore signbit after converting to double.
>> > No strong opinion.     One could argue that the existence of a
>> > conditional like that inherently implies the generic code is dependent
>> > on specific processor behavior which probably is unwise.  But again, no
>> > strong opinion.
>>
>> Yes, but I'm not aware of any other processors that lose the signbit
>> like this, so in practice it's always worked fine to cast the float to
>> double.
>
> The similar glibc fix for strfrom is specific to RISC-V:
> https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=0cc0033ef19bd3378445c2b851e53d7255cb1b1e

I missed the glibc patch, but IIUC the issue here is NaN 
canonicalization losing sign bits.  Presumably it's OK to lose the other 
bits?  Otherwise we'd need some different twiddling.

Either way, I think having the signed-NaN-preserving conversion is 
reasonable as it's what users are going to expect (even if it's only 
recommended by IEEE).  So

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
Acked-by: Palmer Dabbelt <palmer@rivosinc.com>

in case you want to pick it up.  I guess we should backport this too?

Maybe we should also have some sort of arch-independent 
`double __builtin_float_to_double_with_nan_sign_bits(float)` sort of 
thing?  Then we could just use it everywhere rather than duplicating 
this logic all over the place.

> My patch uses copysign unconditionally, to avoid branching on isnan. I
> don't know if that's the right choice.

IMO it's fine: it looks like this can get inlined so having the slightly 
shorter code sequence would help, and it's on an IO path so I doubt 
unconditionally executing the extra conversion instructions really 
matters.
Jeff Law May 7, 2024, 4:31 p.m. UTC | #7 
On 5/7/24 9:36 AM, Andreas Schwab wrote:
> On Mai 07 2024, Jonathan Wakely wrote:
> 
>> +#ifdef __riscv
>> +	return _M_insert(__builtin_copysign((double)__f,
>> +					    (double)-__builtin_signbit(__f));
> 
> Should this use static_cast<double>?
And it's missing a close paren.

jeff
Jonathan Wakely May 7, 2024, 4:39 p.m. UTC | #8 
On Tue, 7 May 2024 at 17:33, Jeff Law wrote:
>
>
>
> On 5/7/24 9:36 AM, Andreas Schwab wrote:
> > On Mai 07 2024, Jonathan Wakely wrote:
> >
> >> +#ifdef __riscv
> >> +    return _M_insert(__builtin_copysign((double)__f,
> >> +                                        (double)-__builtin_signbit(__f));
> >
> > Should this use static_cast<double>?

Meh. It wouldn't fit in 80 columns any more with static_cast, and it
means exactly the same thing.

> And it's missing a close paren.

Now that's more important! Thanks.
Jonathan Wakely May 7, 2024, 4:45 p.m. UTC | #9 
On Tue, 7 May 2024 at 17:39, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Tue, 7 May 2024 at 17:33, Jeff Law wrote:
> >
> >
> >
> > On 5/7/24 9:36 AM, Andreas Schwab wrote:
> > > On Mai 07 2024, Jonathan Wakely wrote:
> > >
> > >> +#ifdef __riscv
> > >> +    return _M_insert(__builtin_copysign((double)__f,
> > >> +                                        (double)-__builtin_signbit(__f));
> > >
> > > Should this use static_cast<double>?
>
> Meh. It wouldn't fit in 80 columns any more with static_cast, and it
> means exactly the same thing.
>
> > And it's missing a close paren.
>
> Now that's more important! Thanks.

Also, I've just realised that signbit might return a negative value if
the signbit is set. The spec only says it returns non-zero if the
signbit is set.

So maybe we want:

#ifdef __riscv
        const int __neg = __builtin_signbit(__f) ? -1 : 0;
        return _M_insert(__builtin_copysign(static_cast<double>(__f),
                                              static_cast<double>(__neg)));
#else
        return _M_insert(static_cast<double>(__f));
#endif
Andrew Waterman May 8, 2024, 10:32 a.m. UTC | #10 
On Tue, May 7, 2024 at 9:46 AM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Tue, 7 May 2024 at 17:39, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> >
> > On Tue, 7 May 2024 at 17:33, Jeff Law wrote:
> > >
> > >
> > >
> > > On 5/7/24 9:36 AM, Andreas Schwab wrote:
> > > > On Mai 07 2024, Jonathan Wakely wrote:
> > > >
> > > >> +#ifdef __riscv
> > > >> +    return _M_insert(__builtin_copysign((double)__f,
> > > >> +                                        (double)-__builtin_signbit(__f));
> > > >
> > > > Should this use static_cast<double>?
> >
> > Meh. It wouldn't fit in 80 columns any more with static_cast, and it
> > means exactly the same thing.
> >
> > > And it's missing a close paren.
> >
> > Now that's more important! Thanks.
>
> Also, I've just realised that signbit might return a negative value if
> the signbit is set. The spec only says it returns non-zero if the
> signbit is set.
>
> So maybe we want:
>
> #ifdef __riscv
>         const int __neg = __builtin_signbit(__f) ? -1 : 0;
>         return _M_insert(__builtin_copysign(static_cast<double>(__f),
>                                               static_cast<double>(__neg)));
> #else
>         return _M_insert(static_cast<double>(__f));
> #endif

We can avoid the signbit call altogether by taking advantage of the
fact that type-punning the float to an int, then converting that int
to a double, will produce a double with the sign of the original
value, with no exceptions raised in the process.  (I don't know
whether we're allowed to use std::bit_cast in this context, but a
type-punning memcpy would have the same effect.)

  int __i = std::bit_cast<int, float>(__f);
  return _M_insert(__builtin_copysign(static_cast<double>(__f),
static_cast<double>(__i)));

Empirically, this saves 3 instructions on RV64 or 1 instruction on
RV32 (as measured on GCC 13.2.0).  Note, I'm not trying to drag-race
on performance here.  Rather, I'm trying to minimize the extent to
which this RISC-V idiosyncrasy results in static code-size bloat.

BTW, I agree with Palmer that adding a __builtin with these semantics
seems advisable if this pattern turns out to recur frequently.
Jonathan Wakely May 8, 2024, 10:40 a.m. UTC | #11 
On Wed, 8 May 2024 at 11:33, Andrew Waterman <andrew@sifive.com> wrote:
>
> On Tue, May 7, 2024 at 9:46 AM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> >
> > On Tue, 7 May 2024 at 17:39, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> > >
> > > On Tue, 7 May 2024 at 17:33, Jeff Law wrote:
> > > >
> > > >
> > > >
> > > > On 5/7/24 9:36 AM, Andreas Schwab wrote:
> > > > > On Mai 07 2024, Jonathan Wakely wrote:
> > > > >
> > > > >> +#ifdef __riscv
> > > > >> +    return _M_insert(__builtin_copysign((double)__f,
> > > > >> +                                        (double)-__builtin_signbit(__f));
> > > > >
> > > > > Should this use static_cast<double>?
> > >
> > > Meh. It wouldn't fit in 80 columns any more with static_cast, and it
> > > means exactly the same thing.
> > >
> > > > And it's missing a close paren.
> > >
> > > Now that's more important! Thanks.
> >
> > Also, I've just realised that signbit might return a negative value if
> > the signbit is set. The spec only says it returns non-zero if the
> > signbit is set.
> >
> > So maybe we want:
> >
> > #ifdef __riscv
> >         const int __neg = __builtin_signbit(__f) ? -1 : 0;
> >         return _M_insert(__builtin_copysign(static_cast<double>(__f),
> >                                               static_cast<double>(__neg)));
> > #else
> >         return _M_insert(static_cast<double>(__f));
> > #endif
>
> We can avoid the signbit call altogether by taking advantage of the
> fact that type-punning the float to an int, then converting that int
> to a double, will produce a double with the sign of the original
> value, with no exceptions raised in the process.  (I don't know
> whether we're allowed to use std::bit_cast in this context, but a
> type-punning memcpy would have the same effect.)

I'll check when Clang added support for __builtin_bit_cast, but I
think we can use that (we can't use std::bit_cast because this needs
to compile as C++98).


>
>   int __i = std::bit_cast<int, float>(__f);
>   return _M_insert(__builtin_copysign(static_cast<double>(__f),
> static_cast<double>(__i)));
>
> Empirically, this saves 3 instructions on RV64 or 1 instruction on
> RV32 (as measured on GCC 13.2.0).  Note, I'm not trying to drag-race
> on performance here.  Rather, I'm trying to minimize the extent to
> which this RISC-V idiosyncrasy results in static code-size bloat.

Yup, this is nice, thanks.

>
> BTW, I agree with Palmer that adding a __builtin with these semantics
> seems advisable if this pattern turns out to recur frequently.
>
Jonathan Wakely May 10, 2024, 10:58 a.m. UTC | #12 
On Tue, 7 May 2024 at 15:11, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On Tue, 7 May 2024 at 15:06, Jonathan Wakely wrote:
> >
> > On Tue, 7 May 2024 at 14:57, Jeff Law wrote:
> > >
> > >
> > >
> > > On 5/7/24 7:49 AM, Jonathan Wakely wrote:
> > > > Do we want this change for RISC-V, to fix PR113578?
> > > >
> > > > I haven't tested it on RISC-V, only on x86_64-linux (where it doesn't do
> > > > anything).
> > > >
> > > > -- >8 --
> > > >
> > > > libstdc++-v3/ChangeLog:
> > > >
> > > >       PR libstdc++/113578
> > > >       * include/std/ostream (operator<<(basic_ostream&, float)):
> > > >       Restore signbit after converting to double.
> > > No strong opinion.     One could argue that the existence of a
> > > conditional like that inherently implies the generic code is dependent
> > > on specific processor behavior which probably is unwise.  But again, no
> > > strong opinion.
> >
> > Yes, but I'm not aware of any other processors that lose the signbit
> > like this, so in practice it's always worked fine to cast the float to
> > double.
>
> The similar glibc fix for strfrom is specific to RISC-V:
> https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=0cc0033ef19bd3378445c2b851e53d7255cb1b1e

Looks like I spoke too soon and the same behaviour exists on Apple M1 chips.


>
> My patch uses copysign unconditionally, to avoid branching on isnan. I
> don't know if that's the right choice.
diff mbox series

Patch

diff --git a/libstdc++-v3/include/std/ostream b/libstdc++-v3/include/std/ostream
index 8a21758d0a3..d492168ca0e 100644
--- a/libstdc++-v3/include/std/ostream
+++ b/libstdc++-v3/include/std/ostream
@@ -233,7 +233,12 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
 	// _GLIBCXX_RESOLVE_LIB_DEFECTS
 	// 117. basic_ostream uses nonexistent num_put member functions.
+#ifdef __riscv
+	return _M_insert(__builtin_copysign((double)__f,
+					    (double)-__builtin_signbit(__f));
+#else
 	return _M_insert(static_cast<double>(__f));
+#endif
       }
 
       __ostream_type&