[Bug translation/96159] New: atomic creates incorrect code for possible isaligned struct

[Bug translation/96159] New: atomic creates incorrect code for possible isaligned struct

[uecker at eecs dot berkeley.edu]

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

            Bug ID: 96159
           Summary: atomic creates incorrect code for possible isaligned
                    struct
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: translation
          Assignee: unassigned at gcc dot gnu.org
          Reporter: uecker at eecs dot berkeley.edu
  Target Milestone: ---

[Bug translation/96159] atomic creates incorrect code for possible isaligned struct

[uecker at eecs dot berkeley.edu]

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

--- Comment #1 from Martin Uecker <uecker at eecs dot berkeley.edu> ---



On x86-64 the following struct has alignment 4 but gcc
creates a single mov instruction which according to my
understanding may fail to be atomic when it crosses a
cache line boundary. 

Documentation seems to imply that __atomic_load is
supposed to work for all types. It also compiles
without warning.


typedef struct { float re; float im; } foo_t;

int foo_align(void)
{
        foo_t x;
        return _Alignof(x);
}

foo_t foo_load(foo_t* x)
{
        foo_t r;
        __atomic_load(x, &r, __ATOMIC_SEQ_CST);
        return r;
}


assembler:

foo_align:
        mov     eax, 4
        ret
foo_load:
        movq    xmm0, QWORD PTR [rdi]
        ret

[Bug translation/96159] atomic creates incorrect code for possible isaligned struct

[uecker at eecs dot berkeley.edu]

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

--- Comment #2 from Martin Uecker <uecker at eecs dot berkeley.edu> ---

Clang produces a call to __atomic_load.

Also here is a godbolt link: https://godbolt.org/z/39PE1G

[Bug middle-end/96159] atomic creates incorrect code for possible isaligned struct

[muecker at gwdg dot de]

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

--- Comment #3 from Martin Uecker <muecker at gwdg dot de> ---

The documentation:

https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html#g_t_005f_005fatomic-Builtins

"The four non-arithmetic functions (load, store, exchange, and
compare_exchange) all have a generic version as well. This generic version
works on any data type.
It uses the lock-free built-in function if the specific data type size makes
that possible; otherwise, an external call is left to be resolved at run time."

[Bug middle-end/96159] atomic creates incorrect code for possible isaligned struct

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
So it looks like __atomic_load assumes the type is _Atomic which has the right
alignment to it.

[Bug middle-end/96159] atomic creates incorrect code for possible isaligned struct

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|ABI, wrong-code             |documentation
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-11-26
             Status|UNCONFIRMED                 |NEW

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
It assumes that because the purpose of those built-ins is to implement C
_Atomic and C++ std::atomic. If you use those APIs the alignment is taken care
of n by the implementation. If you use the low-level, non-portable APIs
directly then you are responsible for getting it right.

This is just a documentation bug.

[Bug middle-end/96159] atomic creates incorrect code for possible isaligned struct

[redi at gcc dot gnu.org]

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Martin Uecker from comment #3)
> "The four non-arithmetic functions (load, store, exchange, and
> compare_exchange) all have a generic version as well. This generic version
> works on any data type.

This is true. They work on your type, but not for all values of that type, only
ones that are appropriately aligned.

> It uses the lock-free built-in function if the specific data type size makes
> that possible;


As it says, it only depends on the size, there is no check for alignment. 

What's missing is documentation about the requirement for the user to ensure
the right alignment.

[Bug middle-end/96159] atomic creates incorrect code for possible isaligned struct

[muecker at gwdg dot de]

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

--- Comment #7 from Martin Uecker <muecker at gwdg dot de> ---


I do not think these functions are meant only as internal tools to implement
the language features. 

We also seem to agree that the documentation implies that there should work for
all types and does not currently mention alignment.

One can of course now retrospectively change the documentation to fix this bug. 

I would rather prefer to make this work as it is very useful functionality.

[Bug middle-end/96159] atomic creates incorrect code for possible isaligned struct

[gabravier at gmail dot com]

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

Gabriel Ravier <gabravier at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gabravier at gmail dot com

--- Comment #8 from Gabriel Ravier <gabravier at gmail dot com> ---
I do agree that either: 
- GCC's behavior should be aligned with Clang's and that it should provide some
kind of "known-aligned load" function (along with the corresponding ones for
store/exchange/compare_exchange)
- or there should at least be some kind of "safe load" function (along with the
corresponding ones) for the cases where the alignment is unknown.

I personally do prefer the first solution, personally, unless the Clang devs
can be convinced to change their builtin, as I think it would be better to have
the same behavior on both compilers.

In any case, though, I do think there is a documentation bug here, and I would
also say it would be quite nice to have a warning when using the built-in in a
way that makes it much slower/invalid, like what Clang does:

<source>:240:5: warning: misaligned atomic operation may incur significant
performance penalty; the expected alignment (8 bytes) exceeds the actual
alignment (4 bytes) [-Watomic-alignment]
    __atomic_load(x, &r, __ATOMIC_SEQ_CST);
    ^

[Bug middle-end/96159] atomic creates incorrect code for possible isaligned struct

[muecker at gwdg dot de]

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

--- Comment #9 from Martin Uecker <muecker at gwdg dot de> ---


Yes the clang behavior is useful. 

If we get the optimal code for types with sufficient alignment, then I do not
think a separate set of functions would be required.  A programmer simply can
use the _Atomic(T) types which have optimal alignment to get fastest code 
possible and somebody who needs these functions on existing data structures
could get the safe access by default (and maybe a warning it alignment is not
optimal).  This would appear to be the ideal design.

Even if we document the alignment requirement, it is still very dangerous to
use because alignment requirement may not be identical for all architectures.

[Bug middle-end/96159] atomic creates incorrect code for possible misaligned struct

[muecker at gwdg dot de]

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

Martin Uecker <muecker at gwdg dot de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|atomic creates incorrect    |atomic creates incorrect
                   |code for possible isaligned |code for possible
                   |struct                      |misaligned struct

--- Comment #10 from Martin Uecker <muecker at gwdg dot de> ---
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87237