[2/2] testsuite: Replace many dg-require-thread-fence with dg-require-atomic-exchange

  > From: Christophe Lyon <christophe.lyon@linaro.org>
> Date: Tue, 3 Oct 2023 15:20:39 +0200

> The patch passed almost all our CI configurations, except arm-eabi when
> testing with
>  -mthumb/-march=armv6s-m/-mtune=cortex-m0/-mfloat-abi=soft/-mfpu=auto
> where is causes these failures:
> FAIL: 29_atomics/atomic_flag/clear/1.cc -std=gnu++17 (test for excess
> errors)
> UNRESOLVED: 29_atomics/atomic_flag/clear/1.cc -std=gnu++17 compilation
> failed to produce executable
> FAIL: 29_atomics/atomic_flag/cons/value_init.cc -std=gnu++20 (test for
> excess errors)
> UNRESOLVED: 29_atomics/atomic_flag/cons/value_init.cc -std=gnu++20
> compilation failed to produce executable
> FAIL: 29_atomics/atomic_flag/cons/value_init.cc -std=gnu++26 (test for
> excess errors)
> UNRESOLVED: 29_atomics/atomic_flag/cons/value_init.cc -std=gnu++26
> compilation failed to produce executable
> FAIL: 29_atomics/atomic_flag/test_and_set/explicit.cc -std=gnu++17 (test
> for excess errors)
> UNRESOLVED: 29_atomics/atomic_flag/test_and_set/explicit.cc -std=gnu++17
> compilation failed to produce executable
> FAIL: 29_atomics/atomic_flag/test_and_set/implicit.cc -std=gnu++17 (test
> for excess errors)
> UNRESOLVED: 29_atomics/atomic_flag/test_and_set/implicit.cc -std=gnu++17
> compilation failed to produce executable
> 
> The linker error is:
> undefined reference to `__atomic_test_and_set'

Here's 2/2, fixing those regressions by, after code
inspection, gating those test-case users of
dg-require-thread-fence that actually use not just
atomic-load/store functionality, but a form of
compare-exchange, including the test-and-set cases listed
above as testsuite regressions.  That neatly includes the
regressions above.

Again, other libstdc++ test-cases should likely also use
this gate, from what I see of "undefined references" in
libstdc++.log.

Tested together with 1/2.
Ok to commit?

(N.B. there was a stray suffix "non-atomic code" in the
subject of 1/2; that's just a typo which will not be
committed.)

-- >8 --
These tests actually use a form of atomic exchange, not just
atomic loading and storing.  Some target have the latter,
but not the former, yielding linker errors for missing
library functions (and not supported by libatomic).

This change is just for existing uses of
dg-require-thread-fence.  It does not fix any other tests
that should also be gated on dg-require-atomic-exchange.

	* testsuite/29_atomics/atomic/compare_exchange_padding.cc,
	testsuite/29_atomics/atomic_flag/clear/1.cc,
	testsuite/29_atomics/atomic_flag/cons/value_init.cc,
	testsuite/29_atomics/atomic_flag/test_and_set/explicit.cc,
	testsuite/29_atomics/atomic_flag/test_and_set/implicit.cc,
	testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc,
	testsuite/29_atomics/atomic_ref/generic.cc,
	testsuite/29_atomics/atomic_ref/integral.cc,
	testsuite/29_atomics/atomic_ref/pointer.cc: Replace
	dg-require-thread-fence with dg-require-atomic-exchange.
---
 .../testsuite/29_atomics/atomic/compare_exchange_padding.cc     | 2 +-
 libstdc++-v3/testsuite/29_atomics/atomic_flag/clear/1.cc        | 2 +-
 .../testsuite/29_atomics/atomic_flag/cons/value_init.cc         | 2 +-
 .../testsuite/29_atomics/atomic_flag/test_and_set/explicit.cc   | 2 +-
 .../testsuite/29_atomics/atomic_flag/test_and_set/implicit.cc   | 2 +-
 .../testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc | 2 +-
 libstdc++-v3/testsuite/29_atomics/atomic_ref/generic.cc         | 2 +-
 libstdc++-v3/testsuite/29_atomics/atomic_ref/integral.cc        | 2 +-
 libstdc++-v3/testsuite/29_atomics/atomic_ref/pointer.cc         | 2 +-
 9 files changed, 9 insertions(+), 9 deletions(-)
  

On Wed, 4 Oct 2023 at 04:11, Hans-Peter Nilsson <hp@axis.com> wrote:
>
> > From: Christophe Lyon <christophe.lyon@linaro.org>
> > Date: Tue, 3 Oct 2023 15:20:39 +0200
>
> > The patch passed almost all our CI configurations, except arm-eabi when
> > testing with
> >  -mthumb/-march=armv6s-m/-mtune=cortex-m0/-mfloat-abi=soft/-mfpu=auto
> > where is causes these failures:
> > FAIL: 29_atomics/atomic_flag/clear/1.cc -std=gnu++17 (test for excess
> > errors)
> > UNRESOLVED: 29_atomics/atomic_flag/clear/1.cc -std=gnu++17 compilation
> > failed to produce executable
> > FAIL: 29_atomics/atomic_flag/cons/value_init.cc -std=gnu++20 (test for
> > excess errors)
> > UNRESOLVED: 29_atomics/atomic_flag/cons/value_init.cc -std=gnu++20
> > compilation failed to produce executable
> > FAIL: 29_atomics/atomic_flag/cons/value_init.cc -std=gnu++26 (test for
> > excess errors)
> > UNRESOLVED: 29_atomics/atomic_flag/cons/value_init.cc -std=gnu++26
> > compilation failed to produce executable
> > FAIL: 29_atomics/atomic_flag/test_and_set/explicit.cc -std=gnu++17 (test
> > for excess errors)
> > UNRESOLVED: 29_atomics/atomic_flag/test_and_set/explicit.cc -std=gnu++17
> > compilation failed to produce executable
> > FAIL: 29_atomics/atomic_flag/test_and_set/implicit.cc -std=gnu++17 (test
> > for excess errors)
> > UNRESOLVED: 29_atomics/atomic_flag/test_and_set/implicit.cc -std=gnu++17
> > compilation failed to produce executable
> >
> > The linker error is:
> > undefined reference to `__atomic_test_and_set'
>
> Here's 2/2, fixing those regressions by, after code
> inspection, gating those test-case users of
> dg-require-thread-fence that actually use not just
> atomic-load/store functionality, but a form of
> compare-exchange, including the test-and-set cases listed
> above as testsuite regressions.  That neatly includes the
> regressions above.

