Correct atomic libcall support for __atomic_*_fetch builtins.

[jyknight]

Summary

In r244063, I had caused these builtins to call the same-named library
functions, __atomic_*_fetch_SIZE. However, this was incorrect: while
those functions are in fact supported by GCC's libatomic, they're not
documented by the spec (and gcc doesn't ever call them).

Instead, you're /supposed/ to call the __atomic_fetch_* builtins and
then redo the operation inline to return the final value.

[compnerd]

Unfortunate that they have this crazy behavior.

lib/CodeGen/CGAtomic.cpp

901 ↗

(On Diff #39375)

I think we should use [[clang::fallthrough]] instead. It annotates as well as the comment, aids the static analyzer, and should be ignored by compilers that don't support it.

985 ↗

(On Diff #39375)

Can you add an explanatory message in the assert? (Yes, thats had to come up with since its obvious if you are familiar with this path).

998 ↗

(On Diff #39375)

Am I mistaken, but, we don't need this unless we have an operation to perform? Cant we sink this into the PostOp condition?

[jyknight]

Respond to comments.

lib/CodeGen/CGAtomic.cpp

901 ↗

(On Diff #39375)

I don't know why, but it appears that this would be the first use of that feature in clang/llvm. Everywhere else uses a comment, so I'd not want to be the first here.

[compnerd]

lib/CodeGen/CGAtomic.cpp

901 ↗

(On Diff #39644)

:-(. Very well.

cfe/trunk/lib/CodeGen/CGAtomic.cpp

case AtomicExpr::AO__atomic_xor_fetch:
     PostOp = llvm::Instruction::Xor;
     // Fall through.
   case AtomicExpr::AO__c11_atomic_fetch_xor:
   case AtomicExpr::AO__atomic_fetch_xor:
     Op = llvm::AtomicRMWInst::Xor;
     break;
 
   case AtomicExpr::AO__atomic_nand_fetch:
-    PostOp = llvm::Instruction::And;
-    // Fall through.
+    PostOp = llvm::Instruction::And; // the NOT is special cased below
   case AtomicExpr::AO__atomic_fetch_nand:
     Op = llvm::AtomicRMWInst::Nand;
     break;
   }
 
   llvm::Value *LoadVal1 = CGF.Builder.CreateLoad(Val1);
   llvm::AtomicRMWInst *RMWI =
       CGF.Builder.CreateAtomicRMW(Op, Ptr.getPointer(), LoadVal1, Order);
if (UseLibcall) {
     Args.add(RValue::get(EmitCastToVoidPtr(Ptr.getPointer())),
              getContext().VoidPtrTy);
 
     std::string LibCallName;
     QualType LoweredMemTy =
       MemTy->isPointerType() ? getContext().getIntPtrType() : MemTy;
     QualType RetTy;
     bool HaveRetTy = false;
+    llvm::Instruction::BinaryOps PostOp = (llvm::Instruction::BinaryOps)0;
     switch (E->getOp()) {
     case AtomicExpr::AO__c11_atomic_init:
       llvm_unreachable("Already handled!");
 
     // There is only one libcall for compare an exchange, because there is no
     // optimisation benefit possible from a libcall version of a weak compare
     // and exchange.
     // bool __atomic_compare_exchange(size_t size, void *mem, void *expected,
case AtomicExpr::AO__atomic_store_n:
       break;
     // void __atomic_load(size_t size, void *mem, void *return, int order)
     // T __atomic_load_N(T *mem, int order)
     case AtomicExpr::AO__c11_atomic_load:
     case AtomicExpr::AO__atomic_load:
     case AtomicExpr::AO__atomic_load_n:
       LibCallName = "__atomic_load";
       break;
+    // T __atomic_add_fetch_N(T *mem, T val, int order)
     // T __atomic_fetch_add_N(T *mem, T val, int order)
+    case AtomicExpr::AO__atomic_add_fetch:
+      PostOp = llvm::Instruction::Add;
+    // Fall through.
     case AtomicExpr::AO__c11_atomic_fetch_add:
     case AtomicExpr::AO__atomic_fetch_add:
       LibCallName = "__atomic_fetch_add";
       AddDirectArgument(*this, Args, UseOptimizedLibcall, Val1.getPointer(),
                         LoweredMemTy, E->getExprLoc(), sizeChars);
       break;
+    // T __atomic_and_fetch_N(T *mem, T val, int order)
     // T __atomic_fetch_and_N(T *mem, T val, int order)
+    case AtomicExpr::AO__atomic_and_fetch:
+      PostOp = llvm::Instruction::And;
+    // Fall through.
     case AtomicExpr::AO__c11_atomic_fetch_and:
     case AtomicExpr::AO__atomic_fetch_and:
       LibCallName = "__atomic_fetch_and";
       AddDirectArgument(*this, Args, UseOptimizedLibcall, Val1.getPointer(),
                         MemTy, E->getExprLoc(), sizeChars);
       break;
+    // T __atomic_or_fetch_N(T *mem, T val, int order)
     // T __atomic_fetch_or_N(T *mem, T val, int order)
+    case AtomicExpr::AO__atomic_or_fetch:
+      PostOp = llvm::Instruction::Or;
+    // Fall through.
     case AtomicExpr::AO__c11_atomic_fetch_or:
     case AtomicExpr::AO__atomic_fetch_or:
       LibCallName = "__atomic_fetch_or";
       AddDirectArgument(*this, Args, UseOptimizedLibcall, Val1.getPointer(),
                         MemTy, E->getExprLoc(), sizeChars);
       break;
+    // T __atomic_sub_fetch_N(T *mem, T val, int order)
     // T __atomic_fetch_sub_N(T *mem, T val, int order)
+    case AtomicExpr::AO__atomic_sub_fetch:
+      PostOp = llvm::Instruction::Sub;
+    // Fall through.
     case AtomicExpr::AO__c11_atomic_fetch_sub:
     case AtomicExpr::AO__atomic_fetch_sub:
       LibCallName = "__atomic_fetch_sub";
       AddDirectArgument(*this, Args, UseOptimizedLibcall, Val1.getPointer(),
                         LoweredMemTy, E->getExprLoc(), sizeChars);
       break;
+    // T __atomic_xor_fetch_N(T *mem, T val, int order)
     // T __atomic_fetch_xor_N(T *mem, T val, int order)
+    case AtomicExpr::AO__atomic_xor_fetch:
+      PostOp = llvm::Instruction::Xor;
+    // Fall through.
     case AtomicExpr::AO__c11_atomic_fetch_xor:
     case AtomicExpr::AO__atomic_fetch_xor:
       LibCallName = "__atomic_fetch_xor";
       AddDirectArgument(*this, Args, UseOptimizedLibcall, Val1.getPointer(),
                         MemTy, E->getExprLoc(), sizeChars);
       break;
+    // T __atomic_nand_fetch_N(T *mem, T val, int order)
     // T __atomic_fetch_nand_N(T *mem, T val, int order)
+    case AtomicExpr::AO__atomic_nand_fetch:
+      PostOp = llvm::Instruction::And; // the NOT is special cased below
+    // Fall through.
     case AtomicExpr::AO__atomic_fetch_nand:
       LibCallName = "__atomic_fetch_nand";
       AddDirectArgument(*this, Args, UseOptimizedLibcall, Val1.getPointer(),
                         MemTy, E->getExprLoc(), sizeChars);
       break;
-
-    // T __atomic_add_fetch_N(T *mem, T val, int order)
-    case AtomicExpr::AO__atomic_add_fetch:
-      LibCallName = "__atomic_add_fetch";
-      AddDirectArgument(*this, Args, UseOptimizedLibcall, Val1.getPointer(),
-                        LoweredMemTy, E->getExprLoc(), sizeChars);
-      break;
-    // T __atomic_and_fetch_N(T *mem, T val, int order)
-    case AtomicExpr::AO__atomic_and_fetch:
-      LibCallName = "__atomic_and_fetch";
-      AddDirectArgument(*this, Args, UseOptimizedLibcall, Val1.getPointer(),
-                        MemTy, E->getExprLoc(), sizeChars);
-      break;
-    // T __atomic_or_fetch_N(T *mem, T val, int order)
-    case AtomicExpr::AO__atomic_or_fetch:
-      LibCallName = "__atomic_or_fetch";
-      AddDirectArgument(*this, Args, UseOptimizedLibcall, Val1.getPointer(),
-                        MemTy, E->getExprLoc(), sizeChars);
-      break;
-    // T __atomic_sub_fetch_N(T *mem, T val, int order)
-    case AtomicExpr::AO__atomic_sub_fetch:
-      LibCallName = "__atomic_sub_fetch";
-      AddDirectArgument(*this, Args, UseOptimizedLibcall, Val1.getPointer(),
-                        LoweredMemTy, E->getExprLoc(), sizeChars);
-      break;
-    // T __atomic_xor_fetch_N(T *mem, T val, int order)
-    case AtomicExpr::AO__atomic_xor_fetch:
-      LibCallName = "__atomic_xor_fetch";
-      AddDirectArgument(*this, Args, UseOptimizedLibcall, Val1.getPointer(),
-                        MemTy, E->getExprLoc(), sizeChars);
-      break;
-    // T __atomic_nand_fetch_N(T *mem, T val, int order)
-    case AtomicExpr::AO__atomic_nand_fetch:
-      LibCallName = "__atomic_nand_fetch";
-      AddDirectArgument(*this, Args, UseOptimizedLibcall, Val1.getPointer(),
-                        MemTy, E->getExprLoc(), sizeChars);
-      break;
     }
 
     // Optimized functions have the size in their name.
     if (UseOptimizedLibcall)
       LibCallName += "_" + llvm::utostr(Size);
     // By default, assume we return a value of the atomic type.
     if (!HaveRetTy) {
       if (UseOptimizedLibcall) {
         // Value is returned directly.
         // The function returns an appropriately sized integer type.
         RetTy = getContext().getIntTypeForBitwidth(
             getContext().toBits(sizeChars), /*Signed=*/false);
       } else {
         // Value is returned through parameter before the order.
         RetTy = getContext().VoidTy;
         Args.add(RValue::get(EmitCastToVoidPtr(Dest.getPointer())),
                  getContext().VoidPtrTy);
       }
     }
     // order is always the last parameter
     Args.add(RValue::get(Order),
              getContext().IntTy);
 
+    // PostOp is only needed for the atomic_*_fetch operations, and
+    // thus is only needed for and implemented in the
+    // UseOptimizedLibcall codepath.
+    assert(UseOptimizedLibcall || !PostOp);
+
     RValue Res = emitAtomicLibcall(*this, LibCallName, RetTy, Args);
     // The value is returned directly from the libcall.
     if (E->isCmpXChg())
       return Res;
 
     // The value is returned directly for optimized libcalls but the expr
     // provided an out-param.
     if (UseOptimizedLibcall && Res.getScalarVal()) {
       llvm::Value *ResVal = Res.getScalarVal();
+      if (PostOp) {
+        llvm::Value *LoadVal1 = Args[1].RV.getScalarVal();
+        ResVal = Builder.CreateBinOp(PostOp, ResVal, LoadVal1);
+      }
+      if (E->getOp() == AtomicExpr::AO__atomic_nand_fetch)
+        ResVal = Builder.CreateNot(ResVal);
+
       Builder.CreateStore(
           ResVal,
           Builder.CreateBitCast(Dest, ResVal->getType()->getPointerTo()));
     }
 
     if (RValTy->isVoidType())
       return RValue::get(nullptr);
 

cfe/trunk/test/CodeGen/atomic-ops-libcall.c

 int test_atomic_fetch_nand(int *p) {
   // CHECK: test_atomic_fetch_nand
   // CHECK: {{%[^ ]*}} = tail call i32 @__atomic_fetch_nand_4(i8* {{%[0-9]+}}, i32 55, i32 5)
   return __atomic_fetch_nand(p, 55, memory_order_seq_cst);
 }
 
 int test_atomic_add_fetch(int *p) {
   // CHECK: test_atomic_add_fetch
-  // CHECK: {{%[^ ]*}} = tail call i32 @__atomic_add_fetch_4(i8* {{%[0-9]+}}, i32 55, i32 5)
+  // CHECK: [[CALL:%[^ ]*]] = tail call i32 @__atomic_fetch_add_4(i8* {{%[0-9]+}}, i32 55, i32 5)
+  // CHECK: {{%[^ ]*}} = add i32 [[CALL]], 55
   return __atomic_add_fetch(p, 55, memory_order_seq_cst);
 }
 
 int test_atomic_sub_fetch(int *p) {
   // CHECK: test_atomic_sub_fetch
-  // CHECK: {{%[^ ]*}} = tail call i32 @__atomic_sub_fetch_4(i8* {{%[0-9]+}}, i32 55, i32 5)
+  // CHECK: [[CALL:%[^ ]*]] = tail call i32 @__atomic_fetch_sub_4(i8* {{%[0-9]+}}, i32 55, i32 5)
+  // CHECK: {{%[^ ]*}} = add i32 [[CALL]], -55
   return __atomic_sub_fetch(p, 55, memory_order_seq_cst);
 }
 
 int test_atomic_and_fetch(int *p) {
   // CHECK: test_atomic_and_fetch
-  // CHECK: {{%[^ ]*}} = tail call i32 @__atomic_and_fetch_4(i8* {{%[0-9]+}}, i32 55, i32 5)
+  // CHECK: [[CALL:%[^ ]*]] = tail call i32 @__atomic_fetch_and_4(i8* {{%[0-9]+}}, i32 55, i32 5)
+  // CHECK: {{%[^ ]*}} = and i32 [[CALL]], 55
   return __atomic_and_fetch(p, 55, memory_order_seq_cst);
 }
 
 int test_atomic_or_fetch(int *p) {
   // CHECK: test_atomic_or_fetch
-  // CHECK: {{%[^ ]*}} = tail call i32 @__atomic_or_fetch_4(i8* {{%[0-9]+}}, i32 55, i32 5)
+  // CHECK: [[CALL:%[^ ]*]] = tail call i32 @__atomic_fetch_or_4(i8* {{%[0-9]+}}, i32 55, i32 5)
+  // CHECK: {{%[^ ]*}} = or i32 [[CALL]], 55
   return __atomic_or_fetch(p, 55, memory_order_seq_cst);
 }
 
 int test_atomic_xor_fetch(int *p) {
   // CHECK: test_atomic_xor_fetch
-  // CHECK: {{%[^ ]*}} = tail call i32 @__atomic_xor_fetch_4(i8* {{%[0-9]+}}, i32 55, i32 5)
+  // CHECK: [[CALL:%[^ ]*]] = tail call i32 @__atomic_fetch_xor_4(i8* {{%[0-9]+}}, i32 55, i32 5)
+  // CHECK: {{%[^ ]*}} = xor i32 [[CALL]], 55
   return __atomic_xor_fetch(p, 55, memory_order_seq_cst);
 }
 
 int test_atomic_nand_fetch(int *p) {
   // CHECK: test_atomic_nand_fetch
-  // CHECK: {{%[^ ]*}} = tail call i32 @__atomic_nand_fetch_4(i8* {{%[0-9]+}}, i32 55, i32 5)
+  // CHECK: [[CALL:%[^ ]*]] = tail call i32 @__atomic_fetch_nand_4(i8* {{%[0-9]+}}, i32 55, i32 5)
+  // CHECK: [[OR:%[^ ]*]] = or i32 [[CALL]], -56
+  // CHECK: {{%[^ ]*}} = xor i32 [[OR]], 55
   return __atomic_nand_fetch(p, 55, memory_order_seq_cst);
 }