[flang-commits] [flang] [flang][OpenMP] Try to unify induction var privatization for sequential loops inside parallel regions. (PR #91116)
Kareem Ergawy via flang-commits
flang-commits at lists.llvm.org
Tue May 7 05:28:28 PDT 2024
https://github.com/ergawy updated https://github.com/llvm/llvm-project/pull/91116
>From 6ad9565006e43f2916cc41fa6dfa7f77d33a653d Mon Sep 17 00:00:00 2001
From: ergawy <kareem.ergawy at amd.com>
Date: Sat, 4 May 2024 00:02:21 -0500
Subject: [PATCH 1/4] [flang][OpenMP] Try to unify induction var privatization
This PR tries to unify the paths taken by flang to privatize the
induction variable of loops. With the changes introduced here, both
cases below are now privatized in the same way:
```fortran
subroutine nested_constructs
implicit none
integer :: i
!$omp parallel default(private)
do i = 1, 10
end do
!$omp end parallel
end subroutine
```
```fortran
subroutine nested_constructs
implicit none
integer :: i
!$omp parallel private(i)
do i = 1, 10
end do
!$omp end parallel
end subroutine
```
---
flang/lib/Semantics/resolve-directives.cpp | 1 -
flang/test/Lower/OpenMP/default-clause.f90 | 13 +++++++++----
flang/test/Semantics/OpenMP/do05-positivecase.f90 | 2 +-
flang/test/Semantics/OpenMP/symbol08.f90 | 8 ++++----
4 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index c99b1c413970e..ba013dcd5d283 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -1674,7 +1674,6 @@ void OmpAttributeVisitor::ResolveSeqLoopIndexInParallelOrTaskConstruct(
// parallel or task
if (auto *symbol{ResolveOmp(iv, Symbol::Flag::OmpPrivate, targetIt->scope)}) {
targetIt++;
- symbol->set(Symbol::Flag::OmpPreDetermined);
iv.symbol = symbol; // adjust the symbol within region
for (auto it{dirContext_.rbegin()}; it != targetIt; ++it) {
AddToContextObjectWithDSA(*symbol, Symbol::Flag::OmpPrivate, *it);
diff --git a/flang/test/Lower/OpenMP/default-clause.f90 b/flang/test/Lower/OpenMP/default-clause.f90
index c9e76780de5de..f09da04d24494 100644
--- a/flang/test/Lower/OpenMP/default-clause.f90
+++ b/flang/test/Lower/OpenMP/default-clause.f90
@@ -537,16 +537,21 @@ subroutine nested_constructs
integer :: y, z
!CHECK: omp.parallel {
-!CHECK: %[[INNER_J:.*]] = fir.alloca i32 {bindc_name = "j", pinned}
-!CHECK: %[[INNER_J_DECL:.*]]:2 = hlfir.declare %[[INNER_J]] {{.*}}
-!CHECK: %[[INNER_I:.*]] = fir.alloca i32 {bindc_name = "i", pinned}
-!CHECK: %[[INNER_I_DECL:.*]]:2 = hlfir.declare %[[INNER_I]] {{.*}}
+
!CHECK: %[[INNER_Y:.*]] = fir.alloca i32 {bindc_name = "y", pinned, uniq_name = "_QFnested_constructsEy"}
!CHECK: %[[INNER_Y_DECL:.*]]:2 = hlfir.declare %[[INNER_Y]] {{.*}}
!CHECK: %[[TEMP:.*]] = fir.load %[[Y_DECL]]#0 : !fir.ref<i32>
!CHECK: hlfir.assign %[[TEMP]] to %[[INNER_Y_DECL]]#0 temporary_lhs : i32, !fir.ref<i32>
+
+!CHECK: %[[INNER_I:.*]] = fir.alloca i32 {bindc_name = "i", pinned, uniq_name
+!CHECK: %[[INNER_I_DECL:.*]]:2 = hlfir.declare %[[INNER_I]] {{.*}}
+
+!CHECK: %[[INNER_J:.*]] = fir.alloca i32 {bindc_name = "j", pinned, uniq_name
+!CHECK: %[[INNER_J_DECL:.*]]:2 = hlfir.declare %[[INNER_J]] {{.*}}
+
!CHECK: %[[INNER_Z:.*]] = fir.alloca i32 {bindc_name = "z", pinned, uniq_name = "_QFnested_constructsEz"}
!CHECK: %[[INNER_Z_DECL:.*]]:2 = hlfir.declare %[[INNER_Z]] {{.*}}
+
!$omp parallel default(private) firstprivate(y)
!CHECK: {{.*}} = fir.do_loop {{.*}} {
do i = 1, 10
diff --git a/flang/test/Semantics/OpenMP/do05-positivecase.f90 b/flang/test/Semantics/OpenMP/do05-positivecase.f90
index 4e02235f58a1a..7fa6f00447052 100644
--- a/flang/test/Semantics/OpenMP/do05-positivecase.f90
+++ b/flang/test/Semantics/OpenMP/do05-positivecase.f90
@@ -9,7 +9,7 @@ program omp_do
!DEF: /omp_do/n ObjectEntity INTEGER(4)
integer i,n
!$omp parallel
- !DEF: /omp_do/OtherConstruct1/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+ !DEF: /omp_do/OtherConstruct1/i (OmpPrivate) HostAssoc INTEGER(4)
do i=1,10
!$omp single
print *, "hello"
diff --git a/flang/test/Semantics/OpenMP/symbol08.f90 b/flang/test/Semantics/OpenMP/symbol08.f90
index 50f34b736cdb7..64643fe76c39d 100644
--- a/flang/test/Semantics/OpenMP/symbol08.f90
+++ b/flang/test/Semantics/OpenMP/symbol08.f90
@@ -37,7 +37,7 @@ subroutine test_do
do j=6,10
!REF: /test_do/a
a(1,1,1) = 0.
- !DEF: /test_do/OtherConstruct1/k (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+ !DEF: /test_do/OtherConstruct1/k (OmpPrivate) HostAssoc INTEGER(4)
do k=11,15
!REF: /test_do/a
!REF: /test_do/OtherConstruct1/k
@@ -170,9 +170,9 @@ subroutine test_simd
!$omp parallel do simd
!DEF: /test_simd/OtherConstruct1/i (OmpLinear, OmpPreDetermined) HostAssoc INTEGER(4)
do i=1,5
- !DEF: /test_simd/OtherConstruct1/j (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+ !DEF: /test_simd/OtherConstruct1/j (OmpPrivate) HostAssoc INTEGER(4)
do j=6,10
- !DEF: /test_simd/OtherConstruct1/k (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+ !DEF: /test_simd/OtherConstruct1/k (OmpPrivate) HostAssoc INTEGER(4)
do k=11,15
!REF: /test_simd/a
!REF: /test_simd/OtherConstruct1/k
@@ -228,7 +228,7 @@ subroutine test_seq_loop
print *, i, j
!$omp parallel
!REF: /test_seq_loop/i
- !DEF: /test_seq_loop/OtherConstruct1/OtherConstruct1/j (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+ !DEF: /test_seq_loop/OtherConstruct1/OtherConstruct1/j (OmpPrivate) HostAssoc INTEGER(4)
print *, i, j
!$omp do
!DEF: /test_seq_loop/OtherConstruct1/OtherConstruct1/OtherConstruct1/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
>From 833b6004bea556c3cc968339b33cead99a700e55 Mon Sep 17 00:00:00 2001
From: ergawy <kareem.ergawy at amd.com>
Date: Mon, 6 May 2024 05:20:03 -0500
Subject: [PATCH 2/4] Add `assert(!OmpPreDetermined)`
---
flang/lib/Lower/OpenMP/DataSharingProcessor.cpp | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 3569a0f070beb..f53f62d6d17d5 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -87,10 +87,8 @@ void DataSharingProcessor::insertDeallocs() {
}
void DataSharingProcessor::cloneSymbol(const Fortran::semantics::Symbol *sym) {
- // Privatization for symbols which are pre-determined (like loop index
- // variables) happen separately, for everything else privatize here.
- if (sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined))
- return;
+ assert(!sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined));
+
bool success = converter.createHostAssociateVarClone(*sym);
(void)success;
assert(success && "Privatization failed due to existing binding");
>From e29c7d43530540193edc6e2cf8ed7a8eeb26cff1 Mon Sep 17 00:00:00 2001
From: ergawy <kareem.ergawy at amd.com>
Date: Mon, 6 May 2024 05:54:19 -0500
Subject: [PATCH 3/4] Undo assert.
---
flang/lib/Lower/OpenMP/DataSharingProcessor.cpp | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index f53f62d6d17d5..3569a0f070beb 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -87,8 +87,10 @@ void DataSharingProcessor::insertDeallocs() {
}
void DataSharingProcessor::cloneSymbol(const Fortran::semantics::Symbol *sym) {
- assert(!sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined));
-
+ // Privatization for symbols which are pre-determined (like loop index
+ // variables) happen separately, for everything else privatize here.
+ if (sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined))
+ return;
bool success = converter.createHostAssociateVarClone(*sym);
(void)success;
assert(success && "Privatization failed due to existing binding");
>From fe3562054af3e3b28fd5a23249841fa0310a09a8 Mon Sep 17 00:00:00 2001
From: ergawy <kareem.ergawy at amd.com>
Date: Tue, 7 May 2024 07:28:13 -0500
Subject: [PATCH 4/4] retain flag setting, check parallel
---
flang/lib/Lower/OpenMP/DataSharingProcessor.cpp | 12 ++++++++++--
flang/lib/Semantics/resolve-directives.cpp | 1 +
flang/test/Semantics/OpenMP/do05-positivecase.f90 | 2 +-
flang/test/Semantics/OpenMP/symbol08.f90 | 8 ++++----
4 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 82d8d8dd98ea2..f2986ad0fab03 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -94,8 +94,17 @@ void DataSharingProcessor::insertDeallocs() {
void DataSharingProcessor::cloneSymbol(const Fortran::semantics::Symbol *sym) {
// Privatization for symbols which are pre-determined (like loop index
// variables) happen separately, for everything else privatize here.
- if (sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined))
+ auto isOMPParallelConstruct = [](Fortran::lower::pft::Evaluation &eval) {
+ if (const auto *ompConstruct = eval.getIf<parser::OpenMPConstruct>())
+ if (std::holds_alternative<parser::OpenMPBlockConstruct>(ompConstruct->u))
+ return true;
+ return false;
+ };
+
+ if (sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined) &&
+ !isOMPParallelConstruct(eval))
return;
+
bool success = converter.createHostAssociateVarClone(*sym);
(void)success;
assert(success && "Privatization failed due to existing binding");
@@ -344,7 +353,6 @@ void DataSharingProcessor::collectSymbols(
assert(curScope && "couldn't find current scope");
if (isPrivatizable(*sym) && !symbolsInNestedRegions.contains(sym) &&
!privatizedSymbols.contains(sym) &&
- !sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined) &&
(collectImplicit ||
!sym->test(Fortran::semantics::Symbol::Flag::OmpImplicit)) &&
clauseScopes.contains(&sym->owner()))
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index b82d300a2864c..2add2056f658d 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -1674,6 +1674,7 @@ void OmpAttributeVisitor::ResolveSeqLoopIndexInParallelOrTaskConstruct(
// parallel or task
if (auto *symbol{ResolveOmp(iv, Symbol::Flag::OmpPrivate, targetIt->scope)}) {
targetIt++;
+ symbol->set(Symbol::Flag::OmpPreDetermined);
iv.symbol = symbol; // adjust the symbol within region
for (auto it{dirContext_.rbegin()}; it != targetIt; ++it) {
AddToContextObjectWithDSA(*symbol, Symbol::Flag::OmpPrivate, *it);
diff --git a/flang/test/Semantics/OpenMP/do05-positivecase.f90 b/flang/test/Semantics/OpenMP/do05-positivecase.f90
index 7fa6f00447052..4e02235f58a1a 100644
--- a/flang/test/Semantics/OpenMP/do05-positivecase.f90
+++ b/flang/test/Semantics/OpenMP/do05-positivecase.f90
@@ -9,7 +9,7 @@ program omp_do
!DEF: /omp_do/n ObjectEntity INTEGER(4)
integer i,n
!$omp parallel
- !DEF: /omp_do/OtherConstruct1/i (OmpPrivate) HostAssoc INTEGER(4)
+ !DEF: /omp_do/OtherConstruct1/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
do i=1,10
!$omp single
print *, "hello"
diff --git a/flang/test/Semantics/OpenMP/symbol08.f90 b/flang/test/Semantics/OpenMP/symbol08.f90
index 52b7567b945e0..3af85af74ee97 100644
--- a/flang/test/Semantics/OpenMP/symbol08.f90
+++ b/flang/test/Semantics/OpenMP/symbol08.f90
@@ -37,7 +37,7 @@ subroutine test_do
do j=6,10
!REF: /test_do/a
a(1,1,1) = 0.
- !DEF: /test_do/OtherConstruct1/k (OmpPrivate) HostAssoc INTEGER(4)
+ !DEF: /test_do/OtherConstruct1/k (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
do k=11,15
!REF: /test_do/a
!REF: /test_do/OtherConstruct1/k
@@ -170,9 +170,9 @@ subroutine test_simd
!$omp parallel do simd
!DEF: /test_simd/OtherConstruct1/i (OmpLinear, OmpPreDetermined) HostAssoc INTEGER(4)
do i=1,5
- !DEF: /test_simd/OtherConstruct1/j (OmpPrivate) HostAssoc INTEGER(4)
+ !DEF: /test_simd/OtherConstruct1/j (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
do j=6,10
- !DEF: /test_simd/OtherConstruct1/k (OmpPrivate) HostAssoc INTEGER(4)
+ !DEF: /test_simd/OtherConstruct1/k (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
do k=11,15
!REF: /test_simd/a
!REF: /test_simd/OtherConstruct1/k
@@ -228,7 +228,7 @@ subroutine test_seq_loop
print *, i, j
!$omp parallel
!REF: /test_seq_loop/i
- !DEF: /test_seq_loop/OtherConstruct1/OtherConstruct1/j (OmpPrivate) HostAssoc INTEGER(4)
+ !DEF: /test_seq_loop/OtherConstruct1/OtherConstruct1/j (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
print *, i, j
!$omp do
!DEF: /test_seq_loop/OtherConstruct1/OtherConstruct1/OtherConstruct1/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
More information about the flang-commits
mailing list