[CGAtomic] Lyft strong requirement for remaining compare_exchange combinations

[bruno]

Summary

Follow up on 431e3138a and complete the other possible combinations.

Besides enforcing the new behavior it also mitigates several TSAN false positives
when combining orders that used to be stronger and therefore not allowed.

[bruno]

Fix failing tests and clang-format issue.

[rjmccall]

Your change is correct, but I think there's a deeper flaw in this code.

clang/lib/CodeGen/CGAtomic.cpp

460

Is this logic correct post-C++17? It looks like if you request a relaxed success order and an acquire failure order, this will use a relaxed order for both. That used to be okay because of the no-stronger restriction, but I think it's just invalid now, and all the conditionality needs to be removed.

[jfb]

clang/lib/CodeGen/CGAtomic.cpp

460

Hmm, I kinda feel like the whole switch thing is silly and we might want to just seq_cst any non-constexpr values. Barring that, yeah we should just adopt the C++17 change unconditionally, since the previous behavior was kinda silly.

[bruno]

clang/lib/CodeGen/CGAtomic.cpp

460

@rjmccall You are right about relaxed/acquire, I plan to address that in the future, I noticed this exact problem this morning while trying to fix D99434.

@jfb why is it silly? Isn't it possible to use runtime values that are still legit and would benefit from the switch? Or did you just mean for the specific case in John's comment?

[rjmccall]

Don't mind JF, he's just forgotten that C++ lacks the ability to turn the typically constant value arguments passed to functions like std::atomic::compare_exchange_strong into actual compile-time constants to the builtin used within those functions, so his advice would mean emitting all atomics in C++ as sequentially consistent.

Bruno, I think it would be straightforward to just fix this now; you just need to make creating AcquireBB and SeqCstBB unconditional. If you think there's a good reason not to do that in one patch, I won't insist, but it feels to me like the right patch.

[jfb]

In D101501#2735352, @rjmccall wrote:

Don't mind JF, he's just forgotten that C++ lacks the ability to turn the typically constant value arguments passed to functions like std::atomic::compare_exchange_strong into actual compile-time constants to the builtin used within those functions, so his advice would mean emitting all atomics in C++ as sequentially consistent.

Ugh right you are ☹️
I have a small soapbox about making this stuff a template parameter but I'll keep it to myself.

[bruno]

Don't mind JF, he's just forgotten that C++ lacks the ability to turn the typically constant value arguments passed to functions like std::atomic::compare_exchange_strong into actual compile-time constants to the builtin used within those functions, so his advice would mean emitting all atomics in C++ as sequentially consistent.

C++ often pushing towards the harsh reality!

Bruno, I think it would be straightforward to just fix this now; you just need to make creating AcquireBB and SeqCstBB unconditional. If you think there's a good reason not to do that in one patch, I won't insist, but it feels to me like the right patch.

Fair enough, will do! Thanks for bringing it up.

I have a small soapbox about making this stuff a template parameter but I'll keep it to myself.

Why not a small paper? :D

[rjmccall]

In D101501#2737180, @jfb wrote:

In D101501#2735352, @rjmccall wrote:

Don't mind JF, he's just forgotten that C++ lacks the ability to turn the typically constant value arguments passed to functions like std::atomic::compare_exchange_strong into actual compile-time constants to the builtin used within those functions, so his advice would mean emitting all atomics in C++ as sequentially consistent.

Ugh right you are ☹️
I have a small soapbox about making this stuff a template parameter but I'll keep it to myself.

It's almost funny how much IR this generates at -O0 (and a fix here will make it worse, of course). I wish it worked any other way. :)

[bruno]

Update after @rjmccall comments!

[bruno]

Reduce the diff in a test.

[rjmccall]

LGTM, except that the commit message should say "Lift", not "Lyft". ;)

[bruno]

LGTM, except that the commit message should say "Lift", not "Lyft". ;)

LOL, thanks!

[bruno]

Forgot to add the Differential tag, my bad. Closed by https://reviews.llvm.org/rG819e0d105e84

clang/lib/CodeGen/CGAtomic.cpp

static void emitAtomicCmpXchgFailureSet(CodeGenFunction &CGF, AtomicExpr *E,
 
   // 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)
     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 1, label %[[RELEASE_ACQUIRE:[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: ]

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 1, label %[[RELEASE_ACQUIRE:[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: ]