builtins: Emit __sync_lock_release_{8,16} call as last resort instead of doing nothing [PR117642]
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
fail
|
Patch failed to apply
|
| linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Test passed
|
Commit Message
Hi!
As the following testcases show, in case of multi-word
__sync_lock_test_and_set where we don't actually support atomics for that
size (__int128 for x86_64 lp64 with -mno-cx16, long long for ia32 with
-march=i{3,4}86), as the last fallback if we don't know anything else
we just emit calls to __sync_lock_test_and_set_{8,16}. Those aren't defined
in libatomic, but perhaps users could define them themselves.
While __sync_lock_release if it gives up and has no way to emit the atomic
store just does nothing at all, so no clear sign to the users something
went wrong and that the code will not do what they expected.
This regressed when __atomic_* support has been introduced, previously we
would just emit those calls even in this case.
The patch just emits the call as the last fallback.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2024-11-27 Jakub Jelinek <jakub@redhat.com>
PR target/117642
* builtins.cc (expand_builtin_sync_lock_release): Change return type
from void to rtx, return result of expand_atomic_store.
(expand_builtin) <case BUILT_IN_SYNC_LOCK_RELEASE_16>: If
expand_builtin_sync_lock_release returns NULL, do a break rather
than return const0_rtx.
* gcc.target/i386/pr117642-1.c: New test.
* gcc.target/i386/pr117642-2.c: New test.
Jakub
Comments
On Wed, 27 Nov 2024, Jakub Jelinek wrote:
> Hi!
>
> As the following testcases show, in case of multi-word
> __sync_lock_test_and_set where we don't actually support atomics for that
> size (__int128 for x86_64 lp64 with -mno-cx16, long long for ia32 with
> -march=i{3,4}86), as the last fallback if we don't know anything else
> we just emit calls to __sync_lock_test_and_set_{8,16}. Those aren't defined
> in libatomic, but perhaps users could define them themselves.
> While __sync_lock_release if it gives up and has no way to emit the atomic
> store just does nothing at all, so no clear sign to the users something
> went wrong and that the code will not do what they expected.
> This regressed when __atomic_* support has been introduced, previously we
> would just emit those calls even in this case.
>
> The patch just emits the call as the last fallback.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
> 2024-11-27 Jakub Jelinek <jakub@redhat.com>
>
> PR target/117642
> * builtins.cc (expand_builtin_sync_lock_release): Change return type
> from void to rtx, return result of expand_atomic_store.
> (expand_builtin) <case BUILT_IN_SYNC_LOCK_RELEASE_16>: If
> expand_builtin_sync_lock_release returns NULL, do a break rather
> than return const0_rtx.
>
> * gcc.target/i386/pr117642-1.c: New test.
> * gcc.target/i386/pr117642-2.c: New test.
>
> --- gcc/builtins.cc.jj 2024-11-26 09:46:39.513228477 +0100
> +++ gcc/builtins.cc 2024-11-26 15:07:16.236123486 +0100
> @@ -6587,7 +6587,7 @@ expand_builtin_sync_lock_test_and_set (m
>
> /* Expand the __sync_lock_release intrinsic. EXP is the CALL_EXPR. */
>
> -static void
> +static rtx
> expand_builtin_sync_lock_release (machine_mode mode, tree exp)
> {
> rtx mem;
> @@ -6595,7 +6595,7 @@ expand_builtin_sync_lock_release (machin
> /* Expand the operands. */
> mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
>
> - expand_atomic_store (mem, const0_rtx, MEMMODEL_SYNC_RELEASE, true);
> + return expand_atomic_store (mem, const0_rtx, MEMMODEL_SYNC_RELEASE, true);
> }
>
> /* Given an integer representing an ``enum memmodel'', verify its
> @@ -8605,8 +8605,9 @@ expand_builtin (tree exp, rtx target, rt
> case BUILT_IN_SYNC_LOCK_RELEASE_8:
> case BUILT_IN_SYNC_LOCK_RELEASE_16:
> mode = get_builtin_sync_mode (fcode - BUILT_IN_SYNC_LOCK_RELEASE_1);
> - expand_builtin_sync_lock_release (mode, exp);
> - return const0_rtx;
> + if (expand_builtin_sync_lock_release (mode, exp))
> + return const0_rtx;
> + break;
>
> case BUILT_IN_SYNC_SYNCHRONIZE:
> expand_builtin_sync_synchronize ();
> --- gcc/testsuite/gcc.target/i386/pr117642-1.c.jj 2024-11-26 15:18:07.270066467 +0100
> +++ gcc/testsuite/gcc.target/i386/pr117642-1.c 2024-11-26 15:19:20.467048094 +0100
> @@ -0,0 +1,19 @@
> +/* PR target/117642 */
> +/* { dg-do compile { target int128 } } */
> +/* { dg-options "-mno-cx16" } */
> +/* { dg-final { scan-assembler "__sync_lock_test_and_set_16" } } */
> +/* { dg-final { scan-assembler "__sync_lock_release_16" } } */
> +
> +__int128 t = 1;
> +
> +void
> +foo (void)
> +{
> + __sync_lock_test_and_set (&t, 1);
> +}
> +
> +void
> +bar (void)
> +{
> + __sync_lock_release (&t);
> +}
> --- gcc/testsuite/gcc.target/i386/pr117642-2.c.jj 2024-11-26 15:19:33.174871291 +0100
> +++ gcc/testsuite/gcc.target/i386/pr117642-2.c 2024-11-26 15:20:04.092441144 +0100
> @@ -0,0 +1,19 @@
> +/* PR target/117642 */
> +/* { dg-do compile { target ia32 } } */
> +/* { dg-options "-march=i486" } */
> +/* { dg-final { scan-assembler "__sync_lock_test_and_set_8" } } */
> +/* { dg-final { scan-assembler "__sync_lock_release_8" } } */
> +
> +long long t = 1;
> +
> +void
> +foo (void)
> +{
> + __sync_lock_test_and_set (&t, 1);
> +}
> +
> +void
> +bar (void)
> +{
> + __sync_lock_release (&t);
> +}
>
> Jakub
>
>
@@ -6587,7 +6587,7 @@ expand_builtin_sync_lock_test_and_set (m
/* Expand the __sync_lock_release intrinsic. EXP is the CALL_EXPR. */
-static void
+static rtx
expand_builtin_sync_lock_release (machine_mode mode, tree exp)
{
rtx mem;
@@ -6595,7 +6595,7 @@ expand_builtin_sync_lock_release (machin
/* Expand the operands. */
mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
- expand_atomic_store (mem, const0_rtx, MEMMODEL_SYNC_RELEASE, true);
+ return expand_atomic_store (mem, const0_rtx, MEMMODEL_SYNC_RELEASE, true);
}
/* Given an integer representing an ``enum memmodel'', verify its
@@ -8605,8 +8605,9 @@ expand_builtin (tree exp, rtx target, rt
case BUILT_IN_SYNC_LOCK_RELEASE_8:
case BUILT_IN_SYNC_LOCK_RELEASE_16:
mode = get_builtin_sync_mode (fcode - BUILT_IN_SYNC_LOCK_RELEASE_1);
- expand_builtin_sync_lock_release (mode, exp);
- return const0_rtx;
+ if (expand_builtin_sync_lock_release (mode, exp))
+ return const0_rtx;
+ break;
case BUILT_IN_SYNC_SYNCHRONIZE:
expand_builtin_sync_synchronize ();
@@ -0,0 +1,19 @@
+/* PR target/117642 */
+/* { dg-do compile { target int128 } } */
+/* { dg-options "-mno-cx16" } */
+/* { dg-final { scan-assembler "__sync_lock_test_and_set_16" } } */
+/* { dg-final { scan-assembler "__sync_lock_release_16" } } */
+
+__int128 t = 1;
+
+void
+foo (void)
+{
+ __sync_lock_test_and_set (&t, 1);
+}
+
+void
+bar (void)
+{
+ __sync_lock_release (&t);
+}
@@ -0,0 +1,19 @@
+/* PR target/117642 */
+/* { dg-do compile { target ia32 } } */
+/* { dg-options "-march=i486" } */
+/* { dg-final { scan-assembler "__sync_lock_test_and_set_8" } } */
+/* { dg-final { scan-assembler "__sync_lock_release_8" } } */
+
+long long t = 1;
+
+void
+foo (void)
+{
+ __sync_lock_test_and_set (&t, 1);
+}
+
+void
+bar (void)
+{
+ __sync_lock_release (&t);
+}