The new dg-require proc checks for __atomic_exchange, which is not the
same as compare-exchange, and not the same as test-and-set on
atomic_flag. Does it just happen to be true for arm that the presence
of __atomic_exchange also implies the other atomic operations?

The new proc also only tests it for int, which presumably means none
of these tests rely on atomics for long long being present. Some of
the tests use atomics on pointers, which should work for ILP32 targets
if atomics work on int, but the new dg-require-atomic-exchange isn't
sufficient to check that it will work for pointers on LP64 targets.

Maybe it happens to work today for the targets where we have issues,
but we seem to be assuming that there will be no LP64 targets where
these atomics are absent for 64-bit pointers. Are there supported
risc-v ISA subsets where that isn't true? And we're assuming that
__atomic_exchange being present implies all other ops are present,
which seems like a bad assumption to me. I would be more confident
testing for __atomic_compare_exchange, because with a wait-free CAS
you can implement any other atomic operation, but the same isn't true
for __atomic_exchange.

>
> Again, other libstdc++ test-cases should likely also use
> this gate, from what I see of "undefined references" in
> libstdc++.log.
>
> Tested together with 1/2.
> Ok to commit?
>
> (N.B. there was a stray suffix "non-atomic code" in the
> subject of 1/2; that's just a typo which will not be
> committed.)
>
> -- >8 --
> These tests actually use a form of atomic exchange, not just
> atomic loading and storing.  Some target have the latter,
> but not the former, yielding linker errors for missing
> library functions (and not supported by libatomic).
>
> This change is just for existing uses of
> dg-require-thread-fence.  It does not fix any other tests
> that should also be gated on dg-require-atomic-exchange.
>
>         * testsuite/29_atomics/atomic/compare_exchange_padding.cc,
>         testsuite/29_atomics/atomic_flag/clear/1.cc,
>         testsuite/29_atomics/atomic_flag/cons/value_init.cc,
>         testsuite/29_atomics/atomic_flag/test_and_set/explicit.cc,
>         testsuite/29_atomics/atomic_flag/test_and_set/implicit.cc,
>         testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc,
>         testsuite/29_atomics/atomic_ref/generic.cc,
>         testsuite/29_atomics/atomic_ref/integral.cc,
>         testsuite/29_atomics/atomic_ref/pointer.cc: Replace
>         dg-require-thread-fence with dg-require-atomic-exchange.
> ---
>  .../testsuite/29_atomics/atomic/compare_exchange_padding.cc     | 2 +-
>  libstdc++-v3/testsuite/29_atomics/atomic_flag/clear/1.cc        | 2 +-
>  .../testsuite/29_atomics/atomic_flag/cons/value_init.cc         | 2 +-
>  .../testsuite/29_atomics/atomic_flag/test_and_set/explicit.cc   | 2 +-
>  .../testsuite/29_atomics/atomic_flag/test_and_set/implicit.cc   | 2 +-
>  .../testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc | 2 +-
>  libstdc++-v3/testsuite/29_atomics/atomic_ref/generic.cc         | 2 +-
>  libstdc++-v3/testsuite/29_atomics/atomic_ref/integral.cc        | 2 +-
>  libstdc++-v3/testsuite/29_atomics/atomic_ref/pointer.cc         | 2 +-
>  9 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc b/libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc
> index 01f7475631e6..14698bb82456 100644
> --- a/libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc
> +++ b/libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc
> @@ -1,5 +1,5 @@
>  // { dg-do run { target c++20 } }
> -// { dg-require-thread-fence "" }
> +// { dg-require-atomic-exchange "" }
>  // { dg-add-options libatomic }
>
>  #include <atomic>
> diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_flag/clear/1.cc b/libstdc++-v3/testsuite/29_atomics/atomic_flag/clear/1.cc
> index 89ed381fe057..0d8a11899ef1 100644
> --- a/libstdc++-v3/testsuite/29_atomics/atomic_flag/clear/1.cc
> +++ b/libstdc++-v3/testsuite/29_atomics/atomic_flag/clear/1.cc
> @@ -1,5 +1,5 @@
>  // { dg-do run { target c++11 } }
> -// { dg-require-thread-fence "" }
> +// { dg-require-atomic-exchange "" }
>
>  // Copyright (C) 2009-2023 Free Software Foundation, Inc.
>  //
> diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/value_init.cc b/libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/value_init.cc
> index f3f38b54dbcd..f95818532107 100644
> --- a/libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/value_init.cc
> +++ b/libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/value_init.cc
> @@ -16,7 +16,7 @@
>  // <http://www.gnu.org/licenses/>.
>
>  // { dg-do run { target c++20 } }
> -// { dg-require-thread-fence "" }
> +// { dg-require-atomic-exchange "" }
>
>  #include <atomic>
>  #include <testsuite_hooks.h>
> diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_flag/test_and_set/explicit.cc b/libstdc++-v3/testsuite/29_atomics/atomic_flag/test_and_set/explicit.cc
> index 6f723eb5f4e7..65ccee32142d 100644
> --- a/libstdc++-v3/testsuite/29_atomics/atomic_flag/test_and_set/explicit.cc
> +++ b/libstdc++-v3/testsuite/29_atomics/atomic_flag/test_and_set/explicit.cc
> @@ -1,5 +1,5 @@
>  // { dg-do run { target c++11 } }
> -// { dg-require-thread-fence "" }
> +// { dg-require-atomic-exchange "" }
>
>  // Copyright (C) 2008-2023 Free Software Foundation, Inc.
>  //
> diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_flag/test_and_set/implicit.cc b/libstdc++-v3/testsuite/29_atomics/atomic_flag/test_and_set/implicit.cc
> index 6f723eb5f4e7..65ccee32142d 100644
> --- a/libstdc++-v3/testsuite/29_atomics/atomic_flag/test_and_set/implicit.cc
> +++ b/libstdc++-v3/testsuite/29_atomics/atomic_flag/test_and_set/implicit.cc
> @@ -1,5 +1,5 @@
>  // { dg-do run { target c++11 } }
> -// { dg-require-thread-fence "" }
> +// { dg-require-atomic-exchange "" }
>
>  // Copyright (C) 2008-2023 Free Software Foundation, Inc.
>  //
> diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc
> index 2a3d1d468c22..1573a11d07e8 100644
> --- a/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc
> +++ b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc
> @@ -1,5 +1,5 @@
>  // { dg-do run { target c++20 } }
> -// { dg-require-thread-fence "" }
> +// { dg-require-atomic-exchange "" }
>  // { dg-add-options libatomic }
>
>  #include <atomic>
> diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_ref/generic.cc b/libstdc++-v3/testsuite/29_atomics/atomic_ref/generic.cc
> index f8751756d02c..1e0aef7ba5f6 100644
> --- a/libstdc++-v3/testsuite/29_atomics/atomic_ref/generic.cc
> +++ b/libstdc++-v3/testsuite/29_atomics/atomic_ref/generic.cc
> @@ -16,7 +16,7 @@
>  // <http://www.gnu.org/licenses/>.
>
>  // { dg-do run { target c++20 } }
> -// { dg-require-thread-fence "" }
> +// { dg-require-atomic-exchange "" }
>  // { dg-add-options libatomic }
>
>  #include <atomic>
> diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_ref/integral.cc b/libstdc++-v3/testsuite/29_atomics/atomic_ref/integral.cc
> index eb22afca03a2..3d81f0886399 100644
> --- a/libstdc++-v3/testsuite/29_atomics/atomic_ref/integral.cc
> +++ b/libstdc++-v3/testsuite/29_atomics/atomic_ref/integral.cc
> @@ -16,7 +16,7 @@
>  // <http://www.gnu.org/licenses/>.
>
>  // { dg-do run { target c++20 } }
> -// { dg-require-thread-fence "" }
> +// { dg-require-atomic-exchange "" }
>  // { dg-add-options libatomic }
>
>  #include <atomic>
> diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_ref/pointer.cc b/libstdc++-v3/testsuite/29_atomics/atomic_ref/pointer.cc
> index 6fe00b557567..9c4afd61c365 100644
> --- a/libstdc++-v3/testsuite/29_atomics/atomic_ref/pointer.cc
> +++ b/libstdc++-v3/testsuite/29_atomics/atomic_ref/pointer.cc
> @@ -16,7 +16,7 @@
>  // <http://www.gnu.org/licenses/>.
>
>  // { dg-do run { target c++20 } }
> -// { dg-require-thread-fence "" }
> +// { dg-require-atomic-exchange "" }
>  // { dg-add-options libatomic }
>
>  #include <atomic>
> --
> 2.30.2
>
  

