[CGAtomic] Lift stronger requirements on cmpxch and add support for acquire failure mode

[bruno]

Summary

• Fix emitAtomicCmpXchgFailureSet to support release/acquire (succ/fail) memory order.
• Remove stronger checks for cmpxch.

Implements http://wg21.link/p0418, related discussions in https://groups.google.com/g/thread-sanitizer/c/bTOVyLAo8p4/m/Y-2yks6ZLAAJ

A follow patch should attempt to implement the TSAN side of failure mode support.

[jfb]

Maybe refer to http://wg21.link/p0418 directly in the commit message?

[bruno]

@jfb sure!

Also fix test failures.

[yln]

clang/lib/CodeGen/CGAtomic.cpp

444–447

Should the following assert in AtomicCmpXchgInst::Init() have been changed as well?

assert(!isStrongerThan(FailureOrdering, SuccessOrdering) &&
       "AtomicCmpXchg failure argument shall be no stronger than the success "
       "argument");

https://github.com/llvm/llvm-project/blob/dad5caa59e6b2bde8d6cf5b64a972c393c526c82/llvm/lib/IR/Instructions.cpp#L1561

I am observing a crash on an internal code base most likely caused by this.

[yln]

clang/lib/CodeGen/CGAtomic.cpp

444–447

friendly ping @bruno

[bruno]

@yln sorry, I didn't get notifications for this somehow. This got fixed as part of the more general support in https://github.com/llvm/llvm-project/commit/819e0d105e84c6081cfcfa0e38fd257b6124553a

clang/lib/CodeGen/CGAtomic.cpp

