[flang][OpenMP] Add some semantic checks for threadprivate and declare target directives

[peixin]

Summary

This supports the following checks for THREADPRIVATE Directive:

[5.1] 2.21.2 THREADPRIVATE Directive
A threadprivate variable must not appear in any clause except the 
copyin, copyprivate, schedule, num_threads, thread_limit, and if clauses.

This supports the following checks for DECLARE TARGET Directive:

[5.1] 2.14.7 Declare Target Directive
A threadprivate variable cannot appear in the directive.

Besides, procedure name and the entity with PARAMETER attribute cannot
be in the threadprivate directive. The main program name and module name
cannot be in the threadprivate directive and declare target directive.
There is no clear description or restriction about the entity with
PARAMETER attribute in OpenMP 5.1 Specification, and a warning is given.

[peixin]

Most of semantic checks for threadprivate directive should be done after this patch.

[shraiysh]

Hi @peixin . Thanks for this patch. I have a few comments and questions for the declare target checks. I will review the threadprivate checks soon.

flang/lib/Semantics/check-omp-structure.cpp

864

I cannot find a test for this error. Can we add that?

872

Same. No tests for this.

flang/test/Semantics/omp-declarative-directive.f90

47–48

I am trying to understand the why is there a warning here. Example declare_target.3.f90 on Page 183 in the OpenMP 5.0 Examples Document allows the usage of #declare target (N). Can it be removed?

flang/test/Semantics/omp-declare-target02.f90

65–66

This should not be the error, right? As this is accepted by the OpenMP Standard. (Are we planning to remove this later?)

68–69

Why is this an error? The declaration of the common block is in the same scope. Can we have a correct behavior test for this? (when the common block is accepted without errors)

76–77

This and other similar variables like blk2_link, bl3_to etc. are not declared variables. As far as I understand, they all really represent the same case. Can the others be removed and the one case be given a random name to avoid confusion? (Let me know if I have missed something here as I am not a Fortran expert)

flang/test/Semantics/omp-declare-target03.f90

14

GFortran does not report this error. The standard says "The directive must appear in the declaration section of a scoping unit in which the common block or variable is declared.". I believe it means that the constraint is that variable should not be out-of-scope and it is not necessary that the declaration is on the same level.

[peixin]

Thanks @shraiysh for the review. Made changes according to the comments.

flang/lib/Semantics/check-omp-structure.cpp

864

Add the test case in omp-threadprivate03.f90.

872

Add the test case in omp-threadprivate03.f90.

flang/test/Semantics/omp-declarative-directive.f90

47–48

The example is truly there. As described in summary, the problem is that there is no clear description or restriction about the entity with PARAMETER attribute in OpenMP 5.1 Specification. I asked help for @Meinersbur that we asked about this issue in the openmp committee. There may be detailed discussion about the entity with PARAMETER attribute in declare target construct in OpenMP 6.0.

flang/test/Semantics/omp-declare-target02.f90

65–66

"blk2" is not "/blk2/". The common block must be specified with "/ /" in threadrpviate and declare target directives. "blk2" is undeclared variable here. Since this patch does not provide new semantic errors about undeclared variable, this test case is removed. The error of "Implicitly typed local entity 'blk2' not allowed in specification expression" was wrong, which should be fixed in another patch later.

68–69

Same as above.

76–77

Removed all the test cases of undeclared variables.

flang/test/Semantics/omp-declare-target03.f90

14

I think you are right. I do not think out one reason to not use the directive and variable in different scoping units.

[shraiysh]

The patch functionally LGTM, but I have not worked on these files and would suggest you to wait for someone else's review (someone who has worked on semantic checks for flang) before merging this.

Minor comments.

flang/lib/Semantics/check-omp-structure.cpp

876

There is no test for this. Is this supposed to be an error? (I see that you have not used _err_en_US here, so why is this message here?)

1479–1480

This same error is reported above too. Can you please explain why there is a duplication of code and if possible, can we merge the objects on which this has loop has to run (mem and name) and run this loop only once?

flang/test/Semantics/omp-declare-target02.f90

65–66

