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

[Palmer Dabbelt <palmer@dabbelt.com>]

[+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.