static void emitAtomicCmpXchgFailureSet(CodeGenFunction &CGF, AtomicExpr *E,
   llvm::AtomicOrdering FailureOrder;
   if (llvm::ConstantInt *FO = dyn_cast<llvm::ConstantInt>(FailureOrderVal)) {
     auto FOS = FO->getSExtValue();
     if (!llvm::isValidAtomicOrderingCABI(FOS))
       FailureOrder = llvm::AtomicOrdering::Monotonic;
     else
       switch ((llvm::AtomicOrderingCABI)FOS) {
       case llvm::AtomicOrderingCABI::relaxed:
+      // 31.7.2.18: "The failure argument shall not be memory_order_release
+      // nor memory_order_acq_rel". Fallback to monotonic.
       case llvm::AtomicOrderingCABI::release:
       case llvm::AtomicOrderingCABI::acq_rel:
         FailureOrder = llvm::AtomicOrdering::Monotonic;
         break;
       case llvm::AtomicOrderingCABI::consume:
       case llvm::AtomicOrderingCABI::acquire:
         FailureOrder = llvm::AtomicOrdering::Acquire;
         break;
       case llvm::AtomicOrderingCABI::seq_cst:
         FailureOrder = llvm::AtomicOrdering::SequentiallyConsistent;
         break;
       }
-    if (isStrongerThan(FailureOrder, SuccessOrder)) {
-      // Don't assert on undefined behavior "failure argument shall be no
-      // stronger than the success argument".
-      FailureOrder =
-          llvm::AtomicCmpXchgInst::getStrongestFailureOrdering(SuccessOrder);
-    }
+    // Prior to c++17, "the failure argument shall be no stronger than the
+    // success argument". This condition has been lifted and the only
+    // precondition is 31.7.2.18. Effectively treat this as a DR and skip
+    // language version checks.
     emitAtomicCmpXchg(CGF, E, IsWeak, Dest, Ptr, Val1, Val2, Size, SuccessOrder,
                       FailureOrder, Scope);
     return;
   }
 
   // Create all the relevant BB's
   llvm::BasicBlock *MonotonicBB = nullptr, *AcquireBB = nullptr,
                    *SeqCstBB = nullptr;
   MonotonicBB = CGF.createBasicBlock("monotonic_fail", CGF.CurFn);
-  if (SuccessOrder != llvm::AtomicOrdering::Monotonic &&
-      SuccessOrder != llvm::AtomicOrdering::Release)
+  if (SuccessOrder != llvm::AtomicOrdering::Monotonic)
     AcquireBB = CGF.createBasicBlock("acquire_fail", CGF.CurFn);
   if (SuccessOrder == llvm::AtomicOrdering::SequentiallyConsistent)
     SeqCstBB = CGF.createBasicBlock("seqcst_fail", CGF.CurFn);
 
   llvm::BasicBlock *ContBB = CGF.createBasicBlock("atomic.continue", CGF.CurFn);
 
   llvm::SwitchInst *SI = CGF.Builder.CreateSwitch(FailureOrderVal, MonotonicBB);
 
   // Emit all the different atomics
 
   // MonotonicBB is arbitrarily chosen as the default case; in practice, this
   // doesn't matter unless someone is crazy enough to use something that
   // doesn't fold to a constant for the ordering.
   CGF.Builder.SetInsertPoint(MonotonicBB);
   emitAtomicCmpXchg(CGF, E, IsWeak, Dest, Ptr, Val1, Val2,
                     Size, SuccessOrder, llvm::AtomicOrdering::Monotonic, Scope);
   CGF.Builder.CreateBr(ContBB);
 
   if (AcquireBB) {
     CGF.Builder.SetInsertPoint(AcquireBB);
     emitAtomicCmpXchg(CGF, E, IsWeak, Dest, Ptr, Val1, Val2,
                       Size, SuccessOrder, llvm::AtomicOrdering::Acquire, Scope);
     CGF.Builder.CreateBr(ContBB);
+    if (SuccessOrder != llvm::AtomicOrdering::Release)
       SI->addCase(CGF.Builder.getInt32((int)llvm::AtomicOrderingCABI::consume),
                   AcquireBB);
     SI->addCase(CGF.Builder.getInt32((int)llvm::AtomicOrderingCABI::acquire),
                 AcquireBB);
   }
   if (SeqCstBB) {
     CGF.Builder.SetInsertPoint(SeqCstBB);
     emitAtomicCmpXchg(CGF, E, IsWeak, Dest, Ptr, Val1, Val2, Size, SuccessOrder,
                       llvm::AtomicOrdering::SequentiallyConsistent, Scope);
     CGF.Builder.CreateBr(ContBB);

clang/test/CodeGen/atomic-ops.c

void generalFailureOrder(_Atomic(int) *ptr, int *ptr2, int success, int fail) {
   // CHECK: [[ACQUIRE]]
   // CHECK: switch {{.*}}, label %[[ACQUIRE_MONOTONIC:[0-9a-zA-Z._]+]] [
   // CHECK-NEXT: i32 1, label %[[ACQUIRE_ACQUIRE:[0-9a-zA-Z._]+]]
   // CHECK-NEXT: i32 2, label %[[ACQUIRE_ACQUIRE:[0-9a-zA-Z._]+]]
   // CHECK-NEXT: ]
 
   // CHECK: [[RELEASE]]
   // CHECK: switch {{.*}}, label %[[RELEASE_MONOTONIC:[0-9a-zA-Z._]+]] [
+  // CHECK-NEXT: i32 2, label %[[RELEASE_ACQUIRE:[0-9a-zA-Z._]+]]
   // CHECK-NEXT: ]
 
   // CHECK: [[ACQREL]]
   // CHECK: switch {{.*}}, label %[[ACQREL_MONOTONIC:[0-9a-zA-Z._]+]] [
   // CHECK-NEXT: i32 1, label %[[ACQREL_ACQUIRE:[0-9a-zA-Z._]+]]
   // CHECK-NEXT: i32 2, label %[[ACQREL_ACQUIRE:[0-9a-zA-Z._]+]]
   // CHECK-NEXT: ]
 
void generalFailureOrder(_Atomic(int) *ptr, int *ptr2, int success, int fail) {
   // CHECK: [[ACQUIRE_MONOTONIC]]
   // CHECK: cmpxchg {{.*}} acquire monotonic, align
   // CHECK: br
 
   // CHECK: [[ACQUIRE_ACQUIRE]]
   // CHECK: cmpxchg {{.*}} acquire acquire, align
   // CHECK: br
 
+  // CHECK: [[RELEASE_MONOTONIC]]
+  // CHECK: cmpxchg {{.*}} release monotonic, align
+  // CHECK: br
+
+  // CHECK: [[RELEASE_ACQUIRE]]
+  // CHECK: cmpxchg {{.*}} release acquire, align
+  // CHECK: br
+
   // CHECK: [[ACQREL_MONOTONIC]]
   // CHECK: cmpxchg {{.*}} acq_rel monotonic, align
   // CHECK: br
 
   // CHECK: [[ACQREL_ACQUIRE]]
   // CHECK: cmpxchg {{.*}} acq_rel acquire, align
   // CHECK: br
 
void generalWeakness(int *ptr, int *ptr2, _Bool weak) {
   // CHECK-NOT: br
   // CHECK: cmpxchg {{.*}} seq_cst seq_cst, align
   // CHECK: br
 
   // CHECK: [[WEAK]]
   // CHECK-NOT: br
   // CHECK: cmpxchg weak {{.*}} seq_cst seq_cst, align
   // CHECK: br
+
+  __atomic_compare_exchange_n(ptr, ptr2, 42, weak, memory_order_release, memory_order_acquire);
+  // CHECK: switch i1 {{.*}}, label %[[WEAK:[0-9a-zA-Z._]+]] [
+  // CHECK-NEXT: i1 false, label %[[STRONG:[0-9a-zA-Z._]+]]
+
+  // CHECK: [[STRONG]]
+  // CHECK-NOT: br
+  // CHECK: cmpxchg {{.*}} release acquire
+  // CHECK: br
+
+  // CHECK: [[WEAK]]
+  // CHECK-NOT: br
+  // CHECK: cmpxchg weak {{.*}} release acquire
+  // CHECK: br
 }
 
 // Having checked the flow in the previous two cases, we'll trust clang to
 // combine them sanely.
 void EMIT_ALL_THE_THINGS(int *ptr, int *ptr2, int new, _Bool weak, int success, int fail) {
   __atomic_compare_exchange(ptr, ptr2, &new, weak, success, fail);
 
   // CHECK: = cmpxchg {{.*}} monotonic monotonic, align
   // CHECK: = cmpxchg weak {{.*}} monotonic monotonic, align
   // CHECK: = cmpxchg {{.*}} acquire monotonic, align
   // CHECK: = cmpxchg {{.*}} acquire acquire, align
   // CHECK: = cmpxchg weak {{.*}} acquire monotonic, align
   // CHECK: = cmpxchg weak {{.*}} acquire acquire, align
   // CHECK: = cmpxchg {{.*}} release monotonic, align
+  // CHECK: = cmpxchg {{.*}} release acquire, align
   // CHECK: = cmpxchg weak {{.*}} release monotonic, align
+  // CHECK: = cmpxchg weak {{.*}} release acquire, align
   // CHECK: = cmpxchg {{.*}} acq_rel monotonic, align
   // CHECK: = cmpxchg {{.*}} acq_rel acquire, align
   // CHECK: = cmpxchg weak {{.*}} acq_rel monotonic, align
   // CHECK: = cmpxchg weak {{.*}} acq_rel acquire, align
   // CHECK: = cmpxchg {{.*}} seq_cst monotonic, align
   // CHECK: = cmpxchg {{.*}} seq_cst acquire, align
   // CHECK: = cmpxchg {{.*}} seq_cst seq_cst, align
   // CHECK: = cmpxchg weak {{.*}} seq_cst monotonic, align

clang/test/CodeGenOpenCL/atomic-ops.cl

void generalFailureOrder(atomic_int *ptr, int *ptr2, int success, int fail) {
   // CHECK: [[ACQUIRE]]
   // CHECK: switch {{.*}}, label %[[ACQUIRE_MONOTONIC:[0-9a-zA-Z._]+]] [
   // CHECK-NEXT: i32 1, label %[[ACQUIRE_ACQUIRE:[0-9a-zA-Z._]+]]
   // CHECK-NEXT: i32 2, label %[[ACQUIRE_ACQUIRE:[0-9a-zA-Z._]+]]
   // CHECK-NEXT: ]
 
   // CHECK: [[RELEASE]]
   // CHECK: switch {{.*}}, label %[[RELEASE_MONOTONIC:[0-9a-zA-Z._]+]] [
+  // CHECK-NEXT: i32 2, label %[[RELEASE_ACQUIRE:[0-9a-zA-Z._]+]]
   // CHECK-NEXT: ]
 
   // CHECK: [[ACQREL]]
   // CHECK: switch {{.*}}, label %[[ACQREL_MONOTONIC:[0-9a-zA-Z._]+]] [
   // CHECK-NEXT: i32 1, label %[[ACQREL_ACQUIRE:[0-9a-zA-Z._]+]]
   // CHECK-NEXT: i32 2, label %[[ACQREL_ACQUIRE:[0-9a-zA-Z._]+]]
   // CHECK-NEXT: ]
 
void generalFailureOrder(atomic_int *ptr, int *ptr2, int success, int fail) {
   // CHECK: [[ACQUIRE_MONOTONIC]]
   // CHECK: cmpxchg {{.*}} acquire monotonic, align 4
   // CHECK: br
 
   // CHECK: [[ACQUIRE_ACQUIRE]]
   // CHECK: cmpxchg {{.*}} acquire acquire, align 4
   // CHECK: br
 
+  // CHECK: [[RELEASE_MONOTONIC]]
+  // CHECK: cmpxchg {{.*}} release monotonic, align 4
+  // CHECK: br
+
+  // CHECK: [[RELEASE_ACQUIRE]]
+  // CHECK: cmpxchg {{.*}} release acquire, align 4
+  // CHECK: br
+
   // CHECK: [[ACQREL_MONOTONIC]]
   // CHECK: cmpxchg {{.*}} acq_rel monotonic, align 4
   // CHECK: br
 
   // CHECK: [[ACQREL_ACQUIRE]]
   // CHECK: cmpxchg {{.*}} acq_rel acquire, align 4
   // CHECK: br
 

llvm/docs/LangRef.rst

 than or equal to a target-specific size limit. '<cmp>' and '<new>' must
 have the same type, and the type of '<pointer>' must be a pointer to
 that type. If the ``cmpxchg`` is marked as ``volatile``, then the
 optimizer is not allowed to modify the number or order of execution of
 this ``cmpxchg`` with other :ref:`volatile operations <volatile>`.
 
 The success and failure :ref:`ordering <ordering>` arguments specify how this
 ``cmpxchg`` synchronizes with other atomic operations. Both ordering parameters
-must be at least ``monotonic``, the ordering constraint on failure must be no
-stronger than that on success, and the failure ordering cannot be either
+must be at least ``monotonic``, the failure ordering cannot be either
 ``release`` or ``acq_rel``.
 
 A ``cmpxchg`` instruction can also take an optional
 ":ref:`syncscope <syncscope>`" argument.
 
 The instruction can take an optional ``align`` attribute.
 The alignment must be a power of two greater or equal to the size of the
 `<value>` type. If unspecified, the alignment is assumed to be equal to the