[flang][OpenMP] Try to unify induction var privatization for OMP regions. #91116

ergawy · 2024-05-05T08:46:06Z

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:

subroutine foo
    implicit none
    integer :: i

    !$omp parallel default(private)
      do i = 1, 10
      end do
    !$omp end parallel
end subroutine

subroutine foo
    implicit none
    integer :: i

    !$omp parallel private(i)
      do i = 1, 10
      end do
    !$omp end parallel
end subroutine

llvmbot · 2024-05-05T08:46:33Z

@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-semantics

Author: Kareem Ergawy (ergawy)

Changes

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:

subroutine nested_constructs
    implicit none
    integer :: i

    !$omp parallel default(private)
      do i = 1, 10
      end do
    !$omp end parallel
end subroutine

subroutine nested_constructs
    implicit none
    integer :: i

    !$omp parallel private(i)
      do i = 1, 10
      end do
    !$omp end parallel
end subroutine

Full diff: https://github.com/llvm/llvm-project/pull/91116.diff

4 Files Affected:

(modified) flang/lib/Semantics/resolve-directives.cpp (-1)
(modified) flang/test/Lower/OpenMP/default-clause.f90 (+9-4)
(modified) flang/test/Semantics/OpenMP/do05-positivecase.f90 (+1-1)
(modified) flang/test/Semantics/OpenMP/symbol08.f90 (+4-4)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index c99b1c413970ef..ba013dcd5d2831 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 c9e76780de5de6..f09da04d244946 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 4e02235f58a1a4..7fa6f004470529 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 50f34b736cdb77..64643fe76c39d4 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)

ergawy · 2024-05-06T10:01:44Z

With this change, both private(i) and default(private) go through DataSharingProcessor::doPrivatize(...) to emit the privatization logic for i.

