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

[Palmer Dabbelt <palmer@rivosinc.com>]

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