[PATCH] D71225: [OpenMP][WIP] atomic update only evaluate once for expression having side effect

[Chi Chun Chen via Phabricator via cfe-commits]

cchen created this revision.
cchen added a reviewer: ABataev.
Herald added subscribers: cfe-commits, jfb, guansong.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.
cchen edited the summary of this revision.
cchen added a project: OpenMP.
cchen added subscribers: dreachem, sandoval.

This patch is for pointing out that we might need to check if the
expression in atomic update having any side effect. In the past we
always only evaluate an expression once, for example:

  int cnt = 0;
  int foo() {
    return ++cnt;
  }
  
  void bar() {
    int iarr[100];
    iarr[foo(), foo(), 0]  = iarr[foo(), foo(), 0] + 1;
  }

For now, the result of cnt is 2, however, we probably want cnt to be 4
or we might want to tell user that we only evaluate the expression once
as diagnosis message.

Actually, `atomic capture` have the exact same issue, but the purpose of
this patch to expose the issue. After the discussion, I might send another
patch to fix the issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71225

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/atomic_update_codegen.cpp


Index: clang/test/OpenMP/atomic_update_codegen.cpp
===================================================================
--- clang/test/OpenMP/atomic_update_codegen.cpp
+++ clang/test/OpenMP/atomic_update_codegen.cpp
@@ -27,6 +27,12 @@
 _Complex int civ, cix;
 _Complex float cfv, cfx;
 _Complex double cdv, cdx;
+int iarr[100];
+int cnt = 0;
+
+int foo() {
+  return cnt++;
+}
 
 typedef int int4 __attribute__((__vector_size__(16)));
 int4 int4x;
@@ -900,6 +906,17 @@
 // CHECK: call{{.*}} @__kmpc_flush(
 #pragma omp atomic seq_cst
   rix = dv / rix;
+
+#pragma omp atomic update
+  iarr[foo(), 0]  = iarr[foo(), 0] + 1;
+// CHECK: call i32 @foo()
+// CHECK: call i32 @foo()
+// CHECK: [[ZERO:%.+]] = atomicrmw add i32* [[ARRAY_IDX:.+]], i32 1 monotonic
+
+#pragma omp atomic update
+  iarr[foo(), foo(), 0]  = iarr[foo(), foo(), 0] + 1;
+// CHECK-COUNT-4: call i32 @foo()
+// CHECK: [[ZERO:%.+]] = atomicrmw add i32* [[ARRAY_IDX:.+]], i32 1 monotonic
   return 0;
 }
 
Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===================================================================
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -3946,6 +3946,30 @@
   return Res;
 }
 
+static void traverseBinOp(CodeGenFunction &CGF, const Expr *E) {
+  if (auto *BinOp = dyn_cast<BinaryOperator>(E)) {
+    traverseBinOp(CGF, BinOp->getLHS());
+    traverseBinOp(CGF, BinOp->getRHS());
+  } else {
+    if (auto *Call = dyn_cast<CallExpr>(E)) {
+      CGF.EmitCallExpr(Call);
+    }
+  }
+}
+
+static void handleArraySubscriptSideEffect(CodeGenFunction &CGF,
+                                           const Expr *X) {
+  // If X is a ArraySubscript and the expression has side effect, then we'll
+  // have wrong result since we only evaluate the expression once.
+  if (X->getStmtClass() == Expr::ArraySubscriptExprClass) {
+    auto E = cast<ArraySubscriptExpr>(X);
+    if (E->getLHS() != E->getIdx()) {
+      assert(E->getRHS() == E->getIdx() && "index was neither LHS nor RHS");
+      traverseBinOp(CGF, E->getIdx());
+    }
+  }
+}
+
 static void emitOMPAtomicUpdateExpr(CodeGenFunction &CGF, bool IsSeqCst,
                                     const Expr *X, const Expr *E,
                                     const Expr *UE, bool IsXLHSInRHSPart,
@@ -3960,6 +3984,7 @@
   // x = x binop expr; -> xrval binop expr
   // x = expr Op x; - > expr binop xrval;
   assert(X->isLValue() && "X of 'omp atomic update' is not lvalue");
+  handleArraySubscriptSideEffect(CGF, X);
   LValue XLValue = CGF.EmitLValue(X);
   RValue ExprRValue = CGF.EmitAnyExpr(E);
   llvm::AtomicOrdering AO = IsSeqCst


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D71225.232929.patch
Type: text/x-patch
Size: 2620 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191209/25d4f85b/attachment-0001.bin>