Okay, I cannot find a test case for declare target with proper usage of blocks. Can you please add that if it is not already there?

[NimishMishra]

Thanks for the patch @peixin. The semantic checks LGTM

[peixin]

Thanks @shraiysh and @NimishMishra for the review. I will wait for more reviews.

flang/lib/Semantics/check-omp-structure.cpp

876

With _en_US, it emits one warning, which is usually generated for unspecified behaviors. Please check omp-declarative-directive.f90 (two warnings for variables "N" and "M".

1479–1480

This is not duplication. The std::visit walks along the parse-tree from "ompObject.u". The "parse::Name" is for common block, and "parser::Designator" is for scalar, array, and so on. This is defined in parse-tree. You can write one simple correct example, and dump the parse-tree with the option "-fdebug-dump-parse-tree" to check it.

flang/test/Semantics/omp-declare-target02.f90

65–66

Oh, you are right. The correct example is missed in my last patch. I will add it later.

[shraiysh]

Thanks for the explanation!

[peixin]

Rebase and add the proper test case as @shraiysh mentioned.

[peixin]

Remove the test case of using common block in declare target directive since resolving declare target directive has not been finished yet. This can be supported later.

[peixin]

@kiranchandramohan Can I land this patch considering it looks ok to @NimishMishra and @shraiysh ?

[shraiysh]

@peixin I think we can merge this to make progress and if Kiran has any post-commit comments, we can address them later.

[peixin]

@shraiysh Thanks for accepting this patch. This path is the last one of semantic checks for threadprivate directive and won't block others. Most people are still on vacation. I can wait for a few more days to see if there will be more comments.

[kiranchandramohan]

LGTM.

In D114941#3214619, @shraiysh wrote:

@peixin I think we can merge this to make progress and if Kiran has any post-commit comments, we can address them later.

+1

flang/lib/Semantics/check-omp-structure.cpp

1479–1480

One trivial code-sharing will be to have a lambda that, given a symbol, tests the Threadprivate flag and prints the error.

[peixin]

Use lambda to reduce the code redundancy.

[peixin]

Thanks for @kiranchandramohan accepting this patch. Addressed the comment.

flang/lib/Semantics/check-omp-structure.cpp

1479–1480

Good idea. Fixed.

flang/lib/Semantics/check-omp-structure.cpp

 
 void OmpStructureChecker::CheckThreadprivateOrDeclareTargetVar(
     const parser::OmpObjectList &objList) {
   for (const auto &ompObject : objList.v) {
     std::visit(
         common::visitors{
             [&](const parser::Designator &) {
               if (const auto *name{parser::Unwrap<parser::Name>(ompObject)}) {
-                const auto &scope{context_.FindScope(name->symbol->name())};
-                if (FindCommonBlockContaining(*name->symbol)) {
+                const auto &declScope{
+                    GetProgramUnitContaining(name->symbol->GetUltimate())};
+                const auto *sym =
+                    declScope.parent().FindSymbol(name->symbol->name());
+                if (sym &&
+                    (sym->has<MainProgramDetails>() ||
+                        sym->has<ModuleDetails>())) {
+                  context_.Say(name->source,
+                      "The module name or main program name cannot be in a %s "
+                      "directive"_err_en_US,
+                      ContextDirectiveAsFortran());
+                } else if (name->symbol->GetUltimate().IsSubprogram()) {
+                  if (GetContext().directive ==
+                      llvm::omp::Directive::OMPD_threadprivate)
+                    context_.Say(name->source,
+                        "The procedure name cannot be in a %s "
+                        "directive"_err_en_US,
+                        ContextDirectiveAsFortran());
+                  // TODO: Check for procedure name in declare target directive.
+                } else if (name->symbol->attrs().test(Attr::PARAMETER)) {
+                  if (GetContext().directive ==
+                      llvm::omp::Directive::OMPD_threadprivate)
+                    context_.Say(name->source,
+                        "The entity with PARAMETER attribute cannot be in a %s "
+                        "directive"_err_en_US,
+                        ContextDirectiveAsFortran());
+                  else if (GetContext().directive ==
+                      llvm::omp::Directive::OMPD_declare_target)
+                    context_.Say(name->source,
+                        "The entity with PARAMETER attribute is used in a %s "
+                        "directive"_en_US,
+                        ContextDirectiveAsFortran());
+                } else if (FindCommonBlockContaining(*name->symbol)) {
                   context_.Say(name->source,
                       "A variable in a %s directive cannot be an element of a "
                       "common block"_err_en_US,
                       ContextDirectiveAsFortran());
                 } else if (!IsSave(*name->symbol) &&
-                    scope.kind() != Scope::Kind::MainProgram &&
-                    scope.kind() != Scope::Kind::Module) {
+                    declScope.kind() != Scope::Kind::MainProgram &&
+                    declScope.kind() != Scope::Kind::Module) {
                   context_.Say(name->source,
                       "A variable that appears in a %s directive must be "
                       "declared in the scope of a module or have the SAVE "
                       "attribute, either explicitly or implicitly"_err_en_US,
                       ContextDirectiveAsFortran());
-                }
-                if (FindEquivalenceSet(*name->symbol)) {
+                } else if (FindEquivalenceSet(*name->symbol)) {
                   context_.Say(name->source,
                       "A variable in a %s directive cannot appear in an "
                       "EQUIVALENCE statement"_err_en_US,
                       ContextDirectiveAsFortran());
+                } else if (name->symbol->test(Symbol::Flag::OmpThreadprivate) &&
+                    GetContext().directive ==
+                        llvm::omp::Directive::OMPD_declare_target) {
+                  context_.Say(name->source,
+                      "A THREADPRIVATE variable cannot appear in a %s "
+                      "directive"_err_en_US,
+                      ContextDirectiveAsFortran());
                 }
               }
             },
             [&](const parser::Name &) {}, // common block
         },
         ompObject.u);
   }
 }
void OmpStructureChecker::Leave(const parser::OmpClauseList &) {
   } // SIMD
 
   // 2.7.3 Single Construct Restriction
   if (GetContext().directive == llvm::omp::Directive::OMPD_end_single) {
     CheckNotAllowedIfClause(
         llvm::omp::Clause::OMPC_copyprivate, {llvm::omp::Clause::OMPC_nowait});
   }
 
+  auto testThreadprivateVarErr = [&](Symbol sym, parser::Name name,
+                                     llvmOmpClause clauseTy) {
+    if (sym.test(Symbol::Flag::OmpThreadprivate))
+      context_.Say(name.source,
+          "A THREADPRIVATE variable cannot be in %s clause"_err_en_US,
+          parser::ToUpperCaseLetters(getClauseName(clauseTy).str()));
+  };
+
+  // [5.1] 2.21.2 Threadprivate Directive Restriction
+  OmpClauseSet threadprivateAllowedSet{llvm::omp::Clause::OMPC_copyin,
+      llvm::omp::Clause::OMPC_copyprivate, llvm::omp::Clause::OMPC_schedule,
+      llvm::omp::Clause::OMPC_num_threads, llvm::omp::Clause::OMPC_thread_limit,
+      llvm::omp::Clause::OMPC_if};
+  for (auto it : GetContext().clauseInfo) {
+    llvmOmpClause type = it.first;
+    const auto *clause = it.second;
+    if (!threadprivateAllowedSet.test(type)) {
+      if (const auto *objList{GetOmpObjectList(*clause)}) {
+        for (const auto &ompObject : objList->v) {
+          std::visit(
+              common::visitors{
+                  [&](const parser::Designator &) {
+                    if (const auto *name{
+                            parser::Unwrap<parser::Name>(ompObject)})
+                      testThreadprivateVarErr(
+                          name->symbol->GetUltimate(), *name, type);
+                  },
+                  [&](const parser::Name &name) {
+                    if (name.symbol) {
+                      for (const auto &mem :
+                          name.symbol->get<CommonBlockDetails>().objects()) {
+                        testThreadprivateVarErr(mem->GetUltimate(), name, type);
+                        break;
+                      }
+                    }
+                  },
+              },
+              ompObject.u);
+        }
+      }
+    }
+  }
+
   CheckRequireAtLeastOneOf();
 }
 
 void OmpStructureChecker::Enter(const parser::OmpClause &x) {
   SetContextClause(x);
 }
 
 // Following clauses do not have a separate node in parse-tree.h.

flang/test/Semantics/omp-declarative-directive.f90

 
 ! 2.10.6 declare-target
 ! 2.15.2 threadprivate
 
 module m2
 contains
   subroutine foo
     !$omp declare target
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
+    !WARNING: The entity with PARAMETER attribute is used in a DECLARE TARGET directive
+    !WARNING: The entity with PARAMETER attribute is used in a DECLARE TARGET directive
     !$omp declare target (foo, N, M)
     !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
     !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
     !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
     !$omp declare target to(Q, S) link(R)
     !ERROR: MAP clause is not allowed on the DECLARE TARGET directive
     !$omp declare target map(from:Q)
     integer, parameter :: N=10000, M=1024

flang/test/Semantics/omp-declare-target02.f90

subroutine func()
     common /blk2/ a2, a2_to, a2_link
     common /blk3/ a3, a3_to, a3_link
     save /blk3/
 
     !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
     !$omp declare target (arr2)
 
     !$omp declare target (arr3)
 
-    !ERROR: Implicitly typed local entity 'blk2' not allowed in specification expression
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
-    !$omp declare target (blk2)
-
     !ERROR: A variable in a DECLARE TARGET directive cannot be an element of a common block
     !$omp declare target (a2)
 
-    !ERROR: Implicitly typed local entity 'blk3' not allowed in specification expression
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
-    !$omp declare target (blk3)
-
     !ERROR: A variable in a DECLARE TARGET directive cannot be an element of a common block
     !$omp declare target (a3)
 
     !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
     !$omp declare target to (arr2_to)
 
     !$omp declare target to (arr3_to)
 
-    !ERROR: Implicitly typed local entity 'blk2_to' not allowed in specification expression
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
-    !$omp declare target to (blk2_to)
-
     !ERROR: A variable in a DECLARE TARGET directive cannot be an element of a common block
     !$omp declare target to (a2_to)
 
-    !ERROR: Implicitly typed local entity 'blk3_to' not allowed in specification expression
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
-    !$omp declare target to (blk3_to)
-
     !ERROR: A variable in a DECLARE TARGET directive cannot be an element of a common block
     !$omp declare target to (a3_to)
 
     !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
     !$omp declare target link (arr2_link)
 
     !$omp declare target link (arr3_link)
 
-    !ERROR: Implicitly typed local entity 'blk2_link' not allowed in specification expression
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
-    !$omp declare target link (blk2_link)
-
     !ERROR: A variable in a DECLARE TARGET directive cannot be an element of a common block
     !$omp declare target link (a2_link)
 
-    !ERROR: Implicitly typed local entity 'blk3_link' not allowed in specification expression
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
-    !$omp declare target link (blk3_link)
-
     !ERROR: A variable in a DECLARE TARGET directive cannot be an element of a common block
     !$omp declare target link (a3_link)
   end
 end
 
 module mod4
   integer :: arr4(10), arr4_to(10), arr4_link(10)
   common /blk4/ a4, a4_to, a4_link

flang/test/Semantics/omp-declare-target03.f90

+! RUN: %python %S/test_errors.py %s %flang_fc1 -fopenmp
+! OpenMP Version 5.1
+! Check OpenMP construct validity for the following directives:
+! 2.14.7 Declare Target Directive
+
+module mod1
+end
+
+program main
+  use mod1
+
+  !ERROR: The module name or main program name cannot be in a DECLARE TARGET directive
+  !$omp declare target (mod1)
+
+  !ERROR: The module name or main program name cannot be in a DECLARE TARGET directive
+  !$omp declare target (main)
+end

flang/test/Semantics/omp-declare-target04.f90

+! RUN: %python %S/test_errors.py %s %flang_fc1 -fopenmp
+! OpenMP Version 5.1
+! Check OpenMP construct validity for the following directives:
+! 2.14.7 Declare Target Directive
+
+program main
+  integer, save :: x, y
+
+  !$omp threadprivate(x)
+
+  !ERROR: A THREADPRIVATE variable cannot appear in a DECLARE TARGET directive
+  !ERROR: A THREADPRIVATE variable cannot appear in a DECLARE TARGET directive
+  !$omp declare target (x, y)
+
+  !$omp threadprivate(y)
+end

flang/test/Semantics/omp-threadprivate02.f90

subroutine func()
 
     !ERROR: A variable that appears in a THREADPRIVATE directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
     !$omp threadprivate(arr2)
 
     !$omp threadprivate(arr3)
 
     !$omp threadprivate(/blk2/)
 
-    !ERROR: Implicitly typed local entity 'blk2' not allowed in specification expression
-    !ERROR: A variable that appears in a THREADPRIVATE directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
-    !$omp threadprivate(blk2)
-
     !ERROR: A variable in a THREADPRIVATE directive cannot be an element of a common block
     !$omp threadprivate(a2)
 
     !$omp threadprivate(/blk3/)
 
-    !ERROR: Implicitly typed local entity 'blk3' not allowed in specification expression
-    !ERROR: A variable that appears in a THREADPRIVATE directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
-    !$omp threadprivate(blk3)
-
     !ERROR: A variable in a THREADPRIVATE directive cannot be an element of a common block
     !$omp threadprivate(a3)
   end
 end
 
 module mod4
   integer :: arr4(10)
   common /blk4/ a4

flang/test/Semantics/omp-threadprivate03.f90

+! RUN: %python %S/test_errors.py %s %flang_fc1 -fopenmp
+! OpenMP Version 5.1
+! Check OpenMP construct validity for the following directives:
+! 2.21.2 Threadprivate Directive
+
+module mod1
+end
+
+program main
+  use mod1
+  integer, parameter :: i = 1
+
+  !ERROR: The module name or main program name cannot be in a THREADPRIVATE directive
+  !$omp threadprivate(mod1)
+
+  !ERROR: The module name or main program name cannot be in a THREADPRIVATE directive
+  !$omp threadprivate(main)
+
+  !ERROR: The entity with PARAMETER attribute cannot be in a THREADPRIVATE directive
+  !$omp threadprivate(i)
+
+contains
+  subroutine sub()
+    !ERROR: The procedure name cannot be in a THREADPRIVATE directive
+    !$omp threadprivate(sub)
+  end
+end

flang/test/Semantics/omp-threadprivate04.f90

+! RUN: %python %S/test_errors.py %s %flang_fc1 -fopenmp
+! OpenMP Version 5.1
+! Check OpenMP construct validity for the following directives:
+! 2.21.2 Threadprivate Directive
+
+program main
+  integer :: i, N = 10
+  integer, save :: x
+  common /blk/ y
+
+  !$omp threadprivate(x, /blk/)
+
+  !$omp parallel num_threads(x)
+  !$omp end parallel
+
+  !$omp single copyprivate(x, /blk/)
+  !$omp end single
+
+  !$omp do schedule(static, x)
+  do i = 1, N
+    y = x
+  end do
+  !$omp end do
+
+  !$omp parallel copyin(x, /blk/)
+  !$omp end parallel
+
+  !$omp parallel if(x > 1)
+  !$omp end parallel
+
+  !$omp teams thread_limit(x)
+  !$omp end teams
+
+  !ERROR: A THREADPRIVATE variable cannot be in PRIVATE clause
+  !ERROR: A THREADPRIVATE variable cannot be in PRIVATE clause
+  !$omp parallel private(x, /blk/)
+  !$omp end parallel
+
+  !ERROR: A THREADPRIVATE variable cannot be in FIRSTPRIVATE clause
+  !ERROR: A THREADPRIVATE variable cannot be in FIRSTPRIVATE clause
+  !$omp parallel firstprivate(x, /blk/)
+  !$omp end parallel
+
+  !ERROR: A THREADPRIVATE variable cannot be in SHARED clause
+  !ERROR: A THREADPRIVATE variable cannot be in SHARED clause
+  !$omp parallel shared(x, /blk/)
+  !$omp end parallel
+end