> From: Jonathan Wakely <jwakely@redhat.com>
> Date: Wed, 4 Oct 2023 09:29:43 +0100

> The new dg-require proc checks for __atomic_exchange, which is not the
> same as compare-exchange, and not the same as test-and-set on
> atomic_flag. Does it just happen to be true for arm that the presence
> of __atomic_exchange also implies the other atomic operations?

I missed pointing out that if the target implements
something that emits actual insns for __atomic_exchange
(and/or __atomic_compare_exchange), it has also implemented
test-and-set.  Cf. optabs.cc:expand_atomic_test_and_set (at
r14-4395-g027a94cf32be0b).

Similarly a insn-level __atomic_compare_exchange
implementation (atomic_compare_and_swapM) also does it for
__atomic_exchange.

> The new proc also only tests it for int, which presumably means none
> of these tests rely on atomics for long long being present. Some of
> the tests use atomics on pointers, which should work for ILP32 targets
> if atomics work on int, but the new dg-require-atomic-exchange isn't
> sufficient to check that it will work for pointers on LP64 targets.

Right, I'll amend to test a uintptr_t...

> Maybe it happens to work today for the targets where we have issues,
> but we seem to be assuming that there will be no LP64 targets where
> these atomics are absent for 64-bit pointers. Are there supported
> risc-v ISA subsets where that isn't true?

