[Bug c++/108659] New: Suboptimal 128 bit atomics codegen on AArch64 and x64

[Bug c++/108659] New: Suboptimal 128 bit atomics codegen on AArch64 and x64

[s_gccbugzilla at nedprod dot com]

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

            Bug ID: 108659
           Summary: Suboptimal 128 bit atomics codegen on AArch64 and x64
           Product: gcc
           Version: 12.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: s_gccbugzilla at nedprod dot com
  Target Milestone: ---

Related:
- https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80878
- https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94649
- https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104688

I got bitten by this again, latest GCC still does not emit single instruction
128 bit atomics, even when the -march is easily new enough. Here is a godbolt
comparing latest MSVC, latest GCC and latest clang for the skylake-avx512
architecture, which unquestionably supports cmpxchg16b. Only clang emits the
single instruction atomic:

https://godbolt.org/z/EnbeeW4az

I'm gathering from the issue comments and from the comments at
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104688 that you're going to wait
for AMD to guarantee atomicity of SSE instructions before changing the codegen
here, which makes sense. However I also wanted to raise potentially suboptimal
128 bit atomic codegen by GCC for AArch64 as compared to clang:

https://godbolt.org/z/oKv4o81nv

GCC emits `dmb` to force a global memory fence, whereas clang does not.

I think clang is in the right here, the seq_cst atomic semantics are not
supposed to globally memory fence.

[Bug target/108659] Suboptimal 128 bit atomics codegen on AArch64 and x64

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Niall Douglas from comment #0)
> I'm gathering from the issue comments and from the comments at
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104688 that you're going to
> wait for AMD to guarantee atomicity of SSE instructions before changing the
> codegen here, which makes sense.

AMD has guaranteed it, but there is still VIA and Zhaoxin and while we have
some statement from the latter, I'm not sure it is enough and we don't have
anything from VIA.  See PR104688 for details.

[Bug target/108659] Suboptimal 128 bit atomics codegen on AArch64 and x64

[ktkachov at gcc dot gnu.org]

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

ktkachov at gcc dot gnu.org changed:

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

--- Comment #2 from ktkachov at gcc dot gnu.org ---
(In reply to Niall Douglas from comment #0)
> Related:
> - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80878
> - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94649
> - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104688
> 
> I got bitten by this again, latest GCC still does not emit single
> instruction 128 bit atomics, even when the -march is easily new enough. Here
> is a godbolt comparing latest MSVC, latest GCC and latest clang for the
> skylake-avx512 architecture, which unquestionably supports cmpxchg16b. Only
> clang emits the single instruction atomic:
> 
> https://godbolt.org/z/EnbeeW4az
> 
> I'm gathering from the issue comments and from the comments at
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104688 that you're going to
> wait for AMD to guarantee atomicity of SSE instructions before changing the
> codegen here, which makes sense. However I also wanted to raise potentially
> suboptimal 128 bit atomic codegen by GCC for AArch64 as compared to clang:
> 
> https://godbolt.org/z/oKv4o81nv
> 
> GCC emits `dmb` to force a global memory fence, whereas clang does not.
> 
> I think clang is in the right here, the seq_cst atomic semantics are not
> supposed to globally memory fence.

FWIW, the GCC codegen for aarch64 is at https://godbolt.org/z/qvx9484nY (arm
and aarch64 are different targets). It emits a call to libatomic, which for GCC
13 will use a lockless implementation when possible at runtime, see
g:d1288d850944f69a795e4ff444a427eba3fec11b

[Bug target/108659] Suboptimal 128 bit atomics codegen on AArch64 and x64

[s_gccbugzilla at nedprod dot com]

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

--- Comment #3 from Niall Douglas <s_gccbugzilla at nedprod dot com> ---
> AMD has guaranteed it, but there is still VIA and Zhaoxin and while we have some statement from the latter, I'm not sure it is enough and we don't have anything from VIA.  See PR104688 for details.

I'm wondering if a compiler opt out flag like -no-msseatomic16 to turn off use
of SSE for 128 bit atomics wouldn't be an idea? Given the small market share of
those CPU vendors, seems a shame to hold up implementation.

