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
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
