* [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] @ 2024-05-07 13:49 Jonathan Wakely 2024-05-07 13:57 ` Jeff Law 2024-05-07 15:36 ` Andreas Schwab 0 siblings, 2 replies; 13+ messages in thread From: Jonathan Wakely @ 2024-05-07 13:49 UTC (permalink / raw) To: libstdc++, gcc-patches 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(+) 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& -- 2.44.0 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] 2024-05-07 13:49 [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] Jonathan Wakely @ 2024-05-07 13:57 ` Jeff Law 2024-05-07 14:06 ` Jonathan Wakely 2024-05-07 15:36 ` Andreas Schwab 1 sibling, 1 reply; 13+ messages in thread From: Jeff Law @ 2024-05-07 13:57 UTC (permalink / raw) To: Jonathan Wakely, libstdc++, gcc-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] 2024-05-07 13:57 ` Jeff Law @ 2024-05-07 14:06 ` Jonathan Wakely 2024-05-07 14:11 ` Jonathan Wakely 2024-05-07 15:25 ` Jeff Law 0 siblings, 2 replies; 13+ messages in thread From: Jonathan Wakely @ 2024-05-07 14:06 UTC (permalink / raw) To: Jeff Law; +Cc: libstdc++, gcc-patches 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] 2024-05-07 14:06 ` Jonathan Wakely @ 2024-05-07 14:11 ` Jonathan Wakely 2024-05-07 16:24 ` Palmer Dabbelt 2024-05-10 10:58 ` Jonathan Wakely 2024-05-07 15:25 ` Jeff Law 1 sibling, 2 replies; 13+ messages in thread From: Jonathan Wakely @ 2024-05-07 14:11 UTC (permalink / raw) To: Jeff Law; +Cc: libstdc++, gcc-patches 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] 2024-05-07 14:11 ` Jonathan Wakely @ 2024-05-07 16:24 ` Palmer Dabbelt 2024-05-10 10:58 ` Jonathan Wakely 1 sibling, 0 replies; 13+ messages in thread From: Palmer Dabbelt @ 2024-05-07 16:24 UTC (permalink / raw) To: jwakely, fantasquex, adhemerval.zanella Cc: jeffreyalaw, libstdc++, gcc-patches [+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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] 2024-05-07 14:11 ` Jonathan Wakely 2024-05-07 16:24 ` Palmer Dabbelt @ 2024-05-10 10:58 ` Jonathan Wakely 1 sibling, 0 replies; 13+ messages in thread From: Jonathan Wakely @ 2024-05-10 10:58 UTC (permalink / raw) To: Jeff Law; +Cc: libstdc++, gcc-patches 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] 2024-05-07 14:06 ` Jonathan Wakely 2024-05-07 14:11 ` Jonathan Wakely @ 2024-05-07 15:25 ` Jeff Law 1 sibling, 0 replies; 13+ messages in thread From: Jeff Law @ 2024-05-07 15:25 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] 2024-05-07 13:49 [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] Jonathan Wakely 2024-05-07 13:57 ` Jeff Law @ 2024-05-07 15:36 ` Andreas Schwab 2024-05-07 16:31 ` Jeff Law 1 sibling, 1 reply; 13+ messages in thread From: Andreas Schwab @ 2024-05-07 15:36 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches 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>? -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] 2024-05-07 15:36 ` Andreas Schwab @ 2024-05-07 16:31 ` Jeff Law 2024-05-07 16:39 ` Jonathan Wakely 0 siblings, 1 reply; 13+ messages in thread From: Jeff Law @ 2024-05-07 16:31 UTC (permalink / raw) To: Andreas Schwab, Jonathan Wakely; +Cc: libstdc++, gcc-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] 2024-05-07 16:31 ` Jeff Law @ 2024-05-07 16:39 ` Jonathan Wakely 2024-05-07 16:45 ` Jonathan Wakely 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Wakely @ 2024-05-07 16:39 UTC (permalink / raw) To: Jeff Law; +Cc: Andreas Schwab, Jonathan Wakely, libstdc++, gcc-patches 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] 2024-05-07 16:39 ` Jonathan Wakely @ 2024-05-07 16:45 ` Jonathan Wakely 2024-05-08 10:32 ` Andrew Waterman 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Wakely @ 2024-05-07 16:45 UTC (permalink / raw) To: Jeff Law; +Cc: Andreas Schwab, Jonathan Wakely, libstdc++, gcc-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] 2024-05-07 16:45 ` Jonathan Wakely @ 2024-05-08 10:32 ` Andrew Waterman 2024-05-08 10:40 ` Jonathan Wakely 0 siblings, 1 reply; 13+ messages in thread From: Andrew Waterman @ 2024-05-08 10:32 UTC (permalink / raw) To: Jonathan Wakely Cc: Jeff Law, Andreas Schwab, Jonathan Wakely, libstdc++, gcc-patches 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] 2024-05-08 10:32 ` Andrew Waterman @ 2024-05-08 10:40 ` Jonathan Wakely 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Wakely @ 2024-05-08 10:40 UTC (permalink / raw) To: Andrew Waterman Cc: Jonathan Wakely, Jeff Law, Andreas Schwab, libstdc++, gcc-patches 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. > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-05-10 10:59 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-05-07 13:49 [PATCH][risc-v] libstdc++: Preserve signbit of nan when converting float to double [PR113578] Jonathan Wakely 2024-05-07 13:57 ` Jeff Law 2024-05-07 14:06 ` Jonathan Wakely 2024-05-07 14:11 ` Jonathan Wakely 2024-05-07 16:24 ` Palmer Dabbelt 2024-05-10 10:58 ` Jonathan Wakely 2024-05-07 15:25 ` Jeff Law 2024-05-07 15:36 ` Andreas Schwab 2024-05-07 16:31 ` Jeff Law 2024-05-07 16:39 ` Jonathan Wakely 2024-05-07 16:45 ` Jonathan Wakely 2024-05-08 10:32 ` Andrew Waterman 2024-05-08 10:40 ` Jonathan Wakely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).