(Also, if you do turn it on by default and advertise that widely, I suspect
those vendors will hurry up with their documentation)

> FWIW, the GCC codegen for aarch64 is at https://godbolt.org/z/qvx9484nY (arm and aarch64 are different targets). It emits a call to libatomic, which for GCC 13 will use a lockless implementation when possible at runtime, see g:d1288d850944f69a795e4ff444a427eba3fec11b

Thanks for the catch, my mistake. It would seem the codegen is similarly
inferior to the codegen from clang for both aarch64 and x64.

You may be interested in reading https://reviews.llvm.org/D110069. It wanted to
have LLVM generate a 128 bit AArch64 CAS for atomics. LLVM merged that change,
it'll be in the next release.

[Bug target/108659] Suboptimal 128 bit atomics codegen on AArch64 and x64

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Niall Douglas from comment #3) 
> You may be interested in reading https://reviews.llvm.org/D110069. It wanted
> to have LLVM generate a 128 bit AArch64 CAS for atomics. LLVM merged that
> change, it'll be in the next release.

Using CAS for atomic load is not valid thing to do ...
Because atomic load from constant rodata needs to work.
LLVM breaks this case as they don't care about it. GCC does though.

[Bug target/108659] Suboptimal 128 bit atomics codegen on AArch64 and x64

[wilco at gcc dot gnu.org]

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

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

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

