[PATCH] D64889: [OPENMP] getDSA(): handle loop control variables
Prince781 created this revision.
Prince781 added reviewers: ABataev, rsmith.
Prince781 added projects: clang, OpenMP.
Herald added subscribers: cfe-commits, jdoerfert, guansong.
The following example compiles incorrectly since at least clang 8.0.0:
#include
#include
#define N 100
int main(void) {
int i;
int arr[N];
#pragma omp parallel // shared(i) shared(arr)
#pragma omp for // private(i)
for (i = 0; i < N; i++) {
#pragma omp task // firstprivate(i) shared(arr)
{
printf("[thread %2d] i = %d\n", omp_get_thread_num(), i);
arr[i] = i;
}
}
for (i = 0; i < N; i++) {
if (arr[i] != i) {
fprintf(stderr, "FAIL: arr[%d] == %d\n", i, arr[i]);
}
}
}
The iteration variable, `i`, should become `private` at the `omp for` construct
and then become implicit `firstprivate` within the task region. What happens
instead is that `i` is never privatized within the task construct. As the task
construct is parsed, when a reference to `i` is determined, the implicit
data-sharing attributes are computed incorrectly.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D64889
Files:
clang/lib/Sema/SemaOpenMP.cpp
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -424,6 +424,11 @@
/// for-loops (from outer to inner).
const LCDeclInfo isLoopControlVariable(const ValueDecl *D) const;
/// Check if the specified variable is a loop control variable for
+ /// given region.
+ /// \return The index of the loop control variable in the list of associated
+ /// for-loops (from outer to inner).
+ const LCDeclInfo isLoopControlVariable(const ValueDecl *D, const
SharingMapTy &Region) const;
+ /// Check if the specified variable is a loop control variable for
/// parent region.
/// \return The index of the loop control variable in the list of associated
/// for-loops (from outer to inner).
@@ -946,11 +951,24 @@
case DSA_none:
return DVar;
case DSA_unspecified:
+DVar.ImplicitDSALoc = Iter->DefaultAttrLoc;
+// OpenMP [2.9.1.1, Data-sharing Attribute Rules for Variables Referenced
+// in a Construct, implicitly determined]
+// The loop iteration variable(s) in the associated for-loop(s) of a for
or
+// parallel for construct is (are) private.
+// OpenMP 5.0 includes taskloop and distribute directives
+if (!isOpenMPSimdDirective(DVar.DKind) &&
+isOpenMPLoopDirective(DVar.DKind) &&
+isLoopControlVariable(D, *Iter).first) {
+ DVar.CKind = OMPC_private;
+ // TODO: OpenMP 5.0: if (Dvar.DKind == OMPD_loop) DVar.CKind =
OMPC_lastprivate;
+ return DVar;
+}
+
// OpenMP [2.9.1.1, Data-sharing Attribute Rules for Variables Referenced
// in a Construct, implicitly determined, p.2]
// In a parallel construct, if no default clause is present, these
// variables are shared.
-DVar.ImplicitDSALoc = Iter->DefaultAttrLoc;
if (isOpenMPParallelDirective(DVar.DKind) ||
isOpenMPTeamsDirective(DVar.DKind)) {
DVar.CKind = OMPC_shared;
@@ -1018,8 +1036,13 @@
const DSAStackTy::LCDeclInfo
DSAStackTy::isLoopControlVariable(const ValueDecl *D) const {
assert(!isStackEmpty() && "Data-sharing attributes stack is empty");
+ return isLoopControlVariable(D, getTopOfStack());
+}
+
+const DSAStackTy::LCDeclInfo
+DSAStackTy::isLoopControlVariable(const ValueDecl *D, const SharingMapTy
&Region) const {
D = getCanonicalDecl(D);
- const SharingMapTy &StackElem = getTopOfStack();
+ const SharingMapTy &StackElem = Region;
auto It = StackElem.LCVMap.find(D);
if (It != StackElem.LCVMap.end())
return It->second;
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -424,6 +424,11 @@
/// for-loops (from outer to inner).
const LCDeclInfo isLoopControlVariable(const ValueDecl *D) const;
/// Check if the specified variable is a loop control variable for
+ /// given region.
+ /// \return The index of the loop control variable in the list of associated
+ /// for-loops (from outer to inner).
+ const LCDeclInfo isLoopControlVariable(const ValueDecl *D, const SharingMapTy &Region) const;
+ /// Check if the specified variable is a loop control variable for
/// parent region.
/// \return The index of the loop control variable in the list of associated
/// for-loops (from outer to inner).
@@ -946,11 +951,24 @@
case DSA_none:
return DVar;
case DSA_unspecified:
+DVar.ImplicitDSALoc = Iter->DefaultAttrLoc;
+// OpenMP [2.9.1.1, Data-sharing Attribute Rules for Variables Referenced
+// in a Construct, implicitly determined]
+// The loop iteration variable(s) in the
[PATCH] D64889: [OPENMP] getDSA(): handle loop control variables
lebedev.ri added a comment. Needs a test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64889/new/ https://reviews.llvm.org/D64889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64889: [OPENMP] getDSA(): handle loop control variables
Prince781 updated this revision to Diff 210459.
Prince781 added a comment.
Added a lit test.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64889/new/
https://reviews.llvm.org/D64889
Files:
clang/lib/Sema/SemaOpenMP.cpp
clang/test/OpenMP/loop_control_var_nested_task.cpp
Index: clang/test/OpenMP/loop_control_var_nested_task.cpp
===
--- /dev/null
+++ clang/test/OpenMP/loop_control_var_nested_task.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 %s -fopenmp -emit-llvm -o - | FileCheck %s
+// CHECK: {{%struct}}.kmp_task_t_with_privates = type {
{{.*%struct\.\.kmp_privates.t.*}} }
+// CHECK: define internal void
@.omp_task_privates_map.({{.*%struct\.\.kmp_privates.t.*}})
+#define N 100
+int main(void) {
+// declare this variable outside, so that it will be shared on the outer
parallel construct
+int i;
+int arr[N];
+
+#pragma omp parallel // shared(i) shared(arr)
+#pragma omp for // private(i) - i should be privatized because it is an
iteration variable
+for (i = 0; i < N; i++) {
+#pragma omp task
+// CHECK: %.firstpriv.ptr.addr.i = alloca i32*, align 8
+// CHECK: {{%[0-9]+}} = load i32*, i32** %.firstpriv.ptr.addr.i, align
8
+arr[i] = i;
+}
+}
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -424,6 +424,11 @@
/// for-loops (from outer to inner).
const LCDeclInfo isLoopControlVariable(const ValueDecl *D) const;
/// Check if the specified variable is a loop control variable for
+ /// given region.
+ /// \return The index of the loop control variable in the list of associated
+ /// for-loops (from outer to inner).
+ const LCDeclInfo isLoopControlVariable(const ValueDecl *D, const
SharingMapTy &Region) const;
+ /// Check if the specified variable is a loop control variable for
/// parent region.
/// \return The index of the loop control variable in the list of associated
/// for-loops (from outer to inner).
@@ -946,11 +951,24 @@
case DSA_none:
return DVar;
case DSA_unspecified:
+DVar.ImplicitDSALoc = Iter->DefaultAttrLoc;
+// OpenMP [2.9.1.1, Data-sharing Attribute Rules for Variables Referenced
+// in a Construct, implicitly determined]
+// The loop iteration variable(s) in the associated for-loop(s) of a for
or
+// parallel for construct is (are) private.
+// OpenMP 5.0 includes taskloop and distribute directives
+if (!isOpenMPSimdDirective(DVar.DKind) &&
+isOpenMPLoopDirective(DVar.DKind) &&
+isLoopControlVariable(D, *Iter).first) {
+ DVar.CKind = OMPC_private;
+ // TODO: OpenMP 5.0: if (Dvar.DKind == OMPD_loop) DVar.CKind =
OMPC_lastprivate;
+ return DVar;
+}
+
// OpenMP [2.9.1.1, Data-sharing Attribute Rules for Variables Referenced
// in a Construct, implicitly determined, p.2]
// In a parallel construct, if no default clause is present, these
// variables are shared.
-DVar.ImplicitDSALoc = Iter->DefaultAttrLoc;
if (isOpenMPParallelDirective(DVar.DKind) ||
isOpenMPTeamsDirective(DVar.DKind)) {
DVar.CKind = OMPC_shared;
@@ -1018,8 +1036,13 @@
const DSAStackTy::LCDeclInfo
DSAStackTy::isLoopControlVariable(const ValueDecl *D) const {
assert(!isStackEmpty() && "Data-sharing attributes stack is empty");
+ return isLoopControlVariable(D, getTopOfStack());
+}
+
+const DSAStackTy::LCDeclInfo
+DSAStackTy::isLoopControlVariable(const ValueDecl *D, const SharingMapTy
&Region) const {
D = getCanonicalDecl(D);
- const SharingMapTy &StackElem = getTopOfStack();
+ const SharingMapTy &StackElem = Region;
auto It = StackElem.LCVMap.find(D);
if (It != StackElem.LCVMap.end())
return It->second;
Index: clang/test/OpenMP/loop_control_var_nested_task.cpp
===
--- /dev/null
+++ clang/test/OpenMP/loop_control_var_nested_task.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 %s -fopenmp -emit-llvm -o - | FileCheck %s
+// CHECK: {{%struct}}.kmp_task_t_with_privates = type { {{.*%struct\.\.kmp_privates.t.*}} }
+// CHECK: define internal void @.omp_task_privates_map.({{.*%struct\.\.kmp_privates.t.*}})
+#define N 100
+int main(void) {
+// declare this variable outside, so that it will be shared on the outer parallel construct
+int i;
+int arr[N];
+
+#pragma omp parallel // shared(i) shared(arr)
+#pragma omp for // private(i) - i should be privatized because it is an iteration variable
+for (i = 0; i < N; i++) {
+#pragma omp task
+// CHECK: %.firstpriv.ptr.addr.i = alloca i32*, align 8
+// CHECK: {{%[0-9]+}} = load i32*, i32** %.firstpriv.ptr.addr.i, align 8
+arr[i] = i;
+}
+}
Index: clang/lib/Sema/SemaOpenMP.cpp
===
[PATCH] D64889: [OPENMP] getDSA(): handle loop control variables
ABataev added a comment. Thanks for the report, but the fix is not quite correct. I'll fix this myself. I just want to push it to 9.0 release branch ASAP. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64889/new/ https://reviews.llvm.org/D64889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
