Skip to content

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