Re: [PATCH v1] libstdc++: Default to mutex-based atomics on RISC-V

Re: [PATCH v1] libstdc++: Default to mutex-based atomics on RISC-V

[Jonathan Wakely]

On 07/04/22 11:46 -0700, Palmer Dabbelt wrote:
>The RISC-V port requires libatomic to be linked in order to resolve
>various atomic functions, which results in builds that have
>"--with-libstdcxx-lock-policy=auto" defaulting to mutex-based locks.
>Changing this to direct atomics breaks the ABI, this forces the auto
>detection mutex-based atomics on RISC-V in order to avoid a silent ABI
>break for users.
>
>See Bug 84568 for more discussion.  In the long run there may be a way
>to get the higher-performance atomics without an ABI flag day, but
>that's going to be a much more complicated operation.  We don't even
>have support for the inline atomics yet, but given that some folks have
>been discussing hacks to make these libatomic routines appear implicitly
>it seems prudent to just turn off the automatic detection for RISC-V.
>
>libstdc++-v3/ChangeLog
>
>	* acinclude.md (GLIBCXX_ENABLE_LOCK_POLICY): Force auto to mutex
>	  for RISC-V.

As documented at https://gcc.gnu.org/lists.html all patches for
libstdc++ need to go to the libstdc++ list as well as gcc-patches
(otherwise I won't see them).

We'd usually do something like:

case "${host}" in
   *-*-riscv) libstdcxx_atomic_lock_policy=mutex ;;
   *-*-*) AC_TRY_COMPILE([ ... ],,[],[])
esac

but this way is simpler. If we add more customization for other
targets we can reconsider using the 'case "${host}"' form.

So this is OK for trunk, modulo regenerating libstdc++-v3/configure
with this change. Let me know if you want me to do that regen for you
(or commit the whole thing for you).


