`__builtin_assume_aligned` not taken into account for `__int128` `__sync*` diagnostics

[tbaederr]

Consider:

static inline void
atomic16_set(__int128 *ptr, __int128 val)
{
    __int128_t *ptr_align = __builtin_assume_aligned(ptr, 16);
    __int128_t old;

    __sync_bool_compare_and_swap_16(ptr_align, old, 0);
}

On s390, this causes a warning from codegen, because ptr_align is not "naturally aligned".

This was introduced in https://reviews.llvm.org/D143813.
Is the correct alignment for 128bit integers 8 byte or 16 byte on s390? Is the __builtin_assume_aligned being propagated and considered for the diagnostic or not?

See also: https://gitlab.com/qemu-project/qemu/-/issues/1934

CC @JonPsson @efriedma-quic

[uweigand]

On s390x, the ABI only requires 8-byte alignment for __int128. (This is primarily because the stack pointer on function entry is only guaranteed to be 8-byte aligned, and therefore any 16-byte aligned local variable would require dynamic stack realignment at runtime.)

That's fine for all non-atomic operations. For atomic operations using the hardware instructions, however, the variable must actually be 16-byte aligned. To achive that, the variable must have been defined using attribute((aligned(16))) - a mere __builtin_assume_aligned does not suffice.

Now, it is possible to use the __atomic_... family of intrinsics even if the __int128 variable is not (known to be) 16-byte aligned. The compiler will in this case emit a call to a library routine, which will perform a runtime alignment check and use either the hardware instruction (if aligned) or a separate software lock (if not). But that is not supported with the old __sync_... family on intrinsics in clang, which are defined to always require naturally-aligned inputs.

So to fix this problem I would recommend you align all inputs using attribute((aligned(16))), or -if that is not always possible- use __atomic_... intrinsics instead of __sync_... intrinsics.

[rth7680]

In our use case, the alignment is known to be 16 because we explicitly allocated it that way.

For the compiler, __builtin_assume_aligned must suffice, because it is an "I told you so".
This should be considered no different than an explicit ptr = ptr & ~15.

[efriedma-quic]

The utility that's used to compute the alignment of the pointer is very limited. It can understand something like the following:

void
atomic16_set(__int128 *ptr, __int128 val)
{
    struct __attribute((may_alias,aligned(16))) {__int128_t x;} *ptr_align = (void*)ptr;
    __int128_t old;
    __sync_bool_compare_and_swap_16(&ptr_align->x, old, 0);
}

But it doesn't understand much else; in particular, it doesn't do any dataflow analysis, so it doesn't look at the call to __builtin_assume_aligned. There's probably room for improvement here.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Timm Baeder (tbaederr)

Consider: ```c static inline void atomic16_set(__int128 *ptr, __int128 val) { __int128_t *ptr_align = __builtin_assume_aligned(ptr, 16); __int128_t old;

__sync_bool_compare_and_swap_16(ptr_align, old, 0);

}

On s390, this causes a warning from codegen, because `ptr_align` is not "naturally aligned".

This was introduced in https://reviews.llvm.org/D143813.
Is the correct alignment for 128bit integers 8 byte or 16 byte on s390? Is the `__builtin_assume_aligned` being propagated and considered for the diagnostic or not?

See also: https://gitlab.com/qemu-project/qemu/-/issues/1934

CC @<!-- -->JonPsson @<!-- -->efriedma-quic 
</details>

[am11]

Same issue with i386 https://godbolt.org/z/ndaEne7Kh. I was expecting m1 and m2 (in addition to m3) to satisfy the diagnostic. I think it's better to suppress Wsync-alignment for now.

[dominik-steenken]

Hi, i attempted to reproduce this issue on current main and failed to do so:

$ cat issue.c
static inline void atomic16_set(__int128 *ptr, __int128 val)
{
    __int128_t *ptr_align = __builtin_assume_aligned(ptr, 16);
    __int128_t old;

    __sync_bool_compare_and_swap_16(ptr_align, old, 0);
}
$ clang issue.c -o issue.o -Wsync-alignment -c && echo "compiles"
compiles

I also tried with llvm-17.0.1, which is the oldest version of llvm that has the commit that introduced the warning:

$ release.llvmorg-17.0.1/bin/clang issue.c -o issue.o -c && echo "compiles"
compiles

Is there anything else i need to reproduce this?

Confusingly, using the godbolt link provided by @am11 , i can see that even on current main the warnings are emitted for i386, but not for s390x or x86_64. In fact, i can't seem to be able to reproduce this issue with any clang version on s390x.

OP's example
@am11's example on s390x