[ClangFE] Check that __sync builtins are naturally aligned.

[jonpa]

Summary

On SystemZ, int128 is aligned to only 8 bytes per the ABI, while 128 bit atomic ISA instructions exist with the requirement of alignment to 16 bytes. "sync" builtins in Clang are however always emitted as atomicrmw instructions which require a natural alignment of the address. This could then result in the situation where the value is aligned to only 8 bytes but an atomicrmw is emitted with an added incorrect alignment value of 16.

This patch checks that all values used with __sync builtins are naturally aligned, or else an error message is emitted.

I first made this a SystemZ specific check for 16 byte values, but it makes sense generally as well, so I changed that patch to always do the check. This is done at the point of emission (CGBuiltin.cpp) in order to be able to inspect the alignment of the value.

[arsenm]

clang/test/CodeGen/SystemZ/sync-builtins-i128-8Al-04.c

6 ↗

(On Diff #496683)

Belongs in test/Sema?

[efriedma]

Making this an error seems unfriendly. The user's code might be working correctly if they ensure the alignment some other way. I guess in some cases, we can prove that the alignment of the address is actually insufficient, but in most cases, we'll just see a pointer we can't analyze, so we don't really know anything about the alignment.

Depending on the alignment computed by EmitPointerWithAlignment() means the diagnostic can change based on CodeGen implementation details we don't really make any promises about.

[jyknight]

(Firstly, really nobody should be using the __sync_* builtins...)

As efriedma says, I don't think this can be an error, we don't know for sure that the pointer IS misaligned -- it just isn't guaranteed to be aligned.

I think it would be OK to make a warning, though. We already have an on-by-default warning for __atomic family, -Watomic-alignment.

E.g. you get test.c:3:3: warning: misaligned atomic operation may incur significant performance penalty; the expected alignment (8 bytes) exceeds the actual alignment (4 bytes) [-Watomic-alignment] from clang -S -o /dev/null -target i386-linux-gnu test.c given:

void f(double *d) {
  double new = 4.0;
  __atomic_store(d, &new, __ATOMIC_SEQ_CST);
}

[jonpa]

Thanks for review!

Making this a warning instead makes sense to me given your reasoning. Since the default alignment for int128 on SystemZ is only 8 bytes, the situation might be slightly different since we then expect an explicit alignment to 16 bytes in the source code, which should hopefully be available. It's a little worrisome that the atomicrmw instruction always gets the 16 byte alignment even if it is not known to be accurate. A warning would be better than nothing, I suppose.

@uweigand : We could make this an error on SystemZ specifically, or settle for the warning like the other targets...

[uweigand]

I believe the situation between the __sync and __atomic builtins is a bit different, though. For __atomic, if the compiler cannot prove correct alignment, it will emit a library call. This should always do the correct thing, however may have a performance overhead - which is what the warning says.

For __sync, whether the compiler can or cannot prove alignment, it will always emit LLVM IR that pretends the variable is aligned, so the back-end will generate some atomic instruction. If it actually was not aligned after all, this will typically simply crash - so it's not just a matter of performance.

But I guess I'd be fine with just a warning anyway (as long as the warning text doesn't imply this is only a performance issue) - if the user ensures alignment, code will still work.

(I certainly agree everyone should be using __atomic and not __sync.)

[jonpa]

Patch updated to only give a warning.

[jonpa]

clang/test/CodeGen/SystemZ/sync-builtins-i128-8Al-04.c

6 ↗

