[PATCH] IRGen: __c11/__atomic compare-and-exchange should respect the standard

[David Majnemer]

Hi chandlerc, rsmith, rjmccall,

'Expected' should only be modified if the operation fails.

This fixes PR18899.

http://llvm-reviews.chandlerc.com/D2922

Files:
  lib/CodeGen/CGAtomic.cpp
  test/CodeGen/atomic-ops.c

Index: lib/CodeGen/CGAtomic.cpp
===================================================================
--- lib/CodeGen/CGAtomic.cpp
+++ lib/CodeGen/CGAtomic.cpp
@@ -201,17 +201,42 @@
   case AtomicExpr::AO__atomic_compare_exchange_n: {
     // Note that cmpxchg only supports specifying one ordering and
     // doesn't support weak cmpxchg, at least at the moment.
-    llvm::LoadInst *LoadVal1 = CGF.Builder.CreateLoad(Val1);
-    LoadVal1->setAlignment(Align);
-    llvm::LoadInst *LoadVal2 = CGF.Builder.CreateLoad(Val2);
-    LoadVal2->setAlignment(Align);
-    llvm::AtomicCmpXchgInst *CXI =
-        CGF.Builder.CreateAtomicCmpXchg(Ptr, LoadVal1, LoadVal2, Order);
-    CXI->setVolatile(E->isVolatile());
-    llvm::StoreInst *StoreVal1 = CGF.Builder.CreateStore(CXI, Val1);
-    StoreVal1->setAlignment(Align);
-    llvm::Value *Cmp = CGF.Builder.CreateICmpEQ(CXI, LoadVal1);
+
+    llvm::LoadInst *Expected = CGF.Builder.CreateLoad(Val1);
+    Expected->setAlignment(Align);
+    llvm::LoadInst *Desired = CGF.Builder.CreateLoad(Val2);
+    Desired->setAlignment(Align);
+    llvm::AtomicCmpXchgInst *Old =
+        CGF.Builder.CreateAtomicCmpXchg(Ptr, Expected, Desired, Order);
+    Old->setVolatile(E->isVolatile());
+
+    // Cmp holds the result of the compare-exchange operation: true on success,
+    // false on failure.
+    llvm::Value *Cmp = CGF.Builder.CreateICmpEQ(Old, Expected);
     CGF.EmitStoreOfScalar(Cmp, CGF.MakeAddrLValue(Dest, E->getType()));
+
+    // This basic block is used to hold the store instruction if the operation
+    // failed.
+    llvm::BasicBlock *StoreExpectedBB =
+        CGF.createBasicBlock("cmpxchg.store_expected", CGF.CurFn);
+
+    // This basic block is the exit point of the operation, we should end up
+    // here regardless of whether or not the operation succeeded.
+    llvm::BasicBlock *ContinueBB =
+        CGF.createBasicBlock("cmpxchg.continue", CGF.CurFn);
+
+    // Update Expected if Expected isn't equal to Old, otherwise branch to the
+    // exit point.
+    CGF.Builder.CreateCondBr(Cmp, ContinueBB, StoreExpectedBB);
+
+    CGF.Builder.SetInsertPoint(StoreExpectedBB);
+    // Update the memory at Expected with Old's value.
+    llvm::StoreInst *StoreExpected = CGF.Builder.CreateStore(Old, Val1);
+    StoreExpected->setAlignment(Align);
+    // Finally, branch to the exit point.
+    CGF.Builder.CreateBr(ContinueBB);
+
+    CGF.Builder.SetInsertPoint(ContinueBB);
     return;
   }
 
Index: test/CodeGen/atomic-ops.c
===================================================================
--- test/CodeGen/atomic-ops.c
+++ test/CodeGen/atomic-ops.c
@@ -91,22 +91,31 @@
 
 _Bool fi4(_Atomic(int) *i) {
   // CHECK: @fi4
-  // CHECK: cmpxchg i32*
+  // CHECK: [[OLD:%[.0-9A-Z_a-z]+]] = cmpxchg i32* [[PTR:%[.0-9A-Z_a-z]+]], i32 [[EXPECTED:%[.0-9A-Z_a-z]+]], i32 [[DESIRED:%[.0-9A-Z_a-z]+]]
+  // CHECK: [[CMP:%[.0-9A-Z_a-z]+]] = icmp eq i32 [[OLD]], [[EXPECTED]]
+  // CHECK: br i1 [[CMP]], label %[[STORE_EXPECTED:[.0-9A-Z_a-z]+]], label %[[CONTINUE:[.0-9A-Z_a-z]+]]
+  // CHECK: store i32 [[OLD]]
   int cmp = 0;
   return __c11_atomic_compare_exchange_strong(i, &cmp, 1, memory_order_acquire, memory_order_acquire);
 }
 
 _Bool fi4a(int *i) {
   // CHECK: @fi4
-  // CHECK: cmpxchg i32*
+  // CHECK: [[OLD:%[.0-9A-Z_a-z]+]] = cmpxchg i32* [[PTR:%[.0-9A-Z_a-z]+]], i32 [[EXPECTED:%[.0-9A-Z_a-z]+]], i32 [[DESIRED:%[.0-9A-Z_a-z]+]]
+  // CHECK: [[CMP:%[.0-9A-Z_a-z]+]] = icmp eq i32 [[OLD]], [[EXPECTED]]
+  // CHECK: br i1 [[CMP]], label %[[STORE_EXPECTED:[.0-9A-Z_a-z]+]], label %[[CONTINUE:[.0-9A-Z_a-z]+]]
+  // CHECK: store i32 [[OLD]]
   int cmp = 0;
   int desired = 1;
   return __atomic_compare_exchange(i, &cmp, &desired, 0, memory_order_acquire, memory_order_acquire);
 }
 
 _Bool fi4b(int *i) {
   // CHECK: @fi4
-  // CHECK: cmpxchg i32*
+  // CHECK: [[OLD:%[.0-9A-Z_a-z]+]] = cmpxchg i32* [[PTR:%[.0-9A-Z_a-z]+]], i32 [[EXPECTED:%[.0-9A-Z_a-z]+]], i32 [[DESIRED:%[.0-9A-Z_a-z]+]]
+  // CHECK: [[CMP:%[.0-9A-Z_a-z]+]] = icmp eq i32 [[OLD]], [[EXPECTED]]
+  // CHECK: br i1 [[CMP]], label %[[STORE_EXPECTED:[.0-9A-Z_a-z]+]], label %[[CONTINUE:[.0-9A-Z_a-z]+]]
+  // CHECK: store i32 [[OLD]]
   int cmp = 0;
   return __atomic_compare_exchange_n(i, &cmp, 1, 1, memory_order_acquire, memory_order_acquire);
 }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D2922.1.patch
Type: text/x-patch
Size: 4295 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140302/9a277143/attachment.bin>