[clang] 10e0d64 - CodeGen: set correct result for atomic compound expressions

[Tim Northover via cfe-commits]

Author: Tim Northover
Date: 2019-11-07T13:36:44Z
New Revision: 10e0d64337d64ebdb658bf9108bd9bb48fb5390c

URL: https://github.com/llvm/llvm-project/commit/10e0d64337d64ebdb658bf9108bd9bb48fb5390c
DIFF: https://github.com/llvm/llvm-project/commit/10e0d64337d64ebdb658bf9108bd9bb48fb5390c.diff

LOG: CodeGen: set correct result for atomic compound expressions

Atomic compound expressions try to use atomicrmw if possible, but this
path doesn't set the Result variable, leaving it to crash in later code
if anything ever tries to use the result of the expression. This fixes
that issue by recalculating the new value based on the old one
atomically loaded.

Added: 
    

Modified: 
    clang/lib/CodeGen/CGExprScalar.cpp
    clang/test/CodeGen/atomic_ops.c

Removed: 
    


################################################################################
diff  --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index c1391d46f60c..822976640643 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -2849,7 +2849,8 @@ LValue ScalarExprEmitter::EmitCompoundAssignLValue(
           CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow)) &&
         CGF.getLangOpts().getSignedOverflowBehavior() !=
             LangOptions::SOB_Trapping) {
-      llvm::AtomicRMWInst::BinOp aop = llvm::AtomicRMWInst::BAD_BINOP;
+      llvm::AtomicRMWInst::BinOp AtomicOp = llvm::AtomicRMWInst::BAD_BINOP;
+      llvm::Instruction::BinaryOps Op;
       switch (OpInfo.Opcode) {
         // We don't have atomicrmw operands for *, %, /, <<, >>
         case BO_MulAssign: case BO_DivAssign:
@@ -2858,30 +2859,40 @@ LValue ScalarExprEmitter::EmitCompoundAssignLValue(
         case BO_ShrAssign:
           break;
         case BO_AddAssign:
-          aop = llvm::AtomicRMWInst::Add;
+          AtomicOp = llvm::AtomicRMWInst::Add;
+          Op = llvm::Instruction::Add;
           break;
         case BO_SubAssign:
-          aop = llvm::AtomicRMWInst::Sub;
+          AtomicOp = llvm::AtomicRMWInst::Sub;
+          Op = llvm::Instruction::Sub;
           break;
         case BO_AndAssign:
-          aop = llvm::AtomicRMWInst::And;
+          AtomicOp = llvm::AtomicRMWInst::And;
+          Op = llvm::Instruction::And;
           break;
         case BO_XorAssign:
-          aop = llvm::AtomicRMWInst::Xor;
+          AtomicOp = llvm::AtomicRMWInst::Xor;
+          Op = llvm::Instruction::Xor;
           break;
         case BO_OrAssign:
-          aop = llvm::AtomicRMWInst::Or;
+          AtomicOp = llvm::AtomicRMWInst::Or;
+          Op = llvm::Instruction::Or;
           break;
         default:
           llvm_unreachable("Invalid compound assignment type");
       }
-      if (aop != llvm::AtomicRMWInst::BAD_BINOP) {
-        llvm::Value *amt = CGF.EmitToMemory(
+      if (AtomicOp != llvm::AtomicRMWInst::BAD_BINOP) {
+        llvm::Value *Amt = CGF.EmitToMemory(
             EmitScalarConversion(OpInfo.RHS, E->getRHS()->getType(), LHSTy,
                                  E->getExprLoc()),
             LHSTy);
-        Builder.CreateAtomicRMW(aop, LHSLV.getPointer(), amt,
+        Value *OldVal = Builder.CreateAtomicRMW(
+            AtomicOp, LHSLV.getPointer(), Amt,
             llvm::AtomicOrdering::SequentiallyConsistent);
+
+        // Since operation is atomic, the result type is guaranteed to be the
+        // same as the input in LLVM terms.
+        Result = Builder.CreateBinOp(Op, OldVal, Amt);
         return LHSLV;
       }
     }

diff  --git a/clang/test/CodeGen/atomic_ops.c b/clang/test/CodeGen/atomic_ops.c
index 0af1d387192d..a853ba9f739c 100644
--- a/clang/test/CodeGen/atomic_ops.c
+++ b/clang/test/CodeGen/atomic_ops.c
@@ -37,3 +37,56 @@ void baz(int y) {
 // CHECK: {{store atomic|call void @__atomic_store}}
   x += y;
 }
+
+_Atomic(int) compound_add(_Atomic(int) in) {
+// CHECK-LABEL: @compound_add
+// CHECK: [[OLD:%.*]] = atomicrmw add i32* {{.*}}, i32 5 seq_cst
+// CHECK: [[NEW:%.*]] = add i32 [[OLD]], 5
+// CHECK: ret i32 [[NEW]]
+
+  return (in += 5);
+}
+
+_Atomic(int) compound_sub(_Atomic(int) in) {
+// CHECK-LABEL: @compound_sub
+// CHECK: [[OLD:%.*]] = atomicrmw sub i32* {{.*}}, i32 5 seq_cst
+// CHECK: [[NEW:%.*]] = sub i32 [[OLD]], 5
+// CHECK: ret i32 [[NEW]]
+
+  return (in -= 5);
+}
+
+_Atomic(int) compound_xor(_Atomic(int) in) {
+// CHECK-LABEL: @compound_xor
+// CHECK: [[OLD:%.*]] = atomicrmw xor i32* {{.*}}, i32 5 seq_cst
+// CHECK: [[NEW:%.*]] = xor i32 [[OLD]], 5
+// CHECK: ret i32 [[NEW]]
+
+  return (in ^= 5);
+}
+
+_Atomic(int) compound_or(_Atomic(int) in) {
+// CHECK-LABEL: @compound_or
+// CHECK: [[OLD:%.*]] = atomicrmw or i32* {{.*}}, i32 5 seq_cst
+// CHECK: [[NEW:%.*]] = or i32 [[OLD]], 5
+// CHECK: ret i32 [[NEW]]
+
+  return (in |= 5);
+}
+
+_Atomic(int) compound_and(_Atomic(int) in) {
+// CHECK-LABEL: @compound_and
+// CHECK: [[OLD:%.*]] = atomicrmw and i32* {{.*}}, i32 5 seq_cst
+// CHECK: [[NEW:%.*]] = and i32 [[OLD]], 5
+// CHECK: ret i32 [[NEW]]
+
+  return (in &= 5);
+}
+
+_Atomic(int) compound_mul(_Atomic(int) in) {
+// CHECK-LABEL: @compound_mul
+// CHECK: cmpxchg i32* {{%.*}}, i32 {{%.*}}, i32 [[NEW:%.*]] seq_cst seq_cst
+// CHECK: ret i32 [[NEW]]
+
+  return (in *= 5);
+}