[Flang] Add semantic check for usage of COPYPRIVATE and NOWAIT clauses

[kiranktp]

1. COPYPRIVATE clause should be specified on END SINGLE construct.
2. NOWAIT clause should be specified on END DO/DO SIMD/SINGLE/SECTIONS construct.

Special handling for semantic checks for nowait/copyprivate clause is needed in Fortran due to the fact that Openmp pragmas for C/C++ doesn't have end directive (clause). nowait/copyprivate clauses are allowed in begin directive clause lists for C/C++ and it is not allowed for Fortran.

[llvmbot]

@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-semantics

Author: Kiran Kumar T P (kiranktp)

Changes

1. COPYPRIVATE clause should be specified on END SINGLE construct.
2. NOWAIT clause should be specified on END DO/DO SIMD/SINGLE/SECTIONS construct.

---

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

6 Files Affected:

• (modified) flang/lib/Semantics/check-omp-structure.cpp (+34-1)
• (modified) flang/lib/Semantics/check-omp-structure.h (+11)
• (modified) flang/test/Semantics/OpenMP/clause-validity01.f90 (+6-3)
• (modified) flang/test/Semantics/OpenMP/copying.f90 (+3-3)
• (modified) flang/test/Semantics/OpenMP/threadprivate04.f90 (+4)
• (modified) llvm/include/llvm/Frontend/OpenMP/OMP.td (+10-2)

diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 53bdf57ff8efa5a..fd5a4a56a6d2773 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -745,7 +745,12 @@ void OmpStructureChecker::Enter(const parser::OmpEndLoopDirective &x) {
   // 2.7.1 end-do -> END DO [nowait-clause]
   // 2.8.3 end-do-simd -> END DO SIMD [nowait-clause]
   case llvm::omp::Directive::OMPD_do:
+    PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_end_do);
+    SetClauseSets(dir.v);
+    break;
   case llvm::omp::Directive::OMPD_do_simd:
+    PushContextAndClauseSets(
+        dir.source, llvm::omp::Directive::OMPD_end_do_simd);
     SetClauseSets(dir.v);
     break;
   default:
@@ -754,6 +759,13 @@ void OmpStructureChecker::Enter(const parser::OmpEndLoopDirective &x) {
   }
 }
 
+void OmpStructureChecker::Leave(const parser::OmpEndLoopDirective &x) {
+  if ((GetContext().directive == llvm::omp::Directive::OMPD_end_do) ||
+      (GetContext().directive == llvm::omp::Directive::OMPD_end_do_simd)) {
+    dirContext_.pop_back();
+  }
+}
+
 void OmpStructureChecker::Enter(const parser::OpenMPBlockConstruct &x) {
   const auto &beginBlockDir{std::get<parser::OmpBeginBlockDirective>(x.t)};
   const auto &endBlockDir{std::get<parser::OmpEndBlockDirective>(x.t)};
@@ -2218,7 +2230,6 @@ CHECK_SIMPLE_CLAUSE(Indirect, OMPC_indirect)
 CHECK_SIMPLE_CLAUSE(Mergeable, OMPC_mergeable)
 CHECK_SIMPLE_CLAUSE(Nogroup, OMPC_nogroup)
 CHECK_SIMPLE_CLAUSE(Notinbranch, OMPC_notinbranch)
-CHECK_SIMPLE_CLAUSE(Nowait, OMPC_nowait)
 CHECK_SIMPLE_CLAUSE(Partial, OMPC_partial)
 CHECK_SIMPLE_CLAUSE(ProcBind, OMPC_proc_bind)
 CHECK_SIMPLE_CLAUSE(Release, OMPC_release)
@@ -2456,6 +2467,19 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Private &x) {
   CheckIntentInPointer(x.v, llvm::omp::Clause::OMPC_private);
 }
 
