[Bug target/105162] New: [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[Bug target/105162] New: [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[spop at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

            Bug ID: 105162
           Summary: [AArch64] outline-atomics drops dmb ish barrier on
                    __sync builtins
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: spop at gcc dot gnu.org
  Target Milestone: ---

With -mno-outline-atomics gcc produces a `dmb ish` barrier on __sync builtins
as required by the Intel specification 
(see fix for https://gcc.gnu.org/PR65697 
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=f70fb3b635f9618c6d2ee3848ba836914f7951c2
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=ab876106eb689947cdd8203f8ecc6e8ac38bf5ba
)

$ cat a.c
int foo(int a)
{
  return __sync_bool_compare_and_swap(&a, 4, 5);
}
$ gcc -O2 a.c -S -o- -mno-outline-atomics 
foo:
        sub     sp, sp, #16
        mov     w1, 5
        str     w0, [sp, 12]
        add     x0, sp, 12
.L4:
        ldxr    w2, [x0]
        cmp     w2, 4
        bne     .L5
        stlxr   w3, w1, [x0]
        cbnz    w3, .L4
.L5:
        dmb     ish
        cset    w0, eq
        add     sp, sp, 16
        ret

With -moutline-atomics gcc does not generate the barrier:

$ gcc -O2 a.c -S -o-  -moutline-atomics 
foo:
        stp     x29, x30, [sp, -32]!
        mov     w1, 5
        mov     x29, sp
        add     x2, sp, 28
        str     w0, [sp, 28]
        mov     w0, 4
        bl      __aarch64_cas4_acq_rel
        cmp     w0, 4
        cset    w0, eq
        ldp     x29, x30, [sp], 32
        ret

Happens on gcc-8, 9, 10, 11, and trunk.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[spop at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

--- Comment #1 from Sebastian Pop <spop at gcc dot gnu.org> ---
Also happens when compiling with LSE: -march=armv8.1-a or later.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[spop at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

--- Comment #2 from Sebastian Pop <spop at gcc dot gnu.org> ---
Created attachment 52750
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52750&action=edit
patch

Fix.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[spop at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

Sebastian Pop <spop at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #52750|0                           |1
        is obsolete|                            |

--- Comment #3 from Sebastian Pop <spop at gcc dot gnu.org> ---
Created attachment 52755
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52755&action=edit
patch

LSE atomics do not need a barrier.

Updated the patch to only generate the barriers after outline-atomics calls.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[spop at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

--- Comment #4 from Sebastian Pop <spop at gcc dot gnu.org> ---
The attached patch degrades performance on cpus with LSE: the barrier is not
needed when outline-atomics execute an LSE instruction.

I was thinking to add the barrier to the armv8.0 generic path (no LSE) in the
outline-atomics functions.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[spop at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

Sebastian Pop <spop at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #52755|0                           |1
        is obsolete|                            |

--- Comment #5 from Sebastian Pop <spop at gcc dot gnu.org> ---
Created attachment 52762
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52762&action=edit
patch

The attached patch fixes the issue for __sync builtins by adding the missing
barrier to -march=armv8-a+nolse path in the outline-atomics functions.

The patch also changes the behavior of __atomic builtins for -moutline-atomics
-march=armv8-a+nolse to be the same as for -march=armv8-a+lse.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

ktkachov at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ktkachov at gcc dot gnu.org

--- Comment #6 from ktkachov at gcc dot gnu.org ---
Can you please send the patch to gcc-patches for review. It'll get more eyes
there

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[wilco at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

Wilco <wilco at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |wilco at gcc dot gnu.org

--- Comment #7 from Wilco <wilco at gcc dot gnu.org> ---
(In reply to Sebastian Pop from comment #5)
> Created attachment 52762 [details]
> patch
> 
> The attached patch fixes the issue for __sync builtins by adding the missing
> barrier to -march=armv8-a+nolse path in the outline-atomics functions.
> 
> The patch also changes the behavior of __atomic builtins for
> -moutline-atomics -march=armv8-a+nolse to be the same as for
> -march=armv8-a+lse.

So what is your reasoning for adding the barrier to __atomic as well? Only
__sync needs the extra full barrier, but __atomic does not.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[spop at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

Sebastian Pop <spop at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #52762|0                           |1
        is obsolete|                            |

--- Comment #8 from Sebastian Pop <spop at gcc dot gnu.org> ---
Created attachment 52826
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52826&action=edit
patch

You are right.  Please see attached an amended patch that only adds the
barriers to __sync builtins.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Sebastian Pop <spop@gcc.gnu.org>:

https://gcc.gnu.org/g:bc25483c055d62f94f8c289f80843dda3c4a6ff4

commit r13-446-gbc25483c055d62f94f8c289f80843dda3c4a6ff4
Author: Sebastian Pop <spop@amazon.com>
Date:   Mon Apr 18 15:13:20 2022 +0000

    [AArch64] add barriers to ool __sync builtins

    2022-05-13  Sebastian Pop  <spop@amazon.com>

    gcc/
            PR target/105162
            * config/aarch64/aarch64-protos.h (atomic_ool_names): Increase
dimension
            of str array.
            * config/aarch64/aarch64.cc (aarch64_atomic_ool_func): Call
            memmodel_from_int and handle MEMMODEL_SYNC_*.
            (DEF0): Add __aarch64_*_sync functions.

    gcc/testsuite/
            PR target/105162
            * gcc.target/aarch64/sync-comp-swap-ool.c: New.
            * gcc.target/aarch64/sync-op-acquire-ool.c: New.
            * gcc.target/aarch64/sync-op-full-ool.c: New.
            * gcc.target/aarch64/target_attr_20.c: Update check.
            * gcc.target/aarch64/target_attr_21.c: Same.

    libgcc/
            PR target/105162
            * config/aarch64/lse.S: Define BARRIER and handle memory MODEL 5.
            * config/aarch64/t-lse: Add a 5th memory model for _sync functions.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Sebastian Pop <spop@gcc.gnu.org>:

https://gcc.gnu.org/g:407adcb310df7c85387a02f9b0d5f1df770258e9

commit r9-10184-g407adcb310df7c85387a02f9b0d5f1df770258e9
Author: Sebastian Pop <spop@amazon.com>
Date:   Mon Apr 18 15:13:20 2022 +0000

    [AArch64] add barriers to ool __sync builtins

    2022-05-13  Sebastian Pop  <spop@amazon.com>

    gcc/
            PR target/105162
            * config/aarch64/aarch64-protos.h (atomic_ool_names): Increase
dimension
            of str array.
            * config/aarch64/aarch64.c (aarch64_atomic_ool_func): Call
            memmodel_from_int and handle MEMMODEL_SYNC_*.
            (DEF0): Add __aarch64_*_sync functions.

    gcc/testsuite/
            PR target/105162
            * gcc.target/aarch64/sync-comp-swap-ool.c: New.
            * gcc.target/aarch64/sync-op-acquire-ool.c: New.
            * gcc.target/aarch64/sync-op-full-ool.c: New.
            * gcc.target/aarch64/target_attr_20.c: Update check.
            * gcc.target/aarch64/target_attr_21.c: Same.

    libgcc/
            PR target/105162
            * config/aarch64/lse.S: Define BARRIER and handle memory MODEL 5.
            * config/aarch64/t-lse: Add a 5th memory model for _sync functions.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Sebastian Pop
<spop@gcc.gnu.org>:

https://gcc.gnu.org/g:90a0a5315375fd53baf677e60f0fc996fe2d70b2

commit r10-10745-g90a0a5315375fd53baf677e60f0fc996fe2d70b2
Author: Sebastian Pop <spop@amazon.com>
Date:   Mon Apr 18 15:13:20 2022 +0000

    add barriers to ool __sync builtins

    2022-05-13  Sebastian Pop  <spop@amazon.com>

    gcc/
            PR target/105162
            * config/aarch64/aarch64-protos.h (atomic_ool_names): Increase
dimension
            of str array.
            * config/aarch64/aarch64.c (aarch64_atomic_ool_func): Call
            memmodel_from_int and handle MEMMODEL_SYNC_*.
            (DEF0): Add __aarch64_*_sync functions.

    gcc/testsuite/
            PR target/105162
            * gcc.target/aarch64/sync-comp-swap-ool.c: New.
            * gcc.target/aarch64/sync-op-acquire-ool.c: New.
            * gcc.target/aarch64/sync-op-full-ool.c: New.
            * gcc.target/aarch64/target_attr_20.c: Update check.
            * gcc.target/aarch64/target_attr_21.c: Same.

    libgcc/
            PR target/105162
            * config/aarch64/lse.S: Define BARRIER and handle memory MODEL 5.
            * config/aarch64/t-lse: Add a 5th memory model for _sync functions.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Sebastian Pop
<spop@gcc.gnu.org>:

https://gcc.gnu.org/g:9ef808fb4373c8d8479539cb19ae00b1bcec87ea

commit r11-10005-g9ef808fb4373c8d8479539cb19ae00b1bcec87ea
Author: Sebastian Pop <spop@amazon.com>
Date:   Mon Apr 18 15:13:20 2022 +0000

    [AArch64] add barriers to ool __sync builtins

    2022-05-13  Sebastian Pop  <spop@amazon.com>

    gcc/
            PR target/105162
            * config/aarch64/aarch64-protos.h (atomic_ool_names): Increase
dimension
            of str array.
            * config/aarch64/aarch64.c (aarch64_atomic_ool_func): Call
            memmodel_from_int and handle MEMMODEL_SYNC_*.
            (DEF0): Add __aarch64_*_sync functions.

    gcc/testsuite/
            PR target/105162
            * gcc.target/aarch64/sync-comp-swap-ool.c: New.
            * gcc.target/aarch64/sync-op-acquire-ool.c: New.
            * gcc.target/aarch64/sync-op-full-ool.c: New.
            * gcc.target/aarch64/target_attr_20.c: Update check.
            * gcc.target/aarch64/target_attr_21.c: Same.

    libgcc/
            PR target/105162
            * config/aarch64/lse.S: Define BARRIER and handle memory MODEL 5.
            * config/aarch64/t-lse: Add a 5th memory model for _sync functions.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Sebastian Pop
<spop@gcc.gnu.org>:

https://gcc.gnu.org/g:5251647c84489af05baa38e4dd103cf359f77b75

commit r12-8389-g5251647c84489af05baa38e4dd103cf359f77b75
Author: Sebastian Pop <spop@amazon.com>
Date:   Mon Apr 18 15:13:20 2022 +0000

    [AArch64] add barriers to ool __sync builtins

    2022-05-13  Sebastian Pop  <spop@amazon.com>

    gcc/
            PR target/105162
            * config/aarch64/aarch64-protos.h (atomic_ool_names): Increase
dimension
            of str array.
            * config/aarch64/aarch64.cc (aarch64_atomic_ool_func): Call
            memmodel_from_int and handle MEMMODEL_SYNC_*.
            (DEF0): Add __aarch64_*_sync functions.

    gcc/testsuite/
            PR target/105162
            * gcc.target/aarch64/sync-comp-swap-ool.c: New.
            * gcc.target/aarch64/sync-op-acquire-ool.c: New.
            * gcc.target/aarch64/sync-op-full-ool.c: New.
            * gcc.target/aarch64/target_attr_20.c: Update check.
            * gcc.target/aarch64/target_attr_21.c: Same.

    libgcc/
            PR target/105162
            * config/aarch64/lse.S: Define BARRIER and handle memory MODEL 5.
            * config/aarch64/t-lse: Add a 5th memory model for _sync functions.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[spop at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

Sebastian Pop <spop at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #14 from Sebastian Pop <spop at gcc dot gnu.org> ---
Fixed.

[Bug target/105162] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |9.5
      Known to work|                            |10.4.0, 12.2.0, 9.5.0
      Known to fail|                            |10.3.0, 12.1.0, 9.4.0

[Bug target/105162] [9/10/11/12/13 Regression] [AArch64] outline-atomics drops dmb ish barrier on __sync builtins

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105162

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[AArch64] outline-atomics   |[9/10/11/12/13 Regression]
                   |drops dmb ish barrier on    |[AArch64] outline-atomics
                   |__sync builtins             |drops dmb ish barrier on
                   |                            |__sync builtins
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=65697

--- Comment #15 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This was a regression as outline is the default on most aarch64 targets.