If you agree with this change, we will be able to switch delayed privatization by default (see #90945). With the changes in this PR, we would get a crash for cases where default(private) is specified for the induction variable. The crash happened because DataSharingProcessor::cloneSymbol(...) would return directly for OmpPreDetermined symbols which prevents delayed privatization from being properly applied to the induction variable.

@ reviewers Please take a look and let me know what you think.

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 ```

kiranchandramohan · 2024-05-06T10:13:33Z

Could you check why the CI is failing?

Try to unify induction var privatization

Could you clarify that this is for sequential loops inside parallel region? We have a separate case for induction variables of worksharing loops.

If you agree with this change, we will be able to switch delayed privatization by default (see #90945).

We should enable delayed privatization for all constructs that are currently supported before switching it on. Otherwise we will regress on existing enabled applications. This will involve adding translation support for constructs for which there is support already.

sections, single, worksharing loop, simd (?), task (only works for non-deferred execution).
In some cases a barrier has to be added after first-privatisation. I think this is the case where there are both first and lastprivate clauses specified.

Or do you mean selectiveley enabling privatization for the parallel construct?

ergawy · 2024-05-06T10:28:03Z

Could you check why the CI is failing?

I added an assertion to test that pre-determined is not set. But seems to be causing issues. Looking into it now...

Or do you mean selectiveley enabling privatization for the parallel construct?

Yes, that is what I meants since parallel is now supported across the pipeline so I was thinking of using delayed privatization for it as early as possible while working on other constructs.

ergawy · 2024-05-06T10:58:59Z

Could you check why the CI is failing?

I added the assertion prematurely and it was triggering for IVs in worksharing loops as you (@kiranchandramohan) pointed out.

I will limit the scope of this PR for sequential loops inside omp parallel regions then.

flang/lib/Semantics/resolve-directives.cpp

kiranchandramohan · 2024-05-06T11:08:48Z

Or do you mean selectiveley enabling privatization for the parallel construct?

Yes, that is what I meants since parallel is now supported across the pipeline so I was thinking of using delayed privatization for it as early as possible while working on other constructs.

Makes sense.

kiranchandramohan · 2024-05-06T11:11:41Z

For testing, please run the gfortran testsuite. Some of our CI will test this.

git clone https://github.com/llvm/llvm-test-suite.git
cd llvm-test-suite
mkdir build
cd build
cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=$HOME/llvm-project/build_release/bin/clang  -DCMAKE_CXX_COMPILER=$HOME/llvm-project/build_release/bin/clang++ -DCMAKE_Fortran_COMPILER=$HOME/llvm-project/build_release/bin/flang-new -DTEST_SUITE_FORTRAN=On -DTEST_SUITE_SUBDIRS=Fortran -DTEST_SUITE_FORTRAN_ISO_C_HEADER_DIR=$HOME/llvm-project/flang/include/flang ../
make -j48
NO_STOP_MESSAGE=1 $HOME/llvm-project/build_release/bin/llvm-lit -v  .

The fujitsu testsuite also is available. Some tests might be failing, so if you using this just ensure that there are no additional failures.
https://github.com/fujitsu/compiler-test-suite

ergawy · 2024-05-06T13:46:57Z

For testing, please run the gfortran testsuite. Some of our CI will test this.
git clone https://github.com/llvm/llvm-test-suite.git
cd llvm-test-suite
mkdir build
cd build
cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=$HOME/llvm-project/build_release/bin/clang  -DCMAKE_CXX_COMPILER=$HOME/llvm-project/build_release/bin/clang++ -DCMAKE_Fortran_COMPILER=$HOME/llvm-project/build_release/bin/flang-new -DTEST_SUITE_FORTRAN=On -DTEST_SUITE_SUBDIRS=Fortran -DTEST_SUITE_FORTRAN_ISO_C_HEADER_DIR=$HOME/llvm-project/flang/include/flang ../
make -j48
NO_STOP_MESSAGE=1 $HOME/llvm-project/build_release/bin/llvm-lit -v  .
The fujitsu testsuite also is available. Some tests might be failing, so if you using this just ensure that there are no additional failures. https://github.com/fujitsu/compiler-test-suite

Thanks for the starting instructions.

Does this currently work for flang-new out of the box?

/home/kaergawy/git/llvm-test-suite/build/Fortran/UnitTests/hello && /home/kaergawy/git/trunk18.0/build/llvm-project-2/bin/flang-new -DNDEBUG  -w -Werror=date-time -c /home/kaergawy/git/llvm-test-suite/Fortran/UnitTests/hello/hello.f90 -o CMakeFiles/hello.dir/hello.f90.o

kiranchandramohan · 2024-05-06T14:01:52Z

For testing, please run the gfortran testsuite. Some of our CI will test this.
git clone https://github.com/llvm/llvm-test-suite.git
cd llvm-test-suite
mkdir build
cd build
cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=$HOME/llvm-project/build_release/bin/clang  -DCMAKE_CXX_COMPILER=$HOME/llvm-project/build_release/bin/clang++ -DCMAKE_Fortran_COMPILER=$HOME/llvm-project/build_release/bin/flang-new -DTEST_SUITE_FORTRAN=On -DTEST_SUITE_SUBDIRS=Fortran -DTEST_SUITE_FORTRAN_ISO_C_HEADER_DIR=$HOME/llvm-project/flang/include/flang ../
make -j48
NO_STOP_MESSAGE=1 $HOME/llvm-project/build_release/bin/llvm-lit -v  .
The fujitsu testsuite also is available. Some tests might be failing, so if you using this just ensure that there are no additional failures. https://github.com/fujitsu/compiler-test-suite
Thanks for the starting instructions.

Does this currently work for flang-new out of the box?
/home/kaergawy/git/llvm-test-suite/build/Fortran/UnitTests/hello && /home/kaergawy/git/trunk18.0/build/llvm-project-2/bin/flang-new -DNDEBUG  -w -Werror=date-time -c /home/kaergawy/git/llvm-test-suite/Fortran/UnitTests/hello/hello.f90 -o CMakeFiles/hello.dir/hello.f90.o

The gfortran testsuite works out of the box. The Fujitsu testsuite has instructions for working with the llvm testsuite. https://github.com/fujitsu/compiler-test-suite/blob/main/RUN.md

…_paths

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

…_paths

ergawy

Thanks for the review. And sorry for the delay here, #90098 changed how things work and I had to find a solution for broken tests after merging with it.

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

…_paths

flang/lib/Lower/OpenMP/Decomposer.cpp

…_paths

kiranchandramohan

LGTM. Thanks for the refactoring here.

kiranchandramohan · 2024-05-16T13:01:03Z

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

-                                   /*collectSymbols=*/true,
-                                   /*collectHostAssociatedSymbols=*/false);
+      else {
+        bool isOrderedConstruct = [&]() {


I think we can fix this separately. Could you check whether the same issue happens with the Critical construct?

…_paths

luporl

LGTM.

…_paths

omjavaid · 2024-05-21T01:48:59Z

This has broken LLVM testsuite on various bots
https://lab.llvm.org/buildbot/#/builders/184/builds/12760
https://lab.llvm.org/buildbot/#/builders/197/builds/14376
https://lab.llvm.org/buildbot/#/builders/179/builds/10176
Its been failing for 2 days so I am going to revert this change for now. Kindly re-apply after fixing the failures.

…OMP regions. (#91116)" This reverts commit 2a97b50. It has broken LLVM testsuite on various bots https://lab.llvm.org/buildbot/#/builders/184/builds/12760 https://lab.llvm.org/buildbot/#/builders/197/builds/14376 https://lab.llvm.org/buildbot/#/builders/179/builds/10176

ergawy · 2024-05-21T08:06:44Z

This has broken LLVM testsuite on various bots https://lab.llvm.org/buildbot/#/builders/184/builds/12760 https://lab.llvm.org/buildbot/#/builders/197/builds/14376 https://lab.llvm.org/buildbot/#/builders/179/builds/10176 Its been failing for 2 days so I am going to revert this change for now. Kindly re-apply after fixing the failures.

Thanks for the revert and sorry for not noticing this. Yesterday was a public holiday and did not check my email. Will look into the failures.

… OMP regions. (llvm#91116)" This reapplies the referenced PR after finding a fix for the previously failing test suite tests.

This PR contains 2 commits: 1. A commit to reapply changes introduced #91116 (was reverted earlier due to test suite failures) 2. A commit containing a possible solution for the issue causing the test suite failures. In particular, it introduces a simple symbol visitor class to keep track of the current active OMP construct and marking this active construct as the scope defining the symbol being visisted.

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp flang:semantics labels May 5, 2024

ergawy requested review from NimishMishra, kiranchandramohan and vzakhari May 5, 2024 10:45

ergawy force-pushed the unify_privatization_paths branch from c0173b7 to 66ca0dd Compare May 6, 2024 10:01

ergawy force-pushed the unify_privatization_paths branch from 66ca0dd to 6ad9565 Compare May 6, 2024 10:11

Add assert(!OmpPreDetermined)

833b600

Undo assert.

e29c7d4

ergawy changed the title ~~[flang][OpenMP] Try to unify induction var privatization~~ [flang][OpenMP] Try to unify induction var privatization for sequential loops inside parallel regions. May 6, 2024

kiranchandramohan reviewed May 6, 2024

View reviewed changes

flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved

ergawy added 3 commits May 7, 2024 03:43

Merge remote-tracking branch 'upstream/main' into unify_privatization…

1bf0e55

…_paths

Merge remote-tracking branch 'upstream/main' into unify_privatization…

d74cb3e

…_paths

retain flag setting, check parallel

fe35620

ergawy commented May 7, 2024

View reviewed changes

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp Outdated Show resolved Hide resolved

ergawy requested a review from luporl May 7, 2024 12:30

ergawy added 2 commits May 8, 2024 03:16

Merge remote-tracking branch 'upstream/main' into unify_privatization…

fd2d53f

…_paths

collect all privatized symbols in one list

81f4e77

ergawy added 3 commits May 14, 2024 21:59

Merge remote-tracking branch 'upstream/main' into unify_privatization…

1023e05

…_paths

collect pre-determined only for last item in the construct queue

0fe21cf

handle some review comments

eb20e54

ergawy commented May 15, 2024

View reviewed changes

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp Show resolved Hide resolved

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp Outdated Show resolved Hide resolved

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp Outdated Show resolved Hide resolved

ergawy added 3 commits May 14, 2024 23:26

Merge remote-tracking branch 'upstream/main' into unify_privatization…

b4612b3

…_paths

balance braces

9c909d0

Merge remote-tracking branch 'upstream/main' into unify_privatization…

cbdb1e5

…_paths

kparzysz reviewed May 15, 2024

View reviewed changes

flang/lib/Lower/OpenMP/Decomposer.cpp Outdated Show resolved Hide resolved

ergawy added 2 commits May 15, 2024 07:42

Merge remote-tracking branch 'upstream/main' into unify_privatization…

799c616

…_paths

review comments

25b8b9d

kiranchandramohan approved these changes May 16, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into unify_privatization…

33114ee

…_paths

luporl approved these changes May 16, 2024

View reviewed changes

ergawy added 4 commits May 16, 2024 08:21

format

dcc1558

handle critical

9bb2215

Merge remote-tracking branch 'upstream/main' into unify_privatization…

3e5c63c

…_paths

handle merge changes

6f1bcee

ergawy merged commit 2a97b50 into llvm:main May 18, 2024
4 checks passed

PeddleSpam mentioned this pull request May 20, 2024

[AMDGPU] Fix error in #88512. #92770

Merged

ergawy mentioned this pull request May 23, 2024

Reapply #91116 with fix #93160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang][OpenMP] Try to unify induction var privatization for OMP regions. #91116

[flang][OpenMP] Try to unify induction var privatization for OMP regions. #91116

ergawy commented May 5, 2024 •

edited

Loading

llvmbot commented May 5, 2024 •

edited

Loading

ergawy commented May 6, 2024 •

edited

Loading

kiranchandramohan commented May 6, 2024

ergawy commented May 6, 2024

ergawy commented May 6, 2024

kiranchandramohan commented May 6, 2024

kiranchandramohan commented May 6, 2024

ergawy commented May 6, 2024

kiranchandramohan commented May 6, 2024

ergawy left a comment

kiranchandramohan left a comment

kiranchandramohan May 16, 2024

luporl left a comment

omjavaid commented May 21, 2024

ergawy commented May 21, 2024

[flang][OpenMP] Try to unify induction var privatization for OMP regions. #91116

[flang][OpenMP] Try to unify induction var privatization for OMP regions. #91116

Conversation

ergawy commented May 5, 2024 • edited Loading

llvmbot commented May 5, 2024 • edited Loading

ergawy commented May 6, 2024 • edited Loading

kiranchandramohan commented May 6, 2024

ergawy commented May 6, 2024

ergawy commented May 6, 2024

kiranchandramohan commented May 6, 2024

kiranchandramohan commented May 6, 2024

ergawy commented May 6, 2024

kiranchandramohan commented May 6, 2024

ergawy left a comment

Choose a reason for hiding this comment

kiranchandramohan left a comment

Choose a reason for hiding this comment

kiranchandramohan May 16, 2024

Choose a reason for hiding this comment

luporl left a comment

Choose a reason for hiding this comment

omjavaid commented May 21, 2024

ergawy commented May 21, 2024

ergawy commented May 5, 2024 •

edited

Loading

llvmbot commented May 5, 2024 •

edited

Loading

ergawy commented May 6, 2024 •

edited

Loading