[Atomics] warn about misaligned atomic accesses using libcalls

[t.p.northover]

Summary

If an atomic variable is misaligned Clang will emit accesses to it as a libcall. These calls are likely to be slow and involve locks; they are almost certainly not what the developer intended. So this patch adds a warning (promotable or ignorable) for tat situation.

[t.p.northover]

Ping.

[compnerd]

Sorry for the delay, didn't see the changes earlier.

clang/lib/CodeGen/CGAtomic.cpp

886

It is kinda unfortunate that you need to look up 125 lines to get the context that the call here is implied by a lack of alignment. Perhaps we can rename UseLibcall to UnsuitableAligned or something?

[t.p.northover]

clang/lib/CodeGen/CGAtomic.cpp

886

I'd be OK with that, or I could just move the diagnostic further up so it is next to the definition (or maybe just after to avoid the atomic_init case)?

[compnerd]

clang/lib/CodeGen/CGAtomic.cpp

886

That addresses my concern too, the choice is yours :-)

[t.p.northover]

Moved diagnostic emission next to where the decision is made.

[t.p.northover]

Ah, I see you approved it before, presumably with the suggested changes. I've committed as r330566.

[rsmith]

There's still something wrong here: in the cases where we produce these warnings, we still produce calls to __atomic_*_<n> library functions. Those functions are specified to only work on properly-aligned inputs, so that's wrong. And there seems to be a problem with alignment in compiler-rt too: the "unoptimized" __atomic_load_n implementation only looks at the size and not the alignment of the pointer when determining whether to use a lock.

See also PR38846.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_atomic_op_bitwise_needs_atomic_int : Error<
   "address argument to bitwise atomic operation must be a pointer to "
   "%select{|atomic }0integer (%1 invalid)">;
 def warn_atomic_op_has_invalid_memory_order : Warning<
   "memory order argument to atomic operation is invalid">,
   InGroup<DiagGroup<"atomic-memory-ordering">>;
 def err_atomic_op_has_invalid_synch_scope : Error<
   "synchronization scope argument to atomic operation is invalid">;
+def warn_atomic_op_misaligned : Warning<
+  "misaligned or large atomic operation may incur significant performance penalty">,
+  InGroup<DiagGroup<"atomic-alignment">>;
 
 def err_overflow_builtin_must_be_int : Error<
   "operand argument to overflow builtin must be an integer (%0 invalid)">;
 def err_overflow_builtin_must_be_ptr_int : Error<
   "result argument to overflow builtin must be a pointer "
   "to a non-const integer (%0 invalid)">;
 
 def err_atomic_load_store_uses_lib : Error<

clang/lib/CodeGen/CGAtomic.cpp

 
 #include "CGCall.h"
 #include "CGRecordLayout.h"
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
+#include "clang/Sema/SemaDiagnostic.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Operator.h"
 
 using namespace clang;
 using namespace CodeGen;
 
if (const AtomicType *AT = AtomicTy->getAs<AtomicType>())
     MemTy = AT->getValueType();
   llvm::Value *IsWeak = nullptr, *OrderFail = nullptr;
 
   Address Val1 = Address::invalid();
   Address Val2 = Address::invalid();
   Address Dest = Address::invalid();
   Address Ptr = EmitPointerWithAlignment(E->getPtr());
 
+  if (E->getOp() == AtomicExpr::AO__c11_atomic_init ||
+      E->getOp() == AtomicExpr::AO__opencl_atomic_init) {
+    LValue lvalue = MakeAddrLValue(Ptr, AtomicTy);
+    EmitAtomicInit(E->getVal1(), lvalue);
+    return RValue::get(nullptr);
+  }
+
   CharUnits sizeChars, alignChars;
   std::tie(sizeChars, alignChars) = getContext().getTypeInfoInChars(AtomicTy);
   uint64_t Size = sizeChars.getQuantity();
   unsigned MaxInlineWidthInBits = getTarget().getMaxAtomicInlineWidth();
   bool UseLibcall = ((Ptr.getAlignment() % sizeChars) != 0 ||
                      getContext().toBits(sizeChars) > MaxInlineWidthInBits);
 
-  if (E->getOp() == AtomicExpr::AO__c11_atomic_init ||
-      E->getOp() == AtomicExpr::AO__opencl_atomic_init) {
-    LValue lvalue = MakeAddrLValue(Ptr, AtomicTy);
-    EmitAtomicInit(E->getVal1(), lvalue);
-    return RValue::get(nullptr);
-  }
+  if (UseLibcall)
+    CGM.getDiags().Report(E->getLocStart(), diag::warn_atomic_op_misaligned);
 
   llvm::Value *Order = EmitScalarExpr(E->getOrder());
   llvm::Value *Scope =
       E->getScopeModel() ? EmitScalarExpr(E->getScope()) : nullptr;
 
   switch (E->getOp()) {
   case AtomicExpr::AO__c11_atomic_init:
   case AtomicExpr::AO__opencl_atomic_init:
if (Dest.isValid())
     Dest = Atomics.emitCastToAtomicIntPointer(Dest);
   else if (E->isCmpXChg())
     Dest = CreateMemTemp(RValTy, "cmpxchg.bool");
   else if (!RValTy->isVoidType())
     Dest = Atomics.emitCastToAtomicIntPointer(Atomics.CreateTempAlloca());
 
   // Use a library call.  See: http://gcc.gnu.org/wiki/Atomic/GCCMM/LIbrary .
   if (UseLibcall) {
     bool UseOptimizedLibcall = false;
     switch (E->getOp()) {
     case AtomicExpr::AO__c11_atomic_init:
     case AtomicExpr::AO__opencl_atomic_init:
       llvm_unreachable("Already handled above with EmitAtomicInit!");
 
     case AtomicExpr::AO__c11_atomic_fetch_add:
     case AtomicExpr::AO__opencl_atomic_fetch_add:
     case AtomicExpr::AO__atomic_fetch_add:

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 or large atomic operation may incur significant performance penalty}}
+  __atomic_store(p, &res, 0); // expected-warning {{misaligned or large atomic operation may incur significant performance penalty}}
+  __atomic_fetch_add((unaligned_int *)p, 1, 2); // expected-warning {{misaligned or large atomic operation may incur significant performance penalty}}
+  __atomic_fetch_sub((unaligned_int *)p, 1, 3); // expected-warning {{misaligned or large atomic operation may incur significant performance penalty}}
+}
+
+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);
+}