+void OmpStructureChecker::Enter(const parser::OmpClause::Nowait &x) {
+  CheckAllowed(llvm::omp::Clause::OMPC_nowait);
+  if (llvm::omp::noWaitClauseAllowedSet.test(GetContext().directive)) {
+    context_.Say(GetContext().clauseSource,
+        "%s clause is not allowed on the OMP %s directive,"
+        " use it on OMP END %s directive "_err_en_US,
+        parser::ToUpperCaseLetters(
+            getClauseName(llvm::omp::Clause::OMPC_nowait).str()),
+        parser::ToUpperCaseLetters(GetContext().directiveSource.ToString()),
+        parser::ToUpperCaseLetters(GetContext().directiveSource.ToString()));
+  }
+}
+
 bool OmpStructureChecker::IsDataRefTypeParamInquiry(
     const parser::DataRef *dataRef) {
   bool dataRefIsTypeParamInquiry{false};
@@ -2830,6 +2854,15 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Copyprivate &x) {
   GetSymbolsInObjectList(x.v, currSymbols);
   CheckCopyingPolymorphicAllocatable(
       currSymbols, llvm::omp::Clause::OMPC_copyprivate);
+  if (GetContext().directive == llvm::omp::Directive::OMPD_single) {
+    context_.Say(GetContext().clauseSource,
+        "%s clause is not allowed on the OMP %s directive,"
+        " use it on OMP END %s directive "_err_en_US,
+        parser::ToUpperCaseLetters(
+            getClauseName(llvm::omp::Clause::OMPC_copyprivate).str()),
+        parser::ToUpperCaseLetters(GetContext().directiveSource.ToString()),
+        parser::ToUpperCaseLetters(GetContext().directiveSource.ToString()));
+  }
 }
 
 void OmpStructureChecker::Enter(const parser::OmpClause::Lastprivate &x) {
diff --git a/flang/lib/Semantics/check-omp-structure.h b/flang/lib/Semantics/check-omp-structure.h
index 90e5c9f19127750..83b8cb32ef53f49 100644
--- a/flang/lib/Semantics/check-omp-structure.h
+++ b/flang/lib/Semantics/check-omp-structure.h
@@ -33,6 +33,16 @@ static OmpClauseSet privateSet{
     Clause::OMPC_private, Clause::OMPC_firstprivate, Clause::OMPC_lastprivate};
 static OmpClauseSet privateReductionSet{
     OmpClauseSet{Clause::OMPC_reduction} | privateSet};
+// omp.td cannot differentiate allowed/not allowed clause list for few
+// directives for fortran. nowait is not allowed on begin directive clause list
+// for below list of directives. Directives with conflicting list of clauses are
+// included in below list.
+static const OmpDirectiveSet noWaitClauseAllowedSet{
+    Directive::OMPD_do,
+    Directive::OMPD_do_simd,
+    Directive::OMPD_sections,
+    Directive::OMPD_single,
+};
 } // namespace omp
 } // namespace llvm
 
@@ -61,6 +71,7 @@ class OmpStructureChecker
   void Enter(const parser::OpenMPLoopConstruct &);
   void Leave(const parser::OpenMPLoopConstruct &);
   void Enter(const parser::OmpEndLoopDirective &);
+  void Leave(const parser::OmpEndLoopDirective &);
 
   void Enter(const parser::OpenMPBlockConstruct &);
   void Leave(const parser::OpenMPBlockConstruct &);
diff --git a/flang/test/Semantics/OpenMP/clause-validity01.f90 b/flang/test/Semantics/OpenMP/clause-validity01.f90
index 976bfe6e6f785f8..3fa86ed105a292f 100644
--- a/flang/test/Semantics/OpenMP/clause-validity01.f90
+++ b/flang/test/Semantics/OpenMP/clause-validity01.f90
@@ -319,7 +319,8 @@
   !$omp parallel
   b = 1
   !ERROR: LASTPRIVATE clause is not allowed on the SINGLE directive
-  !$omp single private(a) lastprivate(c)
+  !ERROR: NOWAIT clause is not allowed on the OMP SINGLE directive, use it on OMP END SINGLE directive 
+  !$omp single private(a) lastprivate(c) nowait
   a = 3.14
   !ERROR: Clause NOWAIT is not allowed if clause COPYPRIVATE appears on the END SINGLE directive
   !ERROR: COPYPRIVATE variable 'a' may not appear on a PRIVATE or FIRSTPRIVATE clause on a SINGLE construct