...but generally, I'd like to leave future amendments like
that to the Next Guy, just like the Previous Guy left
dg-require-thread-fence for me (us) to split up.  But,
perhaps we can prepare for such amendments by giving it a
more specific name: suggesting
dg-require-atomic-cmpxchg-word (bikeshedding opportunity),
testing __atomic_compare_exchange.

IOW, I don't think we should make a distinction, looking for
other operations and sizes at this time.

> And we're assuming that
> __atomic_exchange being present implies all other ops are present,
> which seems like a bad assumption to me. I would be more confident
> testing for __atomic_compare_exchange, because with a wait-free CAS
> you can implement any other atomic operation, but the same isn't true
> for __atomic_exchange.

Yes, I switched them around.

New version coming up.

brgds, H-P
  

On Wed, 4 Oct 2023 at 16:15, Hans-Peter Nilsson <hp@axis.com> wrote:
>
> > From: Jonathan Wakely <jwakely@redhat.com>
> > Date: Wed, 4 Oct 2023 09:29:43 +0100
>
> > The new dg-require proc checks for __atomic_exchange, which is not the
> > same as compare-exchange, and not the same as test-and-set on
> > atomic_flag. Does it just happen to be true for arm that the presence
> > of __atomic_exchange also implies the other atomic operations?
>
> I missed pointing out that if the target implements
> something that emits actual insns for __atomic_exchange
> (and/or __atomic_compare_exchange), it has also implemented
> test-and-set.  Cf. optabs.cc:expand_atomic_test_and_set (at
> r14-4395-g027a94cf32be0b).
>
> Similarly a insn-level __atomic_compare_exchange
> implementation (atomic_compare_and_swapM) also does it for
> __atomic_exchange.