(On Diff #496683)

Not sure about moving the tests: these checks are done in CodeGen, and the tests for the __atomic alignments are also in CodeGen (CodeGen/atomics-sema-alignment.c).

[jonpa]

ping - everybody happy with this now?

[efriedma]

Usually for this sort of warning, we provide diagnostic notes: how should the user fix their code if it's wrong? How should the user suppress the warning if it's wrong?

I don't really like generating warnings out of CodeGen because they're less consistent; the warning doesn't show up with -fsyntax-only, and could appear/disappear depending on compiler optimizations. Maybe okay if it significantly reduces the false positive rate, though.

[jonpa]

Usually for this sort of warning, we provide diagnostic notes: how should the user fix their code if it's wrong? How should the user suppress the warning if it's wrong?

I added a hint in the warning message to the user that __atomic is the way to go. The warning is enabled with -Wsync-alignment, which is also part of the warning message.

I don't really like generating warnings out of CodeGen because they're less consistent; the warning doesn't show up with -fsyntax-only, and could appear/disappear depending on compiler optimizations. > Maybe okay if it significantly reduces the false positive rate, though.

Since I don't know of any way to actually check the provided alignment in Sema, we would either have to warn all the time if moving the check there, or keep it in CodeGen the way it is...

[jonpa]

ping!

[jonpa]

Ping - does it look ok to emit a warning like this from CodeGen?

[efriedma]

I guess it's not any worse than the existing -Watomic-alignment. LGTM.

(I think we don't currently have any utility that tries to infer the alignment of a pointer in Sema, but we might consider adding one at some point...)

[srj]

(I certainly agree everyone should be using atomic and not sync.)

Halide still uses the __sync builtins for 32-bit targets, for reasons I don't have at hand now (IIRC there were downstream users with tooling issues, I will re-check). (I don't think we are using __sync for 64-bit targets; if we are, that's a mistake on our part.)

[ahatanak]

I think this patch is causing the assertion in CodeGenFunction::setAddrOfLocalVar to fail when the following code is compiled:

void foo(unsigned long long t) {
  __sync_bool_compare_and_swap(({int x = 1; &t;}), t, t);
}

Could you take a look?

[jonpa]

In D143813#4256969, @ahatanak wrote:

I think this patch is causing the assertion in CodeGenFunction::setAddrOfLocalVar to fail when the following code is compiled:

void foo(unsigned long long t) {
  __sync_bool_compare_and_swap(({int x = 1; &t;}), t, t);
}

Could you take a look?

I don't understand the first argument - I thought it was supposed to be just an address...

This patch seems to (in CheckAtomicAlignment()) call CGF.EmitPointerWithAlignment() on the CompoundStmt, which is unexpected. If this is a legal parameter for the address, I guess there should be some kind of special handling here. Perhaps it would be ok to only emit the warning for plain address paramters, I might think...?

[efriedma]

I think the issue is that you're calling EmitPointerWithAlignment() on the argument, then calling EmitScalarExpr on the same argument. Essentially, emitting the argument twice. If emitting the argument has side-effects, that will cause an issue. Sorry, should have spotted that while reviewing. Should be straightforward to fix, though; just make CheckAtomicAlignment return the computed pointer.

[ahatanak]

In D143813#4257943, @jonpa wrote:

I don't understand the first argument - I thought it was supposed to be just an address...

It's a statement expression. https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html

The value of the last subexpression serves as the value of the entire construct.

[jonpa]

In D143813#4258744, @ahatanak wrote:

In D143813#4257943, @jonpa wrote:

I don't understand the first argument - I thought it was supposed to be just an address...

It's a statement expression. https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html

The value of the last subexpression serves as the value of the entire construct.

Ah, ok, thanks!

[jonpa]

Should be straightforward to fix, though; just make CheckAtomicAlignment return the computed pointer.

Something like this?

[efriedma]

Please don't upload reviews for followup patches on top of the original patch; it confuses the history of happened when. But sure, looks fine.

[jonpa]

In D143813#4269073, @efriedma wrote:

Please don't upload reviews for followup patches on top of the original patch; it confuses the history of happened when. But sure, looks fine.

Sorry - moved this last follow-up diff to https://reviews.llvm.org/D148422.

[tstellar]

This introduced a build failure on s390x: https://github.com/llvm/llvm-project/issues/69146

clang/lib/CodeGen/CGBuiltin.cpp

static Value *EmitFromInt(CodeGenFunction &CGF, llvm::Value *V,
 
   if (ResultType->isPointerTy())
     return CGF.Builder.CreateIntToPtr(V, ResultType);
 
   assert(V->getType() == ResultType);
   return V;
 }
 
+static llvm::Value *CheckAtomicAlignment(CodeGenFunction &CGF,
+                                         const CallExpr *E) {
+  ASTContext &Ctx = CGF.getContext();
+  Address Ptr = CGF.EmitPointerWithAlignment(E->getArg(0));
+  unsigned Bytes = Ptr.getElementType()->isPointerTy()
+                       ? Ctx.getTypeSizeInChars(Ctx.VoidPtrTy).getQuantity()
+                       : Ptr.getElementType()->getScalarSizeInBits() / 8;
+  unsigned Align = Ptr.getAlignment().getQuantity();
+  if (Align % Bytes != 0) {
+    DiagnosticsEngine &Diags = CGF.CGM.getDiags();
+    Diags.Report(E->getBeginLoc(), diag::warn_sync_op_misaligned);
+  }
+  return Ptr.getPointer();
+}
+
 /// Utility to insert an atomic instruction based on Intrinsic::ID
 /// and the expression node.
 static Value *MakeBinaryAtomicValue(
     CodeGenFunction &CGF, llvm::AtomicRMWInst::BinOp Kind, const CallExpr *E,
     AtomicOrdering Ordering = AtomicOrdering::SequentiallyConsistent) {
 
   QualType T = E->getType();
   assert(E->getArg(0)->getType()->isPointerType());
   assert(CGF.getContext().hasSameUnqualifiedType(T,
                                   E->getArg(0)->getType()->getPointeeType()));
   assert(CGF.getContext().hasSameUnqualifiedType(T, E->getArg(1)->getType()));
 
-  llvm::Value *DestPtr = CGF.EmitScalarExpr(E->getArg(0));
+  llvm::Value *DestPtr = CheckAtomicAlignment(CGF, E);
   unsigned AddrSpace = DestPtr->getType()->getPointerAddressSpace();
 
   llvm::IntegerType *IntType =
     llvm::IntegerType::get(CGF.getLLVMContext(),
                            CGF.getContext().getTypeSize(T));
   llvm::Type *IntPtrType = IntType->getPointerTo(AddrSpace);
 
   llvm::Value *Args[2];

 static Value *EmitNontemporalLoad(CodeGenFunction &CGF, const CallExpr *E) {
   Value *Address = CGF.EmitScalarExpr(E->getArg(0));
 
   LValue LV = CGF.MakeNaturalAlignAddrLValue(Address, E->getType());
   LV.setNontemporal(true);
   return CGF.EmitLoadOfScalar(LV, E->getExprLoc());
 }
 
-static void CheckAtomicAlignment(CodeGenFunction &CGF, const CallExpr *E) {
-  ASTContext &Ctx = CGF.getContext();
-  Address Ptr = CGF.EmitPointerWithAlignment(E->getArg(0));
-  unsigned Bytes = Ptr.getElementType()->isPointerTy()
-                       ? Ctx.getTypeSizeInChars(Ctx.VoidPtrTy).getQuantity()
-                       : Ptr.getElementType()->getScalarSizeInBits() / 8;
-  unsigned Align = Ptr.getAlignment().getQuantity();
-  if (Align % Bytes != 0) {
-    DiagnosticsEngine &Diags = CGF.CGM.getDiags();
-    Diags.Report(E->getBeginLoc(), diag::warn_sync_op_misaligned);
-  }
-}
-
 static RValue EmitBinaryAtomic(CodeGenFunction &CGF,
                                llvm::AtomicRMWInst::BinOp Kind,
                                const CallExpr *E) {
-  CheckAtomicAlignment(CGF, E);
   return RValue::get(MakeBinaryAtomicValue(CGF, Kind, E));
 }
 
 /// Utility to insert an atomic instruction based Intrinsic::ID and
 /// the expression node, where the return value is the result of the
 /// operation.
 static RValue EmitBinaryAtomicPost(CodeGenFunction &CGF,
                                    llvm::AtomicRMWInst::BinOp Kind,
                                    const CallExpr *E,
                                    Instruction::BinaryOps Op,
                                    bool Invert = false) {
-  CheckAtomicAlignment(CGF, E);
   QualType T = E->getType();
   assert(E->getArg(0)->getType()->isPointerType());
   assert(CGF.getContext().hasSameUnqualifiedType(T,
                                   E->getArg(0)->getType()->getPointeeType()));
   assert(CGF.getContext().hasSameUnqualifiedType(T, E->getArg(1)->getType()));
 
-  llvm::Value *DestPtr = CGF.EmitScalarExpr(E->getArg(0));
+  llvm::Value *DestPtr = CheckAtomicAlignment(CGF, E);
   unsigned AddrSpace = DestPtr->getType()->getPointerAddressSpace();
 
   llvm::IntegerType *IntType =
     llvm::IntegerType::get(CGF.getLLVMContext(),
                            CGF.getContext().getTypeSize(T));
   llvm::Type *IntPtrType = IntType->getPointerTo(AddrSpace);
 
   llvm::Value *Args[2];

 ///                   cmpxchg result or the old value.
 ///
 /// @returns result of cmpxchg, according to ReturnBool
 ///
 /// Note: In order to lower Microsoft's _InterlockedCompareExchange* intrinsics
 /// invoke the function EmitAtomicCmpXchgForMSIntrin.
 static Value *MakeAtomicCmpXchgValue(CodeGenFunction &CGF, const CallExpr *E,
                                      bool ReturnBool) {
-  CheckAtomicAlignment(CGF, E);
   QualType T = ReturnBool ? E->getArg(1)->getType() : E->getType();
-  llvm::Value *DestPtr = CGF.EmitScalarExpr(E->getArg(0));
+  llvm::Value *DestPtr = CheckAtomicAlignment(CGF, E);
   unsigned AddrSpace = DestPtr->getType()->getPointerAddressSpace();
 
   llvm::IntegerType *IntType = llvm::IntegerType::get(
       CGF.getLLVMContext(), CGF.getContext().getTypeSize(T));
   llvm::Type *IntPtrType = IntType->getPointerTo(AddrSpace);
 
   Value *Args[3];
   Args[0] = CGF.Builder.CreateBitCast(DestPtr, IntPtrType);

   case Builtin::BI__sync_lock_test_and_set_16:
     return EmitBinaryAtomic(*this, llvm::AtomicRMWInst::Xchg, E);
 
   case Builtin::BI__sync_lock_release_1:
   case Builtin::BI__sync_lock_release_2:
   case Builtin::BI__sync_lock_release_4:
   case Builtin::BI__sync_lock_release_8:
   case Builtin::BI__sync_lock_release_16: {
-    CheckAtomicAlignment(*this, E);
-    Value *Ptr = EmitScalarExpr(E->getArg(0));
+    Value *Ptr = CheckAtomicAlignment(*this, E);
     QualType ElTy = E->getArg(0)->getType()->getPointeeType();
     CharUnits StoreSize = getContext().getTypeSizeInChars(ElTy);
     llvm::Type *ITy = llvm::IntegerType::get(getLLVMContext(),
                                              StoreSize.getQuantity() * 8);
     Ptr = Builder.CreateBitCast(Ptr, ITy->getPointerTo());
     llvm::StoreInst *Store =
       Builder.CreateAlignedStore(llvm::Constant::getNullValue(ITy), Ptr,
                                  StoreSize);

clang/test/CodeGen/SystemZ/sync-builtins-i128-16Al.c

 // CHECK-NEXT:    [[TMP0:%.*]] = load i128, ptr @Val, align 16, !tbaa [[TBAA2]]
 // CHECK-NEXT:    [[TMP1:%.*]] = atomicrmw xchg ptr @Ptr, i128 [[TMP0]] seq_cst, align 16
 // CHECK-NEXT:    store i128 [[TMP1]], ptr [[AGG_RESULT:%.*]], align 8, !tbaa [[TBAA2]]
 // CHECK-NEXT:    ret void
 //
 __int128 f17() {
   return __sync_swap(&Ptr, Val);
 }
+
+// Test that a statement expression compiles.
+// CHECK-LABEL: @f18(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[T_ADDR:%.*]] = alloca i128, align 8
+// CHECK-NEXT:    [[T:%.*]] = load i128, ptr [[TMP0:%.*]], align 8, !tbaa [[TBAA2]]
+// CHECK-NEXT:    store i128 [[T]], ptr [[T_ADDR]], align 8, !tbaa [[TBAA2]]
+// CHECK-NEXT:    [[TMP1:%.*]] = cmpxchg ptr [[T_ADDR]], i128 [[T]], i128 [[T]] seq_cst seq_cst, align 16
+// CHECK-NEXT:    ret void
+//
+void f18(__int128 t) {
+  __sync_bool_compare_and_swap(({int x = 1; &t;}), t, t);
+}