>---
>
>I haven't even built this one, as I'm sure there's a better way to do it
>then sticking some more C code in there.
>---
> libstdc++-v3/acinclude.m4 | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4
>index f53461c85a5..945c0c66f8d 100644
>--- a/libstdc++-v3/acinclude.m4
>+++ b/libstdc++-v3/acinclude.m4
>@@ -3612,6 +3612,9 @@ AC_DEFUN([GLIBCXX_ENABLE_LOCK_POLICY], [
>     dnl Why don't we check 8-byte CAS for sparc64, where _Atomic_word is long?!
>     dnl New targets should only check for CAS for the _Atomic_word type.
>     AC_TRY_COMPILE([
>+    #if defined __riscv
>+    # error "Defaulting to mutex-based locks for ABI compatibility"
>+    #endif
>     #if ! defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2
>     # error "No 2-byte compare-and-swap"
>     #elif ! defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4

Re: [PATCH v1] libstdc++: Default to mutex-based atomics on RISC-V

[Palmer Dabbelt]

On Thu, 14 Apr 2022 08:08:17 PDT (-0700), jwakely@redhat.com wrote:
> On 07/04/22 11:46 -0700, Palmer Dabbelt wrote:
>>The RISC-V port requires libatomic to be linked in order to resolve
>>various atomic functions, which results in builds that have
>>"--with-libstdcxx-lock-policy=auto" defaulting to mutex-based locks.
>>Changing this to direct atomics breaks the ABI, this forces the auto
>>detection mutex-based atomics on RISC-V in order to avoid a silent ABI
>>break for users.
>>
>>See Bug 84568 for more discussion.  In the long run there may be a way
>>to get the higher-performance atomics without an ABI flag day, but
>>that's going to be a much more complicated operation.  We don't even
>>have support for the inline atomics yet, but given that some folks have
>>been discussing hacks to make these libatomic routines appear implicitly
>>it seems prudent to just turn off the automatic detection for RISC-V.
>>
>>libstdc++-v3/ChangeLog
>>
>>	* acinclude.md (GLIBCXX_ENABLE_LOCK_POLICY): Force auto to mutex
>>	  for RISC-V.
>
> As documented at https://gcc.gnu.org/lists.html all patches for
> libstdc++ need to go to the libstdc++ list as well as gcc-patches
> (otherwise I won't see them).

Thanks, I'll try to remember to look next time.

> We'd usually do something like:
>
> case "${host}" in
>    *-*-riscv) libstdcxx_atomic_lock_policy=mutex ;;
>    *-*-*) AC_TRY_COMPILE([ ... ],,[],[])
> esac
>
> but this way is simpler. If we add more customization for other
> targets we can reconsider using the 'case "${host}"' form.

Ya, that's kind of where I came to as well -- the proper autoconf flavor 
would scale way better, but hopefully nobody else makes this mistake and 
thus we don't need to worry about that.

I'm fine with either way (though I think we'd need a "riscv*" there, to 
match riscv32 and riscv64?), so if you want to swap it over (or have me 
re-spin this) it's no big deal on my end -- also fine, as per below, 
with you just committing this ;)

> So this is OK for trunk, modulo regenerating libstdc++-v3/configure
> with this change. Let me know if you want me to do that regen for you
> (or commit the whole thing for you).

That'd be great, thanks!  It usually takes me a while to get all the 
autotools versions lined up (we just got new machines at the office), 
that way I won't have to do so.

>
>
>>---
>>
>>I haven't even built this one, as I'm sure there's a better way to do it
>>then sticking some more C code in there.
>>---
>> libstdc++-v3/acinclude.m4 | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>>diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4
>>index f53461c85a5..945c0c66f8d 100644
>>--- a/libstdc++-v3/acinclude.m4
>>+++ b/libstdc++-v3/acinclude.m4
>>@@ -3612,6 +3612,9 @@ AC_DEFUN([GLIBCXX_ENABLE_LOCK_POLICY], [
>>     dnl Why don't we check 8-byte CAS for sparc64, where _Atomic_word is long?!
>>     dnl New targets should only check for CAS for the _Atomic_word type.
>>     AC_TRY_COMPILE([
>>+    #if defined __riscv
>>+    # error "Defaulting to mutex-based locks for ABI compatibility"
>>+    #endif
>>     #if ! defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2
>>     # error "No 2-byte compare-and-swap"
>>     #elif ! defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4

Re: [PATCH v1] libstdc++: Default to mutex-based atomics on RISC-V

[Jonathan Wakely]

On Thu, 14 Apr 2022 at 16:18, Palmer Dabbelt wrote:
>
> On Thu, 14 Apr 2022 08:08:17 PDT (-0700), jwakely@redhat.com wrote:
> > On 07/04/22 11:46 -0700, Palmer Dabbelt wrote:
> >>The RISC-V port requires libatomic to be linked in order to resolve
> >>various atomic functions, which results in builds that have
> >>"--with-libstdcxx-lock-policy=auto" defaulting to mutex-based locks.
> >>Changing this to direct atomics breaks the ABI, this forces the auto
> >>detection mutex-based atomics on RISC-V in order to avoid a silent ABI
> >>break for users.
> >>
> >>See Bug 84568 for more discussion.  In the long run there may be a way
> >>to get the higher-performance atomics without an ABI flag day, but
> >>that's going to be a much more complicated operation.  We don't even
> >>have support for the inline atomics yet, but given that some folks have
> >>been discussing hacks to make these libatomic routines appear implicitly
> >>it seems prudent to just turn off the automatic detection for RISC-V.
> >>
> >>libstdc++-v3/ChangeLog
> >>
> >>      * acinclude.md (GLIBCXX_ENABLE_LOCK_POLICY): Force auto to mutex
> >>        for RISC-V.
> >
> > As documented at https://gcc.gnu.org/lists.html all patches for
> > libstdc++ need to go to the libstdc++ list as well as gcc-patches
> > (otherwise I won't see them).
>
> Thanks, I'll try to remember to look next time.
>
> > We'd usually do something like:
> >
> > case "${host}" in
> >    *-*-riscv) libstdcxx_atomic_lock_policy=mutex ;;
> >    *-*-*) AC_TRY_COMPILE([ ... ],,[],[])
> > esac
> >
> > but this way is simpler. If we add more customization for other
> > targets we can reconsider using the 'case "${host}"' form.
>
> Ya, that's kind of where I came to as well -- the proper autoconf flavor
> would scale way better, but hopefully nobody else makes this mistake and
> thus we don't need to worry about that.

<nod>

> I'm fine with either way (though I think we'd need a "riscv*" there, to
> match riscv32 and riscv64?), so if you want to swap it over (or have me
> re-spin this) it's no big deal on my end -- also fine, as per below,
> with you just committing this ;)

Yeah, I figured *-*-riscv probably wasn't right, so that's another
reason to prefer your approach.


>
> > So this is OK for trunk, modulo regenerating libstdc++-v3/configure
> > with this change. Let me know if you want me to do that regen for you
> > (or commit the whole thing for you).
>
> That'd be great, thanks!  It usually takes me a while to get all the
> autotools versions lined up (we just got new machines at the office),
> that way I won't have to do so.

No problem, I can regen+push for you.

Re: [PATCH v1] libstdc++: Default to mutex-based atomics on RISC-V

[Palmer Dabbelt]

On Thu, 14 Apr 2022 08:22:05 PDT (-0700), jwakely@redhat.com wrote:
> On Thu, 14 Apr 2022 at 16:18, Palmer Dabbelt wrote:
>>
>> On Thu, 14 Apr 2022 08:08:17 PDT (-0700), jwakely@redhat.com wrote:
>> > On 07/04/22 11:46 -0700, Palmer Dabbelt wrote:
>> >>The RISC-V port requires libatomic to be linked in order to resolve
>> >>various atomic functions, which results in builds that have
>> >>"--with-libstdcxx-lock-policy=auto" defaulting to mutex-based locks.
>> >>Changing this to direct atomics breaks the ABI, this forces the auto
>> >>detection mutex-based atomics on RISC-V in order to avoid a silent ABI
>> >>break for users.
>> >>
>> >>See Bug 84568 for more discussion.  In the long run there may be a way
>> >>to get the higher-performance atomics without an ABI flag day, but
>> >>that's going to be a much more complicated operation.  We don't even
>> >>have support for the inline atomics yet, but given that some folks have
>> >>been discussing hacks to make these libatomic routines appear implicitly
>> >>it seems prudent to just turn off the automatic detection for RISC-V.
>> >>
>> >>libstdc++-v3/ChangeLog
>> >>
>> >>      * acinclude.md (GLIBCXX_ENABLE_LOCK_POLICY): Force auto to mutex
>> >>        for RISC-V.
>> >
>> > As documented at https://gcc.gnu.org/lists.html all patches for
>> > libstdc++ need to go to the libstdc++ list as well as gcc-patches
>> > (otherwise I won't see them).
>>
>> Thanks, I'll try to remember to look next time.
>>
>> > We'd usually do something like:
>> >
>> > case "${host}" in
>> >    *-*-riscv) libstdcxx_atomic_lock_policy=mutex ;;
>> >    *-*-*) AC_TRY_COMPILE([ ... ],,[],[])
>> > esac
>> >
>> > but this way is simpler. If we add more customization for other
>> > targets we can reconsider using the 'case "${host}"' form.
>>
>> Ya, that's kind of where I came to as well -- the proper autoconf flavor
>> would scale way better, but hopefully nobody else makes this mistake and
>> thus we don't need to worry about that.
>
> <nod>
>
>> I'm fine with either way (though I think we'd need a "riscv*" there, to
>> match riscv32 and riscv64?), so if you want to swap it over (or have me
>> re-spin this) it's no big deal on my end -- also fine, as per below,
>> with you just committing this ;)
>
> Yeah, I figured *-*-riscv probably wasn't right, so that's another
> reason to prefer your approach.
>
>
>>
>> > So this is OK for trunk, modulo regenerating libstdc++-v3/configure
>> > with this change. Let me know if you want me to do that regen for you
>> > (or commit the whole thing for you).
>>
>> That'd be great, thanks!  It usually takes me a while to get all the
>> autotools versions lined up (we just got new machines at the office),
>> that way I won't have to do so.
>
> No problem, I can regen+push for you.

