Hi!
On Tue, 26 Sept 2023 at 16:34, Hans-Peter Nilsson <h...@axis.com> wrote:
> Tested cris-elf, native x86_64-pc-linux-gnu and arm-eabi.
>
> For arm-eabi, notably lacking any atomic support for the
> default multilib, with --target_board=arm-sim it regressed
> 29_atomics/atomic_flag/cons/value_init.cc with the expected
> linker failure due to lack of __atomic_test_and_set - which
> is a good thing. With this one, there are 44 unexpected
> FAILs for libstdc+++ at r14-4210-g94982a6b9cf4. This number
> was 206 as late as r14-3470-g721f7e2c4e5e, but mitigated by
> r14-3980-g62b29347c38394, deliberately. To fix the
> regression, I'll do the same and follow up with adding
> dg-require-thread-fence on
> 29_atomics/atomic_flag/cons/value_init.cc (and if approved,
> commit it before this one).
>
> Incidentally, the fortran test-results for arm-eabi are
> riddled with missing-__sync_synchronize linker errors
> causing some 18134 unexpected failures, where cris-elf has
> 121.
>
>
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'
Maybe we need a new variant of dg-require-thread-fence ?
Thanks,
Christophe
Ok to commit?
>
> -- >8 --
> Make __atomic_test_and_set consistent with other __atomic_ and __sync_
> builtins: call a matching library function instead of emitting
> non-atomic code when the target has no direct insn support.
>
> There's special-case code handling targetm.atomic_test_and_set_trueval
> != 1 trying a modified maybe_emit_sync_lock_test_and_set. Previously,
> if that worked but its matching emit_store_flag_force returned NULL,
> we'd segfault later on. Now that the caller handles NULL, gcc_assert
> here instead.
>
> While the referenced PR:s are ARM-specific, the issue is general.
>
> PR target/107567
> PR target/109166
> * builtins.cc (expand_builtin) <case BUILT_IN_ATOMIC_TEST_AND_SET>:
> Handle failure from expand_builtin_atomic_test_and_set.
> * optabs.cc (expand_atomic_test_and_set): When all attempts fail to
> generate atomic code through target support, return NULL
> instead of emitting non-atomic code. Also, for code handling
> targetm.atomic_test_and_set_trueval != 1, gcc_assert result
> from calling emit_store_flag_force instead of returning NULL.
> ---
> gcc/builtins.cc | 5 ++++-
> gcc/optabs.cc | 22 +++++++---------------
> 2 files changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/gcc/builtins.cc b/gcc/builtins.cc
> index 6e4274bb2a4e..40dfd36a3197 100644
> --- a/gcc/builtins.cc
> +++ b/gcc/builtins.cc
> @@ -8387,7 +8387,10 @@ expand_builtin (tree exp, rtx target, rtx
> subtarget, machine_mode mode,
> break;
>
> case BUILT_IN_ATOMIC_TEST_AND_SET:
> - return expand_builtin_atomic_test_and_set (exp, target);
> + target = expand_builtin_atomic_test_and_set (exp, target);
> + if (target)
> + return target;
> + break;
>
> case BUILT_IN_ATOMIC_CLEAR:
> return expand_builtin_atomic_clear (exp);
> diff --git a/gcc/optabs.cc b/gcc/optabs.cc
> index 8b96f23aec05..e1898da22808 100644
> --- a/gcc/optabs.cc
> +++ b/gcc/optabs.cc
> @@ -7080,25 +7080,17 @@ expand_atomic_test_and_set (rtx target, rtx mem,
> enum memmodel model)
> /* Recall that the legacy lock_test_and_set optab was allowed to do
> magic
> things with the value 1. Thus we try again without trueval. */
> if (!ret && targetm.atomic_test_and_set_trueval != 1)
> - ret = maybe_emit_sync_lock_test_and_set (subtarget, mem, const1_rtx,
> model);
> -
> - /* Failing all else, assume a single threaded environment and simply
> - perform the operation. */
> - if (!ret)
> {
> - /* If the result is ignored skip the move to target. */
> - if (subtarget != const0_rtx)
> - emit_move_insn (subtarget, mem);
> + ret = maybe_emit_sync_lock_test_and_set (subtarget, mem,
> const1_rtx, model);
>
> - emit_move_insn (mem, trueval);
> - ret = subtarget;
> + if (ret)
> + {
> + /* Rectify the not-one trueval. */
> + ret = emit_store_flag_force (target, NE, ret, const0_rtx, mode,
> 0, 1);
> + gcc_assert (ret);
> + }
> }
>
> - /* Recall that have to return a boolean value; rectify if trueval
> - is not exactly one. */
> - if (targetm.atomic_test_and_set_trueval != 1)
> - ret = emit_store_flag_force (target, NE, ret, const0_rtx, mode, 0, 1);
> -
> return ret;
> }
>
> --
> 2.30.2
>
>
