Message ID | 20220407184614.1216-1-palmer@rivosinc.com |
---|---|
State | New |
Headers | show |
Series | [v1] libstdc++: Default to mutex-based atomics on RISC-V | expand |
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
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
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.
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!
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
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