[flang][OpenMP] Make static duration variables default to shared DSA

[mrkajetanp]

According to the OpenMP standard, variables with static storage duration are predetermined as shared.
Add a check when creating implicit symbols for OpenMP to fix them erroneously getting set to firstprivate.

Fixes #140732.

[llvmbot]

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Kajetan Puchalski (mrkajetanp)

Changes

According to the OpenMP standard, variables with static storage duration are predetermined as shared.
Add a check when creating implicit symbols for OpenMP to fix them erroneously getting set to firstprivate.

Fixes llvm#140732.

---

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

2 Files Affected:

• (modified) flang/lib/Semantics/resolve-directives.cpp (+4-1)
• (modified) flang/test/Semantics/OpenMP/implicit-dsa.f90 (+20)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index ec385b7baf8bd..d1495e7b3b7f2 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2310,6 +2310,8 @@ void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
     bool targetDir = llvm::omp::allTargetSet.test(dirContext.directive);
     bool parallelDir = llvm::omp::allParallelSet.test(dirContext.directive);
     bool teamsDir = llvm::omp::allTeamsSet.test(dirContext.directive);
+    bool isStaticStorageDuration =
+        symbol->flags().test(Symbol::Flag::InDataStmt);
 
     if (dsa.any()) {
       if (parallelDir || taskGenDir || teamsDir) {
@@ -2367,7 +2369,8 @@ void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
       dsa = prevDSA;
     } else if (taskGenDir) {
       // TODO 5) dummy arg in orphaned taskgen construct -> firstprivate
-      if (prevDSA.test(Symbol::Flag::OmpShared)) {
+      // variables with static storage duration are predetermined as shared
+      if (prevDSA.test(Symbol::Flag::OmpShared) || isStaticStorageDuration) {
         // 6) shared in enclosing context -> shared
         dsa = {Symbol::Flag::OmpShared};
         makeSymbol(dsa);
diff --git a/flang/test/Semantics/OpenMP/implicit-dsa.f90 b/flang/test/Semantics/OpenMP/implicit-dsa.f90
index 7e38435274b7b..9e48dd2a83257 100644
--- a/flang/test/Semantics/OpenMP/implicit-dsa.f90
+++ b/flang/test/Semantics/OpenMP/implicit-dsa.f90
@@ -169,3 +169,23 @@ subroutine implicit_dsa_test8
     end do
   !$omp end task
 end subroutine
+
+! Test static storage duration variables default to shared DSA
+!DEF: /implicit_dsa_test9_mod Module
+module implicit_dsa_test9_mod
+ !DEF: /implicit_dsa_test9_mod/tm3a PUBLIC (InDataStmt) ObjectEntity COMPLEX(4)
+  complex tm3a/(0,0)/
+contains
+ !DEF: /implicit_dsa_test9_mod/implict_dsa_test9 PUBLIC (Subroutine) Subprogram
+  subroutine implict_dsa_test9
+    !$omp task
+      !$omp task
+        !DEF: /implicit_dsa_test9_mod/implict_dsa_test9/OtherConstruct1/OtherConstruct1/tm3a (OmpShared) HostAssoc COMPLEX(4)
+        tm3a = (1, 2)
+      !$omp end task
+    !$omp end task
+  !$omp taskwait
+  !REF: /implicit_dsa_test9_mod/tm3a
+  print *,tm3a
+  end subroutine
+end module

[kiranchandramohan]

The OpenMP standard says the following for static storage duration:

For Fortran, the lifetime of a variable with a SAVE attribute, implicit or explicit, a
common block object or a variable declared in a module.

Could you comment on whether all these cases (variable with SAVE, common block object, variables in module) are handled in this patch?

[mrkajetanp]

Could you comment on whether all these cases (variable with SAVE, common block object, variables in module) are handled in this patch?

The patch checks for the InDataStmt flag, which I think should be set in all of these cases. I'll add tests for those alongside the changes Tom asked for to make sure.

[mrkajetanp]

I've checked the other cases. Variables with SAVE and common block objects do not actually have any Symbol flags set on them as things stand, so with the patch as-is they will not be shared. Should the approach then be to always determine whether the symbol has static storage duration and if so make it shared? I think that'd require adding some more plumbing to get the right information.

[tblah]

This LGTM but please don't merge without approval from @luporl.

I'm a bit surprised there wasn't already a flag for common blocks but it seems like you are right.

[luporl]

LGTM, thanks!

Note that I'm assuming the reason for static storage variables defaulting to shared DSA is:

• Implicit rule that says that in a task generating construct, with no default clause, a variable that is shared in the enclosing context is determined to be shared.
• When the enclosing context is not an OpenMP directive, a variable is shared if it has static storage duration.

[eugeneepshteyn]

Hello, this PR caused regression in the following Fujitsu tests:

• https://github.com/fujitsu/compiler-test-suite/blob/main/Fortran/1050/1050_0196.f90
• https://github.com/fujitsu/compiler-test-suite/blob/main/Fortran/1050/1050_0197.f90
• https://github.com/fujitsu/compiler-test-suite/blob/main/Fortran/1050/1050_0198.f90

$ flang -fopenmp -fopenmp-version=50  1050_0196.f90 && ./a.out
flang-21: warning: OpenMP support in flang is still experimental [-Wexperimental-option]
 NG: 6 a= 16
 NG: 5 a= 15
 NG: 2 a= 12
 NG: 3 a= 13
 NG: 9 a= 19
 NG: 1 a= 11
 NG: 4 a= 14
 NG: 10 a= 20
 NG: 7 a= 17
 NG: 8 a= 18
 pass

"NG" (probably means "not good") should not appear in the output. When I compile flang with the commit just before the one in this PR, the tests pass.

Now, it's possible that these tests are incorrect or perhaps not everything needed by these tests is implemented. Could someone please take a look? I could file a new issue for flang/OpenMP, if it's easier. Thanks!

[mrkajetanp]

Very interesting, you're right, though I'm quite surprised it's hitting that code path.
I'd have expected it to bail out here because 'a' in the test program is explicitly made private and should already have a DSA. But it does not seem to be the case when this runs. Unless this check is not supposed to cover this scenario at all?

[mrkajetanp]

The issue is that the current implementation checks the ultimate symbol rather than the 'enclosing' symbol. In this case because there are multiple nesting levels, the enclosing symbol is actually an implicitly created one rather than the ultimate one.

  Module scope: shvar size=4 alignment=4 sourceRange=56 bytes
    a, PUBLIC size=4 offset=0: ObjectEntity type: INTEGER(4) init:5_4 <-- ultimate; static storage duration
  Subprogram scope: sub size=4 alignment=4 sourceRange=256 bytes
    a: Use from a in shvar
    i size=4 offset=0: ObjectEntity type: INTEGER(4)
    sub (Subroutine): HostAssoc
    OtherConstruct scope: size=0 alignment=1 sourceRange=181 bytes
      a (OmpPrivate, OmpExplicit): HostAssoc <-- private because it is explicitly made so
      i (OmpPrivate, OmpPreDetermined): HostAssoc
      OtherConstruct scope: size=0 alignment=1 sourceRange=77 bytes
        a (OmpShared): HostAssoc <-- should be private because 'a' is not shared in the enclosing context
        i (OmpFirstPrivate, OmpImplicit): HostAssoc

[luporl]

It looks like we're not handling the DSA of the enclosing symbol correctly in implicit rule "6".
I didn't notice it in the review.

If the flags of the enclosing context symbol have any DSA set, we should not consider whether the ultimate symbol has static storage duration or not.

Replacing

if (prevDSA.test(Symbol::Flag::OmpShared) || isStaticStorageDuration) {

by

if (prevDSA.test(Symbol::Flag::OmpShared) || (isStaticStorageDuration && (prevDSA & dataSharingAttributeFlags).none())) {

should fix it.

[mrkajetanp]

Good suggestion, thanks! This makes it work for me. PR here: #143601