Ah great, that makes testing __atomic_exchange good enough then.

>
> > The new proc also only tests it for int, which presumably means none
> > of these tests rely on atomics for long long being present. Some of
> > the tests use atomics on pointers, which should work for ILP32 targets
> > if atomics work on int, but the new dg-require-atomic-exchange isn't
> > sufficient to check that it will work for pointers on LP64 targets.
>
> Right, I'll amend to test a uintptr_t...
>
> > Maybe it happens to work today for the targets where we have issues,
> > but we seem to be assuming that there will be no LP64 targets where
> > these atomics are absent for 64-bit pointers. Are there supported
> > risc-v ISA subsets where that isn't true?
>
> ...but generally, I'd like to leave future amendments like
> that to the Next Guy, just like the Previous Guy left
> dg-require-thread-fence for me (us) to split up.  But,
> perhaps we can prepare for such amendments by giving it a
> more specific name: suggesting
> dg-require-atomic-cmpxchg-word (bikeshedding opportunity),
> testing __atomic_compare_exchange.
>
> IOW, I don't think we should make a distinction, looking for
> other operations and sizes at this time.

Yep, that's fine. Worse is better. Maybe just add a comment before the
definition of the new effective target check noting that we only test
for one atomic op, but the implementation in gcc means that's good
enough.

>
> > And we're assuming that
> > __atomic_exchange being present implies all other ops are present,
> > which seems like a bad assumption to me. I would be more confident
> > testing for __atomic_compare_exchange, because with a wait-free CAS
> > you can implement any other atomic operation, but the same isn't true
> > for __atomic_exchange.
>
> Yes, I switched them around.
>
> New version coming up.

Thanks!


>
> brgds, H-P
>