Great, thanks!

Re: [PATCH v1] libstdc++: Default to mutex-based atomics on RISC-V

[Jonathan Wakely]

On Thu, 14 Apr 2022 at 16:24, Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> On Thu, 14 Apr 2022 08:22:05 PDT (-0700), jwakely@redhat.com wrote:
> > On Thu, 14 Apr 2022 at 16:18, Palmer Dabbelt wrote:
> >>
> >> On Thu, 14 Apr 2022 08:08:17 PDT (-0700), jwakely@redhat.com wrote:
> >> > On 07/04/22 11:46 -0700, Palmer Dabbelt wrote:
> >> >>The RISC-V port requires libatomic to be linked in order to resolve
> >> >>various atomic functions, which results in builds that have
> >> >>"--with-libstdcxx-lock-policy=auto" defaulting to mutex-based locks.
> >> >>Changing this to direct atomics breaks the ABI, this forces the auto
> >> >>detection mutex-based atomics on RISC-V in order to avoid a silent ABI
> >> >>break for users.
> >> >>
> >> >>See Bug 84568 for more discussion.  In the long run there may be a way
> >> >>to get the higher-performance atomics without an ABI flag day, but
> >> >>that's going to be a much more complicated operation.  We don't even
> >> >>have support for the inline atomics yet, but given that some folks have
> >> >>been discussing hacks to make these libatomic routines appear implicitly
> >> >>it seems prudent to just turn off the automatic detection for RISC-V.
> >> >>
> >> >>libstdc++-v3/ChangeLog
> >> >>
> >> >>      * acinclude.md (GLIBCXX_ENABLE_LOCK_POLICY): Force auto to mutex
> >> >>        for RISC-V.
> >> >
> >> > As documented at https://gcc.gnu.org/lists.html all patches for
> >> > libstdc++ need to go to the libstdc++ list as well as gcc-patches
> >> > (otherwise I won't see them).
> >>
> >> Thanks, I'll try to remember to look next time.
> >>
> >> > We'd usually do something like:
> >> >
> >> > case "${host}" in
> >> >    *-*-riscv) libstdcxx_atomic_lock_policy=mutex ;;
> >> >    *-*-*) AC_TRY_COMPILE([ ... ],,[],[])
> >> > esac
> >> >
> >> > but this way is simpler. If we add more customization for other
> >> > targets we can reconsider using the 'case "${host}"' form.
> >>
> >> Ya, that's kind of where I came to as well -- the proper autoconf flavor
> >> would scale way better, but hopefully nobody else makes this mistake and
> >> thus we don't need to worry about that.
> >
> > <nod>
> >
> >> I'm fine with either way (though I think we'd need a "riscv*" there, to
> >> match riscv32 and riscv64?), so if you want to swap it over (or have me
> >> re-spin this) it's no big deal on my end -- also fine, as per below,
> >> with you just committing this ;)
> >
> > Yeah, I figured *-*-riscv probably wasn't right, so that's another
> > reason to prefer your approach.
> >
> >
> >>
> >> > So this is OK for trunk, modulo regenerating libstdc++-v3/configure
> >> > with this change. Let me know if you want me to do that regen for you
> >> > (or commit the whole thing for you).
> >>
> >> That'd be great, thanks!  It usually takes me a while to get all the
> >> autotools versions lined up (we just got new machines at the office),
> >> that way I won't have to do so.
> >
> > No problem, I can regen+push for you.
>
> Great, thanks!

Pushed as r12-8161-g3fc22eedb033cb