[clang codegen] Assume arguments of __atomic_* are aligned.

[efriedma]

Summary

This seems weird, but it's necessary to match gcc, and avoid unnecessary calls to libatomic from libstdc++ <atomic>.

Fixes https://github.com/llvm/llvm-project/issues/54790 .

[jyknight]

I disagree with this on principle -- IMO, it is basically a historical bug in GCC that it ignores the type alignment, and we should NOT try to match that -- it's dangerous.

We ought to resolve the bug via other fixes:

• As a workaround: add alignas(uint64_t) to the affected struct in lld. (is GHashCell the only one?)
• Fix __builtin_addressof in Clang to propagate alignment in the same way & does (they ought to act equivalently; I think a new stanza in CodeGenFunction::EmitPointerWithAlignment will fix that).
• Ask GCC to modify libstdc++ so that __builtin_addressof is called directly, instead of going through std::__addressof.

In addition to not liking the intended change, I think this implementation is wrong -- and will be treacherous to attempt to fix -- because I'm pretty sure GCC's implementation doesn't assume particular alignments in general; if it decides it _cannot_ emit inline atomics (based on the size and target's configuration), then no extra alignment is assumed. (And note that Clang's idea of MaxPromoteWidth is unrelated to which sizes of atomics GCC inlines.)

[efriedma]

I disagree with this on principle -- IMO, it is basically a historical bug in GCC that it ignores the type alignment, and we should NOT try to match that -- it's dangerous.

gcc has always behaved this way, and I don't see any indication they intend to change it. I mean, you can call it a bug, but at the end of the day the bug reports will land in our bugtracker, not gcc's.

As a workaround: add alignas(uint64_t) to the affected struct in lld. (is GHashCell the only one?)

I think that's the only one, at least according to git grep std::atomic; I guess that works. (Assuming you meant alignas(sizeof(uint64_t)).)

Ask GCC to modify libstdc++ so that __builtin_addressof is called directly, instead of going through std::__addressof.

Even if gcc did this today, it would take years to reach people on Linux.

I guess this is motivation to implement namespaced builtins...?

[xbolva00]

IMO, it is basically a historical bug in GCC that it ignores the type alignment

Do you have any comments related to this issue by gcc devs that this is a "known" bug?

cc @jakubjelinek @msebor @jwakely

[rjmccall]

The GCC documentation really ought to specify what alignment assumptions these functions make, if any. But they control the specification here, so if they're making alignment assumptions, then I agree we probably ought to make the same assumptions.

Have we verified that this is the rule that GCC uses? Is it true even if e.g. the pointer expression is the address of a variable with a known alignment, or if the pointer has an explicitly-aligned type (e.g. with an aligned typedef)?

[efriedma]

Have we verified that this is the rule that GCC uses? Is it true even if e.g. the pointer expression is the address of a variable with a known alignment, or if the pointer has an explicitly-aligned type (e.g. with an aligned typedef)?

As far as I can tell, if the type's size is a power of two, and there's an inline atomic sequence available, gcc will use an inline sequence that assumes the address is naturally aligned. Otherwise, it makes the libcall. I've tried a variety of ways of writing the operation; alignment doesn't matter at all, no matter what the alignment actually is.

Oddly, __atomic_is_lock_free/__atomic_always_lock_free do care about the alignment, though.

[jyknight]

In D123642#3450110, @efriedma wrote:

I disagree with this on principle -- IMO, it is basically a historical bug in GCC that it ignores the type alignment, and we should NOT try to match that -- it's dangerous.

gcc has always behaved this way, and I don't see any indication they intend to change it. I mean, you can call it a bug, but at the end of the day the bug reports will land in our bugtracker, not gcc's.

Have we had many other such bug reports?

On the other hand, I have seen many cases where people wrote code using the __atomic_* APIs, and pass arguments which are underaligned on some platforms (though not the one the code was developed on). Having it be silently non-atomic (which is what typically happens with misaligned atomic instructions) is just...really nasty.

Ask GCC to modify libstdc++ so that __builtin_addressof is called directly, instead of going through std::__addressof.

Even if gcc did this today, it would take years to reach people on Linux.

True, but the behavior in the meantime is correct. And given the apparent lack of widespread issues, I'm not sure it much matters.

[rsmith]

In D123642#3450129, @xbolva00 wrote:

Do you have any comments related to this issue by gcc devs that this is a "known" bug?

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87237#c1, from someone in the GCC MAINTAINERS file, suggests that this is either a GCC bug or a GCC documentation bug.

[efriedma]

Opened https://github.com/llvm/llvm-project/issues/54963 to more specifically track the clang issue.

With b27430f, it should be easy enough to special-case calls to std::__addressof in CodeGenFunction::EmitPointerWithAlignment, so we can just do that, I guess.

clang/lib/CodeGen/CGAtomic.cpp

if (E->getOp() == AtomicExpr::AO__c11_atomic_init ||
     LValue lvalue = MakeAddrLValue(Ptr, AtomicTy);
     EmitAtomicInit(E->getVal1(), lvalue);
     return RValue::get(nullptr);
   }
 
   auto TInfo = getContext().getTypeInfoInChars(AtomicTy);
   uint64_t Size = TInfo.Width.getQuantity();
   unsigned MaxInlineWidthInBits = getTarget().getMaxAtomicInlineWidth();
+  unsigned MaxPromoteWidthInBits = getTarget().getMaxAtomicPromoteWidth();
+
+  switch (E->getOp()) {
+  case AtomicExpr::AO__atomic_load_n:
+  case AtomicExpr::AO__atomic_load:
+  case AtomicExpr::AO__atomic_store_n:
+  case AtomicExpr::AO__atomic_store:
+  case AtomicExpr::AO__atomic_exchange_n:
+  case AtomicExpr::AO__atomic_exchange:
+  case AtomicExpr::AO__atomic_compare_exchange_n:
+  case AtomicExpr::AO__atomic_compare_exchange:
+  case AtomicExpr::AO__atomic_fetch_add:
+  case AtomicExpr::AO__atomic_fetch_sub:
+  case AtomicExpr::AO__atomic_add_fetch:
+  case AtomicExpr::AO__atomic_sub_fetch:
+  case AtomicExpr::AO__atomic_fetch_and:
+  case AtomicExpr::AO__atomic_fetch_or:
+  case AtomicExpr::AO__atomic_fetch_xor:
+  case AtomicExpr::AO__atomic_fetch_nand:
+  case AtomicExpr::AO__atomic_and_fetch:
+  case AtomicExpr::AO__atomic_or_fetch:
+  case AtomicExpr::AO__atomic_xor_fetch:
+  case AtomicExpr::AO__atomic_nand_fetch:
+  case AtomicExpr::AO__atomic_max_fetch:
+  case AtomicExpr::AO__atomic_min_fetch:
+  case AtomicExpr::AO__atomic_fetch_max:
+  case AtomicExpr::AO__atomic_fetch_min:
+    // According to gcc, if an __atomic_* builtin is inlineable, we should
+    // assume the value is naturally aligned. This isn't documented anywhere,
+    // but it follows from the generated assembly.
+    //
+    // (In other cases, it's hard to tell what gcc expects.)
+    if (TInfo.Width.isPowerOfTwo() && Ptr.getAlignment() < TInfo.Width &&
+        getContext().toBits(TInfo.Width) <= MaxPromoteWidthInBits)
+      Ptr = Ptr.withAlignment(TInfo.Width);
+    break;
+  default:
+    break;
+  }
 
   bool Oversized = getContext().toBits(TInfo.Width) > MaxInlineWidthInBits;
   bool Misaligned = (Ptr.getAlignment() % TInfo.Width) != 0;
   bool UseLibcall = Misaligned | Oversized;
   bool ShouldCastToIntPtrTy = true;
 
   CharUnits MaxInlineWidth =
       getContext().toCharUnitsFromBits(MaxInlineWidthInBits);

clang/test/CodeGen/atomic-ops.c

struct S implicit_load(_Atomic(struct S) *a) {
   return *a;
 }
 
 struct S fd1(struct S *a) {
   // CHECK-LABEL: @fd1
   // CHECK: [[RETVAL:%.*]] = alloca %struct.S, align 4
   // CHECK: [[A:%.*]]   = bitcast %struct.S* {{.*}} to i64*
   // CHECK: [[CAST:%.*]]  = bitcast %struct.S* [[RETVAL]] to i64*
-  // CHECK: [[SRC:%.*]]  = bitcast i64* [[A]] to i8*
-  // CHECK: [[DEST:%.*]]  = bitcast i64* [[CAST]] to i8*
-  // CHECK: call void @__atomic_load(i32 noundef 8, i8* noundef [[SRC]], i8* noundef [[DEST]], i32 noundef 5)
+  // CHECK: load atomic i64, i64* [[A]]
+  // CHECK: store i64 {{.*}}, i64* [[CAST]]
   // CHECK: ret
   struct S ret;
   __atomic_load(a, &ret, memory_order_seq_cst);
   return ret;
 }
 
 void fd2(struct S *a, struct S *b) {
   // CHECK-LABEL: @fd2
   // CHECK:      [[A_ADDR:%.*]] = alloca %struct.S*, align 4
   // CHECK-NEXT: [[B_ADDR:%.*]] = alloca %struct.S*, align 4
   // CHECK-NEXT: store %struct.S* %a, %struct.S** [[A_ADDR]], align 4
   // CHECK-NEXT: store %struct.S* %b, %struct.S** [[B_ADDR]], align 4
   // CHECK-NEXT: [[LOAD_A_PTR:%.*]] = load %struct.S*, %struct.S** [[A_ADDR]], align 4
   // CHECK-NEXT: [[LOAD_B_PTR:%.*]] = load %struct.S*, %struct.S** [[B_ADDR]], align 4
   // CHECK-NEXT: [[COERCED_A_TMP:%.*]] = bitcast %struct.S* [[LOAD_A_PTR]] to i64*
   // CHECK-NEXT: [[COERCED_B:%.*]] = bitcast %struct.S* [[LOAD_B_PTR]] to i64*
-  // CHECK-NEXT: [[COERCED_A:%.*]] = bitcast i64* [[COERCED_A_TMP]] to i8*
-  // CHECK-NEXT: [[CAST_B:%.*]] = bitcast i64* [[COERCED_B]] to i8*
-  // CHECK-NEXT: call void @__atomic_store(i32 noundef 8, i8* noundef [[COERCED_A]], i8* noundef [[CAST_B]],
+  // CHECK-NEXT: load i64, i64* [[COERCED_B]]
+  // CHECK-NEXT: store atomic i64 {{.*}}, i64* [[COERCED_A_TMP]]
   // CHECK-NEXT: ret void
   __atomic_store(a, b, memory_order_seq_cst);
 }
 
 void fd3(struct S *a, struct S *b, struct S *c) {
   // CHECK-LABEL: @fd3
   // CHECK:      [[A_ADDR:%.*]] = alloca %struct.S*, align 4
   // CHECK-NEXT: [[B_ADDR:%.*]] = alloca %struct.S*, align 4
   // CHECK-NEXT: [[C_ADDR:%.*]] = alloca %struct.S*, align 4
   // CHECK-NEXT: store %struct.S* %a, %struct.S** [[A_ADDR]], align 4
   // CHECK-NEXT: store %struct.S* %b, %struct.S** [[B_ADDR]], align 4
   // CHECK-NEXT: store %struct.S* %c, %struct.S** [[C_ADDR]], align 4
   // CHECK-NEXT: [[LOAD_A_PTR:%.*]] = load %struct.S*, %struct.S** [[A_ADDR]], align 4
   // CHECK-NEXT: [[LOAD_B_PTR:%.*]] = load %struct.S*, %struct.S** [[B_ADDR]], align 4
   // CHECK-NEXT: [[LOAD_C_PTR:%.*]] = load %struct.S*, %struct.S** [[C_ADDR]], align 4
   // CHECK-NEXT: [[COERCED_A_TMP:%.*]] = bitcast %struct.S* [[LOAD_A_PTR]] to i64*
   // CHECK-NEXT: [[COERCED_B:%.*]] = bitcast %struct.S* [[LOAD_B_PTR]] to i64*
   // CHECK-NEXT: [[COERCED_C:%.*]] = bitcast %struct.S* [[LOAD_C_PTR]] to i64*
-  // CHECK-NEXT: [[COERCED_A:%.*]] = bitcast i64* [[COERCED_A_TMP]] to i8*
-  // CHECK-NEXT: [[CAST_B:%.*]] = bitcast i64* [[COERCED_B]] to i8*
-  // CHECK-NEXT: [[CAST_C:%.*]] = bitcast i64* [[COERCED_C]] to i8*
-  // CHECK-NEXT: call void @__atomic_exchange(i32 noundef 8, i8* noundef [[COERCED_A]], i8* noundef [[CAST_B]], i8* noundef [[CAST_C]],
-
+  // CHECK-NEXT: load i64, i64* [[COERCED_B]]
+  // CHECK-NEXT: atomicrmw xchg i64* [[COERCED_A_TMP]]
   __atomic_exchange(a, b, c, memory_order_seq_cst);
 }
 
 _Bool fd4(struct S *a, struct S *b, struct S *c) {
   // CHECK-LABEL: @fd4
   // CHECK:      [[A_ADDR:%.*]] = alloca %struct.S*, align 4
   // CHECK-NEXT: [[B_ADDR:%.*]] = alloca %struct.S*, align 4
   // CHECK-NEXT: [[C_ADDR:%.*]] = alloca %struct.S*, align 4
   // CHECK:      store %struct.S* %a, %struct.S** [[A_ADDR]], align 4
   // CHECK-NEXT: store %struct.S* %b, %struct.S** [[B_ADDR]], align 4
   // CHECK-NEXT: store %struct.S* %c, %struct.S** [[C_ADDR]], align 4
   // CHECK-NEXT: [[LOAD_A_PTR:%.*]] = load %struct.S*, %struct.S** [[A_ADDR]], align 4
   // CHECK-NEXT: [[LOAD_B_PTR:%.*]] = load %struct.S*, %struct.S** [[B_ADDR]], align 4
   // CHECK-NEXT: [[LOAD_C_PTR:%.*]] = load %struct.S*, %struct.S** [[C_ADDR]], align 4
   // CHECK-NEXT: [[COERCED_A_TMP:%.*]] = bitcast %struct.S* [[LOAD_A_PTR]] to i64*
   // CHECK-NEXT: [[COERCED_B_TMP:%.*]] = bitcast %struct.S* [[LOAD_B_PTR]] to i64*
   // CHECK-NEXT: [[COERCED_C:%.*]] = bitcast %struct.S* [[LOAD_C_PTR]] to i64*
-  // CHECK-NEXT: [[COERCED_A:%.*]] = bitcast i64* [[COERCED_A_TMP]] to i8*
-  // CHECK-NEXT: [[COERCED_B:%.*]] = bitcast i64* [[COERCED_B_TMP]] to i8*
-  // CHECK-NEXT: [[CAST_C:%.*]] = bitcast i64* [[COERCED_C]] to i8*
-  // CHECK-NEXT: [[CALL:%.*]] = call zeroext i1 @__atomic_compare_exchange(i32 noundef 8, i8* noundef [[COERCED_A]], i8* noundef [[COERCED_B]], i8* noundef [[CAST_C]],
-  // CHECK-NEXT: ret i1 [[CALL]]
+  // CHECK-NEXT: load i64
+  // CHECK-NEXT: load i64
+  // CHECK-NEXT: cmpxchg weak i64
   return __atomic_compare_exchange(a, b, c, 1, 5, 5);
 }
 
 int* fp1(_Atomic(int*) *p) {
   // CHECK-LABEL: @fp1
   // CHECK: load atomic i32, i32* {{.*}} seq_cst, align 4
   return __c11_atomic_load(p, memory_order_seq_cst);
 }
int PR17306_2(volatile int *i, int value) {
   // CHECK-NEXT: ret i32 %[[retval]]
   return __atomic_add_fetch(i, value, memory_order_seq_cst);
 }
 
 void test_underaligned(void) {
   // CHECK-LABEL: @test_underaligned
   struct Underaligned { char c[8]; } underaligned_a, underaligned_b, underaligned_c;
 
-  // CHECK: call void @__atomic_load(i32 noundef 8,
+  // CHECK: load atomic i64, {{.*}}, align 8
   __atomic_load(&underaligned_a, &underaligned_b, memory_order_seq_cst);
-  // CHECK: call void @__atomic_store(i32 noundef 8,
+  // CHECK: store atomic i64 {{.*}}, align 8
   __atomic_store(&underaligned_a, &underaligned_b, memory_order_seq_cst);
-  // CHECK: call void @__atomic_exchange(i32 noundef 8,
+  // CHECK: atomicrmw xchg i64* {{.*}}, align 8
   __atomic_exchange(&underaligned_a, &underaligned_b, &underaligned_c, memory_order_seq_cst);
-  // CHECK: call {{.*}} @__atomic_compare_exchange(i32 noundef 8,
+  // CHECK: cmpxchg weak i64* {{.*}}, align 8
   __atomic_compare_exchange(&underaligned_a, &underaligned_b, &underaligned_c, 1, memory_order_seq_cst, memory_order_seq_cst);
 
   __attribute__((aligned)) struct Underaligned aligned_a, aligned_b, aligned_c;
 
   // CHECK: load atomic i64, {{.*}}, align 16
   __atomic_load(&aligned_a, &aligned_b, memory_order_seq_cst);
   // CHECK: store atomic i64 {{.*}}, align 16
   __atomic_store(&aligned_a, &aligned_b, memory_order_seq_cst);
void test_minmax_postop(int *si, unsigned *ui, unsigned short *us, signed char *sc, unsigned long long *ull) {
   *us = __atomic_min_fetch(us, 42, memory_order_release);
 
   // CHECK: [[OLD:%.*]] = atomicrmw min i8* [[PTR:%.*]], i8 [[RHS:%.*]] release, align 1
   // CHECK: [[TST:%.*]] = icmp slt i8 [[OLD]], [[RHS]]
   // CHECK: [[NEW:%.*]] = select i1 [[TST]], i8 [[OLD]], i8 [[RHS]]
   // CHECK: store i8 [[NEW]], i8*
   *sc = __atomic_min_fetch(sc, 42, memory_order_release);
 
-  // CHECK: [[OLD:%.*]] = call i64 @__atomic_fetch_umin_8(i8* noundef {{%.*}}, i64 noundef [[RHS:%.*]],
+  // CHECK: [[OLD:%.*]] = atomicrmw umin i64* [[PTR:%.*]], i64 [[RHS:%.*]] release, align 8
   // CHECK: [[TST:%.*]] = icmp ult i64 [[OLD]], [[RHS]]
   // CHECK: [[NEW:%.*]] = select i1 [[TST]], i64 [[OLD]], i64 [[RHS]]
   // CHECK: store i64 [[NEW]], i64*
   *ull = __atomic_min_fetch(ull, 42, memory_order_release);
 
 }
 
 #endif

clang/test/CodeGen/atomics-sema-alignment.c

 // RUN: %clang_cc1 -triple aarch64-linux-gnu %s -emit-llvm -o /dev/null -verify
 
 typedef struct {
   int a, b;
 } IntPair;
 
 typedef struct {
   long long a;
 } LongStruct;
 
 typedef int __attribute__((aligned(1))) unaligned_int;
 
 void func(IntPair *p) {
   IntPair res;
-  __atomic_load(p, &res, 0);                    // expected-warning {{misaligned atomic operation may incur significant performance penalty; the expected alignment (8 bytes) exceeds the actual alignment (4 bytes)}}
-  __atomic_store(p, &res, 0);                   // expected-warning {{misaligned atomic operation may incur significant performance penalty; the expected alignment (8 bytes) exceeds the actual alignment (4 bytes)}}
-  __atomic_fetch_add((unaligned_int *)p, 1, 2); // expected-warning {{misaligned atomic operation may incur significant performance penalty; the expected alignment (4 bytes) exceeds the actual alignment (1 bytes)}}
-  __atomic_fetch_sub((unaligned_int *)p, 1, 3); // expected-warning {{misaligned atomic operation may incur significant performance penalty; the expected alignment (4 bytes) exceeds the actual alignment (1 bytes)}}
+  __atomic_load(p, &res, 0);
+  __atomic_store(p, &res, 0);
+  __atomic_fetch_add((unaligned_int *)p, 1, 2);
+  __atomic_fetch_sub((unaligned_int *)p, 1, 3);
 }
 
 void func1(LongStruct *p) {
   LongStruct res;
   __atomic_load(p, &res, 0);
   __atomic_store(p, &res, 0);
   __atomic_fetch_add((int *)p, 1, 2);
   __atomic_fetch_sub((int *)p, 1, 3);
 }
 
 typedef struct {
   void *a;
   void *b;
 } Foo;
 
 typedef struct {
   void *a;
   void *b;
   void *c;
   void *d;
 } __attribute__((aligned(32))) ThirtyTwo;
 
 void braz(Foo *foo, ThirtyTwo *braz) {
   Foo bar;
-  __atomic_load(foo, &bar, __ATOMIC_RELAXED); // expected-warning {{misaligned atomic operation may incur significant performance penalty; the expected alignment (16 bytes) exceeds the actual alignment (8 bytes)}}
+  __atomic_load(foo, &bar, __ATOMIC_RELAXED);
 
   ThirtyTwo thirtyTwo1;
   ThirtyTwo thirtyTwo2;
   __atomic_load(&thirtyTwo1, &thirtyTwo2, __ATOMIC_RELAXED); // expected-warning {{large atomic operation may incur significant performance penalty; the access size (32 bytes) exceeds the max lock-free size (16  bytes)}}
 }