--- Comment #5 from Wilco <wilco at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #4)
> (In reply to Niall Douglas from comment #3) 
> > You may be interested in reading https://reviews.llvm.org/D110069. It wanted
> > to have LLVM generate a 128 bit AArch64 CAS for atomics. LLVM merged that
> > change, it'll be in the next release.
> 
> Using CAS for atomic load is not valid thing to do ...
> Because atomic load from constant rodata needs to work.
> LLVM breaks this case as they don't care about it. GCC does though.

The question is how useful is this in reality? If memory is not writeable then
you can use atomic loads but no other atomic accesses.

We could be pragmatic and say that using 128-bit atomic loads from
non-writeable memory is a user error just like unaligned atomic accesses.

To me a far worse issue is that this difference for 128-bit atomics means that
LLVM and GCC are binary incompatible. AFAIK isn't an option to make them
compatible either (on AArch64 GCC13 will use a compatible sequence only if LSE2
is available).

[Bug target/108659] Suboptimal 128 bit atomics codegen on AArch64 and x64

[pinskia at gcc dot gnu.org]

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

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Wilco from comment #5)
> To me a far worse issue is that this difference for 128-bit atomics means
> that LLVM and GCC are binary incompatible. AFAIK isn't an option to make
> them compatible either (on AArch64 GCC13 will use a compatible sequence only
> if LSE2 is available).

Right and that would mean LLVM is broken for valid C code. And breaks the
binary compatibility with GCC rather than the other way around.
This is not the first time LLVM has chosen to break things with respect to
binary compatibility with GCC (x86_64 argument passing for an example).

[Bug target/108659] Suboptimal 128 bit atomics codegen on AArch64 and x64

[s_gccbugzilla at nedprod dot com]

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

--- Comment #7 from Niall Douglas <s_gccbugzilla at nedprod dot com> ---
(In reply to Andrew Pinski from comment #4)
> (In reply to Niall Douglas from comment #3) 
> > You may be interested in reading https://reviews.llvm.org/D110069. It wanted
> > to have LLVM generate a 128 bit AArch64 CAS for atomics. LLVM merged that
> > change, it'll be in the next release.
> 
> Using CAS for atomic load is not valid thing to do ...
> Because atomic load from constant rodata needs to work.
> LLVM breaks this case as they don't care about it. GCC does though.

I've heard that argument before, and I've always wondered why _Atomic128 types
couldn't have an attribute which applies attribute section to their static
const variable incarnations to force them into r/w memory. That would also
solve the LLVM issue. Said attribute is not unuseful in general actually, it
would help avoid having to mess with mprotect to apply copy on write perms on
regions in .rodata when you need to modify static const variable values.

I don't think that the standard *guarantees* that static const variables go
into read only memory, and besides, before C23 128 bit integers weren't
supported anyway so one could argue as a proprietary extension (__int128) you
get proprietary special casing.

Niall

[Bug target/108659] Suboptimal 128 bit atomics codegen on AArch64 and x64

[wilco at gcc dot gnu.org]

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

--- Comment #8 from Wilco <wilco at gcc dot gnu.org> ---
(In reply to Niall Douglas from comment #7)
> (In reply to Andrew Pinski from comment #4)
> > (In reply to Niall Douglas from comment #3) 
> > > You may be interested in reading https://reviews.llvm.org/D110069. It wanted
> > > to have LLVM generate a 128 bit AArch64 CAS for atomics. LLVM merged that
> > > change, it'll be in the next release.
> > 
> > Using CAS for atomic load is not valid thing to do ...
> > Because atomic load from constant rodata needs to work.
> > LLVM breaks this case as they don't care about it. GCC does though.
> 
> I've heard that argument before, and I've always wondered why _Atomic128
> types couldn't have an attribute which applies attribute section to their
> static const variable incarnations to force them into r/w memory. That would
> also solve the LLVM issue. Said attribute is not unuseful in general
> actually, it would help avoid having to mess with mprotect to apply copy on
> write perms on regions in .rodata when you need to modify static const
> variable values.
> 
> I don't think that the standard *guarantees* that static const variables go
> into read only memory, and besides, before C23 128 bit integers weren't
> supported anyway so one could argue as a proprietary extension (__int128)
> you get proprietary special casing.

Yes that sounds like a reasonable approach. There will language lawyers that
say it must also work on mmap after mprotect of course, but that seems even
more unlikely in the real world...

I believe that the vast majority of developers just want 128-bit atomics to
work efficiently without locks when possible.

Currently various packages are forced to create 128-bit atomics using inline
assembler - and that seems a much worse hack than supporting lock-free atomics
in the compiler.

[Bug target/108659] Suboptimal 128 bit atomics codegen on AArch64 and x64

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Wilco from comment #8)
> Yes that sounds like a reasonable approach.

I don't think so.  Not all variables on which __atomic_* intrinsics are used
are actually _Atomic, the vars can be embedded in const aggregates etc.

[Bug target/108659] Suboptimal 128 bit atomics codegen on AArch64 and x64

[s_gccbugzilla at nedprod dot com]

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

--- Comment #10 from Niall Douglas <s_gccbugzilla at nedprod dot com> ---
(In reply to Jakub Jelinek from comment #9)
> (In reply to Wilco from comment #8)
> > Yes that sounds like a reasonable approach.
> 
> I don't think so.  Not all variables on which __atomic_* intrinsics are used
> are actually _Atomic, the vars can be embedded in const aggregates etc.

I'd have the attribute propagate to enclosing types, like over-alignment.

[Bug target/108659] Suboptimal 128 bit atomics codegen on AArch64 and x64

[wilco at gcc dot gnu.org]

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

--- Comment #11 from Wilco <wilco at gcc dot gnu.org> ---
(In reply to Niall Douglas from comment #10)
> (In reply to Jakub Jelinek from comment #9)
> > (In reply to Wilco from comment #8)
> > > Yes that sounds like a reasonable approach.
> > 
> > I don't think so.  Not all variables on which __atomic_* intrinsics are used
> > are actually _Atomic, the vars can be embedded in const aggregates etc.
> 
> I'd have the attribute propagate to enclosing types, like over-alignment.

Yes, a structure with a 128-bit Atomic type in a subfield/union would be forced
to rwdata.

And arbitrary casts (eg. from char* to an atomic type) wouldn't work due to
Atomics requiring strict alignment. A 128-bit atomic type might have a higher
alignment than a 128-bit integer so even casting that seems questionable.

[Bug target/108659] Suboptimal 128 bit atomics codegen x64

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Suboptimal 128 bit atomics  |Suboptimal 128 bit atomics
                   |codegen on AArch64 and x64  |codegen x64
             Target|aarch64 x86_64              |x86_64
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=70814

--- Comment #12 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
aarch64 was recorded in PR 70814 before.