Skip to content

[flang][OpenMP] Don't try to privatize FORALL/DO CONCURRENT indices #123341

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

luporl
Copy link
Contributor

@luporl luporl commented Jan 17, 2025

FORALL/DO CONCURRENT indices have predetermined private DSA (OpenMP 5.2 5.1.1).

As FORALL/DO CONCURRENT indices are defined in the construct itself, and OpenMP
directives may not appear in it, they are already private and don't need to be modified.

Fixes #100919
Fixes #120023
Fixes #123537

FORALL indices have predetermined private DSA (OpenMP 5.2 5.1.1).

As lowering already makes them private, don't modify FORALL index
symbols.

Fixes llvm#120023
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:openmp flang:semantics labels Jan 17, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 17, 2025

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Leandro Lupori (luporl)

Changes

FORALL indices have predetermined private DSA (OpenMP 5.2 5.1.1).

As lowering already makes them private, don't modify FORALL index
symbols.

Fixes #120023


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

2 Files Affected:

  • (modified) flang/lib/Semantics/resolve-directives.cpp (+1)
  • (added) flang/test/Semantics/OpenMP/forall.f90 (+30)
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 39478b58a9070d..878021e0f71609 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2119,6 +2119,7 @@ static bool IsPrivatizable(const Symbol *sym) {
           *sym) && /* OpenMP 5.2, 5.1.1: Assumed-size arrays are shared*/
       !sym->owner().IsDerivedType() &&
       sym->owner().kind() != Scope::Kind::ImpliedDos &&
+      sym->owner().kind() != Scope::Kind::Forall &&
       !sym->detailsIf<semantics::AssocEntityDetails>() &&
       !sym->detailsIf<semantics::NamelistDetails>() &&
       (!misc ||
diff --git a/flang/test/Semantics/OpenMP/forall.f90 b/flang/test/Semantics/OpenMP/forall.f90
new file mode 100644
index 00000000000000..c7fb8c64edb24e
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/forall.f90
@@ -0,0 +1,30 @@
+! RUN: %python %S/../test_symbols.py %s %flang_fc1 -fopenmp
+
+! OpenMP 5.2 5.1.1 Variables Referenced in a Construct
+! FORALL indices have predetermined private DSA.
+! As lowering already makes them private, check that their symbols are not
+! modified.
+
+  !DEF: /MainProgram1/a ObjectEntity INTEGER(4)
+  !DEF: /MainProgram1/b ObjectEntity INTEGER(4)
+  integer a(5), b(5)
+
+  !REF: /MainProgram1/a
+  a = 0
+  !REF: /MainProgram1/b
+  b = 0
+
+  !$omp parallel
+    !DEF: /MainProgram1/OtherConstruct1/Forall1/i (Implicit) ObjectEntity INTEGER(4)
+    !DEF: /MainProgram1/OtherConstruct1/a HostAssoc INTEGER(4)
+    !DEF: /MainProgram1/OtherConstruct1/b HostAssoc INTEGER(4)
+    forall(i = 1:5) a(i) = b(i) * 2
+  !$omp end parallel
+
+  !$omp parallel default(private)
+    !DEF: /MainProgram1/OtherConstruct2/Forall1/i (Implicit) ObjectEntity INTEGER(4)
+    !DEF: /MainProgram1/OtherConstruct2/a (OmpPrivate) HostAssoc INTEGER(4)
+    !DEF: /MainProgram1/OtherConstruct2/b (OmpPrivate) HostAssoc INTEGER(4)
+    forall(i = 1:5) a(i) = b(i) * 2
+  !$omp end parallel
+end program

@kiranchandramohan
Copy link
Contributor

@luporl Can you check whether the issue with Do Concurrent is also similar and can be fixed along with this patch?
#100919

@luporl luporl changed the title [flang][OpenMP] Don't try to privatize FORALL indices [flang][OpenMP] Don't try to privatize FORALL/DO CONCURRENT indices Jan 17, 2025
@@ -1777,28 +1777,13 @@ void OmpAttributeVisitor::ResolveSeqLoopIndexInParallelOrTaskConstruct(
// Use of DO CONCURRENT inside OpenMP construct is unspecified behavior
// till OpenMP-5.0 standard.
// In above both cases we skip the privatization of iteration variables.
// [OpenMP 5.1] DO CONCURRENT indices are private
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would be better to include a comment like the one below to explain why nothing is done for DO CONCURRENT in this function:

// DO CONCURRENT indices have predetermined private DSA, but as they are
// defined in the construct itself, and OpenMP directives may not appear in it,
// they are already private.

@kiranchandramohan
Copy link
Contributor

I was hoping that it would be the same change required for both FORALL and DOConcurrent.
I am slightly concerned that the handling for do-concurrent is different from the do indices. May be run the gfortran tests also once before submitting.

Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG.

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@luporl
Copy link
Contributor Author

luporl commented Jan 20, 2025

Thanks for the reviews. I've run the gfortran tests and there were no failures.

@luporl luporl merged commit 9c464e6 into llvm:main Jan 20, 2025
8 checks passed
@luporl luporl deleted the luporl-fix-forall branch January 20, 2025 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:openmp flang:semantics flang Flang issues not falling into any other category
Projects
None yet
4 participants