@@ -399,13 +400,15 @@
   !$omp parallel
   !ERROR: No ORDERED clause with a parameter can be specified on the DO SIMD directive
   !ERROR: NOGROUP clause is not allowed on the DO SIMD directive
-  !$omp do simd ordered(2) NOGROUP
+  !ERROR: NOWAIT clause is not allowed on the OMP DO SIMD directive, use it on OMP END DO SIMD directive 
+  !$omp do simd ordered(2) NOGROUP nowait
   do i = 1, N
      do j = 1, N
         a = 3.14
      enddo
   enddo
-  !$omp end parallel
+  !omp end do nowait
+  !$omp end parallel 
 
 ! 2.11.4 parallel-do-simd-clause -> parallel-clause |
 !                                   do-simd-clause
diff --git a/flang/test/Semantics/OpenMP/copying.f90 b/flang/test/Semantics/OpenMP/copying.f90
index 171e29bf67afbfb..63fb39a0f26e59a 100644
--- a/flang/test/Semantics/OpenMP/copying.f90
+++ b/flang/test/Semantics/OpenMP/copying.f90
@@ -44,9 +44,9 @@ subroutine copyprivate()
   class(*), allocatable, save :: x
   !$omp threadprivate(x)
 
-  !PORTABILITY: If a polymorphic variable with allocatable attribute 'x' is in COPYPRIVATE clause, the behavior is unspecified
-  !$omp single copyprivate(x)
+  !$omp single
     call sub()
-  !$omp end single
+  !PORTABILITY: If a polymorphic variable with allocatable attribute 'x' is in COPYPRIVATE clause, the behavior is unspecified
+  !$omp end single copyprivate(x)
 
 end
diff --git a/flang/test/Semantics/OpenMP/threadprivate04.f90 b/flang/test/Semantics/OpenMP/threadprivate04.f90
index f523711f415671f..3d8c7fb8de8fa16 100644
--- a/flang/test/Semantics/OpenMP/threadprivate04.f90
+++ b/flang/test/Semantics/OpenMP/threadprivate04.f90
@@ -14,9 +14,13 @@ program main
   !$omp parallel num_threads(x1)
   !$omp end parallel
 
+  !ERROR: COPYPRIVATE clause is not allowed on the OMP SINGLE directive, use it on OMP END SINGLE directive 
   !$omp single copyprivate(x2, /blk1/)
   !$omp end single
 
+  !$omp single
+  !$omp end single copyprivate(x2, /blk1/)
+
   !$omp do schedule(static, x3)
   do i = 1, N
     y1 = x3
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.td b/llvm/include/llvm/Frontend/OpenMP/OMP.td
index c782c452b796569..2388abac81ceb47 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.td
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td
@@ -2018,8 +2018,16 @@ def OMP_ParallelWorkshare : Directive<"parallel workshare"> {
   ];
 }
 def OMP_Workshare : Directive<"workshare"> {}
-def OMP_EndDo : Directive<"end do"> {}
-def OMP_EndDoSimd : Directive<"end do simd"> {}
+def OMP_EndDo : Directive<"end do"> {
+  let allowedOnceClauses = [
+    VersionedClause<OMPC_NoWait>
+  ];
+}
+def OMP_EndDoSimd : Directive<"end do simd"> {
+  let allowedOnceClauses = [
+    VersionedClause<OMPC_NoWait>
+  ];
+}
 def OMP_EndSections : Directive<"end sections"> {
   let allowedOnceClauses = [
     VersionedClause<OMPC_NoWait>

[luporl]

Looks good to me overall, but I don't have much experience with OpenMP semantic checks.

[kiranktp]

Re-based. Please review.

[kiranchandramohan]

Does this check look only at the construct and not the begin or end directive?

[kiranktp]

This does look for begin or end directive. But for C/C++ we do not have end directive, so nowait clause is present in begin directive list as well in OMP.td. So this will not flag any error for Fortran when nowait is used in begin directive clause list. Removing the nowait clause from begin directive list in OMP.td will flag error for C/C++ tests.

[kiranchandramohan]

Thanks. Since the handling is different due to the C/C++ issue. It will be good to call this out in the patch summary.

[kiranchandramohan]

LG.