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