[Flang][OpenMP] Allow copyprivate and nowait on the directive clauses #127769

Thirumalai-Shaktivel · 2025-02-19T08:55:23Z

Issue:

Single construct used to throw semantic error for copyprivate and nowiat clause when used in the single directive.
Also, the Nowait and copyprivate restrictions has be removed from OpenMP 6.0

Fix:

The above mentioned semantic error was valid until OpenMP 5.1.
From OpenMP 5.2, copyprivate and nowait is allowed in both clause and end-clause

From Reference guide (OpenMP 5.2, 2.10.2):

!$omp single [clause[ [,]clause] ... ]
loosely-structured-block
!$omp end single [end-clause[ [,]end-clause] ...]

clause:
  copyprivate (list)
  nowait
  [...]

end-clause:
  copyprivate (list)
  nowait

Towards: #110008

Issue: - Single construct used to throw semantic error for copyprivate and nowiat clause when used in the single directive. - Also, the Nowait and copyprivate restrictions has be removed from OpenMP 6.0 Fix: - The above mentioned semantic error was valid until OpenMP 5.1. - From OpenMP 5.2, copyprivate and nowait is allowed in both clause and end-clause From Reference guide (OpenMP 5.2, 2.10.2): ``` !$omp single [clause[ [,]clause] ... ] loosely-structured-block !$omp end single [end-clause[ [,]end-clause] ...] clause: copyprivate (list) nowait [...] end-clause: copyprivate (list) nowait ```

llvmbot · 2025-02-19T08:55:58Z

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Thirumalai Shaktivel (Thirumalai-Shaktivel)

Changes

Issue:

Single construct used to throw semantic error for copyprivate and nowiat clause when used in the single directive.
Also, the Nowait and copyprivate restrictions has be removed from OpenMP 6.0

Fix:

The above mentioned semantic error was valid until OpenMP 5.1.
From OpenMP 5.2, copyprivate and nowait is allowed in both clause and end-clause

From Reference guide (OpenMP 5.2, 2.10.2):

!$omp single [clause[ [,]clause] ... ]
loosely-structured-block
!$omp end single [end-clause[ [,]end-clause] ...]

clause:
  copyprivate (list)
  nowait
  [...]

end-clause:
  copyprivate (list)
  nowait

Towards: #110008

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

4 Files Affected:

(modified) flang/lib/Semantics/check-omp-structure.cpp (+48-9)
(modified) flang/test/Semantics/OpenMP/clause-validity01.f90 (+3-3)
(added) flang/test/Semantics/OpenMP/single03.f90 (+54)
(modified) flang/test/Semantics/OpenMP/threadprivate04.f90 (+1-1)

diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 1d6fe6c8d4249..93f71c1951658 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -1203,6 +1203,40 @@ void OmpStructureChecker::Enter(const parser::OpenMPBlockConstruct &x) {
     deviceConstructFound_ = true;
   }
 
+  unsigned version{context_.langOptions().OpenMPVersion};
+  if (version >= 52 &&
+      GetContext().directive == llvm::omp::Directive::OMPD_single) {
+    bool foundCopyPrivate{false};
+    bool foundNowait{false};
+    parser::CharBlock NowaitSource{""};
+    auto catchCopyPrivateNowaitClauses = [&](const auto &dir) {
+      for (auto &clause : std::get<parser::OmpClauseList>(dir.t).v) {
+        if (clause.Id() == llvm::omp::Clause::OMPC_copyprivate) {
+          if (foundCopyPrivate) {
+            context_.Say(clause.source,
+                "At most one COPYPRIVATE clause can appear on the SINGLE directive"_err_en_US);
+          } else {
+            foundCopyPrivate = true;
+          }
+        } else if (clause.Id() == llvm::omp::Clause::OMPC_nowait) {
+          if (foundNowait) {
+            context_.Say(clause.source,
+                "At most one NOWAIT clause can appear on the SINGLE directive"_err_en_US);
+          } else {
+            foundNowait = true;
+            NowaitSource = clause.source;
+          }
+        }
+      }
+    };
+    catchCopyPrivateNowaitClauses(beginBlockDir);
+    catchCopyPrivateNowaitClauses(endBlockDir);
+    if (version == 52 && foundCopyPrivate && foundNowait) {
+      context_.Say(NowaitSource,
+          "NOWAIT clause must not be used with COPYPRIVATE clause on the SINGLE directive"_err_en_US);
+    }
+  }
+
   switch (beginDir.v) {
   case llvm::omp::Directive::OMPD_target:
     if (CheckTargetBlockOnlyTeams(block)) {
@@ -2853,7 +2887,8 @@ void OmpStructureChecker::Leave(const parser::OmpClauseList &) {
   CheckMultListItems();
 
   // 2.7.3 Single Construct Restriction
-  if (GetContext().directive == llvm::omp::Directive::OMPD_end_single) {
+  if (context_.langOptions().OpenMPVersion < 52 &&
+      GetContext().directive == llvm::omp::Directive::OMPD_end_single) {
     CheckNotAllowedIfClause(
         llvm::omp::Clause::OMPC_copyprivate, {llvm::omp::Clause::OMPC_nowait});
   }
@@ -3481,14 +3516,16 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Private &x) {
 
 void OmpStructureChecker::Enter(const parser::OmpClause::Nowait &x) {
   CheckAllowedClause(llvm::omp::Clause::OMPC_nowait);
-  if (llvm::omp::noWaitClauseNotAllowedSet.test(GetContext().directive)) {
+  if (context_.langOptions().OpenMPVersion < 52 &&
+      llvm::omp::noWaitClauseNotAllowedSet.test(GetContext().directive)) {
+    std::string dirName{
+        parser::ToUpperCaseLetters(GetContext().directiveSource.ToString())};
     context_.Say(GetContext().clauseSource,
         "%s clause is not allowed on the OMP %s directive,"
-        " use it on OMP END %s directive "_err_en_US,
+        " use it on OMP END %s directive or %s"_err_en_US,
         parser::ToUpperCaseLetters(
             getClauseName(llvm::omp::Clause::OMPC_nowait).str()),
-        parser::ToUpperCaseLetters(GetContext().directiveSource.ToString()),
-        parser::ToUpperCaseLetters(GetContext().directiveSource.ToString()));
+        dirName, dirName, TryVersion(52));
   }
 }
 
@@ -4220,14 +4257,16 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Copyprivate &x) {
   CheckIntentInPointer(symbols, llvm::omp::Clause::OMPC_copyprivate);
   CheckCopyingPolymorphicAllocatable(
       symbols, llvm::omp::Clause::OMPC_copyprivate);
-  if (GetContext().directive == llvm::omp::Directive::OMPD_single) {
+  if (context_.langOptions().OpenMPVersion < 52 &&
+      GetContext().directive == llvm::omp::Directive::OMPD_single) {
+    std::string dirName{
+        parser::ToUpperCaseLetters(GetContext().directiveSource.ToString())};
     context_.Say(GetContext().clauseSource,
         "%s clause is not allowed on the OMP %s directive,"
-        " use it on OMP END %s directive "_err_en_US,
+        " use it on OMP END %s directive or %s"_err_en_US,
         parser::ToUpperCaseLetters(
             getClauseName(llvm::omp::Clause::OMPC_copyprivate).str()),
-        parser::ToUpperCaseLetters(GetContext().directiveSource.ToString()),
-        parser::ToUpperCaseLetters(GetContext().directiveSource.ToString()));
+        dirName, dirName, TryVersion(52));
   }
 }
 
diff --git a/flang/test/Semantics/OpenMP/clause-validity01.f90 b/flang/test/Semantics/OpenMP/clause-validity01.f90
index e8114154a809b..0349a5a760753 100644
--- a/flang/test/Semantics/OpenMP/clause-validity01.f90
+++ b/flang/test/Semantics/OpenMP/clause-validity01.f90
@@ -330,7 +330,7 @@
   !$omp parallel
   b = 1
   !ERROR: LASTPRIVATE clause is not allowed on the SINGLE directive
-  !ERROR: NOWAIT clause is not allowed on the OMP SINGLE directive, use it on OMP END SINGLE directive 
+  !ERROR: NOWAIT clause is not allowed on the OMP SINGLE directive, use it on OMP END SINGLE directive or try -fopenmp-version=52
   !$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
@@ -351,7 +351,7 @@
   a = 1.0
   !ERROR: COPYPRIVATE clause is not allowed on the END WORKSHARE directive
   !$omp end workshare nowait copyprivate(a)
-  !ERROR: NOWAIT clause is not allowed on the OMP WORKSHARE directive, use it on OMP END WORKSHARE directive 
+  !ERROR: NOWAIT clause is not allowed on the OMP WORKSHARE directive, use it on OMP END WORKSHARE directive or try -fopenmp-version=52
   !$omp workshare nowait
   !$omp end workshare
   !$omp end parallel
@@ -420,7 +420,7 @@
   !$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
-  !ERROR: NOWAIT clause is not allowed on the OMP DO SIMD directive, use it on OMP END DO SIMD directive 
+  !ERROR: NOWAIT clause is not allowed on the OMP DO SIMD directive, use it on OMP END DO SIMD directive or try -fopenmp-version=52
   !$omp do simd ordered(2) NOGROUP nowait
   do i = 1, N
      do j = 1, N
diff --git a/flang/test/Semantics/OpenMP/single03.f90 b/flang/test/Semantics/OpenMP/single03.f90
new file mode 100644
index 0000000000000..bc7647b66e39f
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/single03.f90
@@ -0,0 +1,54 @@
+! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp -fopenmp-version=52
+!
+! OpenMP Version 5.2
+!
+! 2.10.2 single Construct
+! Copyprivate and Nowait clauses are allowed in both clause and end clause
+
+subroutine omp_single
+    integer, save :: i
+    integer       :: j
+    i = 10; j = 11
+
+    !ERROR: COPYPRIVATE variable 'i' is not PRIVATE or THREADPRIVATE in outer context
+    !ERROR: NOWAIT clause must not be used with COPYPRIVATE clause on the SINGLE directive
+    !$omp single copyprivate(i) nowait
+        print *, "omp single", i
+    !$omp end single
+
+    !$omp parallel private(i)
+        !$omp single copyprivate(i)
+            print *, "omp single", i
+        !$omp end single
+    !$omp end parallel
+
+    !$omp parallel
+        !ERROR: NOWAIT clause must not be used with COPYPRIVATE clause on the SINGLE directive
+        !$omp single nowait
+            print *, "omp single", i
+        !ERROR: COPYPRIVATE variable 'i' is not PRIVATE or THREADPRIVATE in outer context
+        !$omp end single copyprivate(i)
+
+        !ERROR: COPYPRIVATE variable 'i' is not PRIVATE or THREADPRIVATE in outer context
+        !$omp single copyprivate(i)
+            print *, "omp single", i
+        !ERROR: NOWAIT clause must not be used with COPYPRIVATE clause on the SINGLE directive
+        !$omp end single nowait
+
+        !ERROR: COPYPRIVATE variable 'j' may not appear on a PRIVATE or FIRSTPRIVATE clause on a SINGLE construct
+        !$omp single private(j) copyprivate(j)
+            print *, "omp single", j
+        !ERROR: At most one COPYPRIVATE clause can appear on the SINGLE directive
+        !ERROR: COPYPRIVATE variable 'j' may not appear on a PRIVATE or FIRSTPRIVATE clause on a SINGLE construct
+        !$omp end single copyprivate(j)
+
+        !$omp single nowait
+            print *, "omp single", j
+        !ERROR: At most one NOWAIT clause can appear on the SINGLE directive
+        !$omp end single nowait
+    !$omp end parallel
+
+    !$omp single nowait
+        print *, "omp single", i
+    !$omp end single
+end subroutine omp_single
diff --git a/flang/test/Semantics/OpenMP/threadprivate04.f90 b/flang/test/Semantics/OpenMP/threadprivate04.f90
index 3d8c7fb8de8fa..db23cdb06ed96 100644
--- a/flang/test/Semantics/OpenMP/threadprivate04.f90
+++ b/flang/test/Semantics/OpenMP/threadprivate04.f90
@@ -14,7 +14,7 @@ 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 
+  !ERROR: COPYPRIVATE clause is not allowed on the OMP SINGLE directive, use it on OMP END SINGLE directive or try -fopenmp-version=52
   !$omp single copyprivate(x2, /blk1/)
   !$omp end single

kiranchandramohan · 2025-02-21T17:02:57Z

Would it make sense to allow this usage irrespective of the standard?

kkwli · 2025-02-21T17:21:48Z

Would it make sense to allow this usage irrespective of the standard?

My two cents. If flang supports both forms, it'd better to disallow mixing two forms in a single construct. There is if copyprivate is specified on the single directive, there shouldn't be anything on the end single. I guess that is less confusing.

Thirumalai-Shaktivel · 2025-02-24T04:49:32Z

Would it make sense to allow this usage irrespective of the standard?

Yes, that makes sense, I will make the required changes soon.

@kkwli, I have added a check to catch the duplicate usage of the clauses, see: https://github.com/llvm/llvm-project/pull/127769/files#diff-69cb55326952c1dffb012c3c2691d8ddc1e65bf9500fd41821f1196d04216bf9R39-R48. I hope, it looks good.

Thirumalai-Shaktivel · 2025-02-24T09:03:06Z

Ready for review!

mjklemm · 2025-02-24T16:40:33Z

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

+    auto catchCopyPrivateNowaitClauses = [&](const auto &dir) {
+      for (auto &clause : std::get<parser::OmpClauseList>(dir.t).v) {
+        if (clause.Id() == llvm::omp::Clause::OMPC_copyprivate) {
+          if (foundCopyPrivate) {


This condition is incorrect and needs to be tied to the list-items in the copyprivate clause. Several copyprivate clauses can be at the directive, as long as they have different list-items.

I missed this, thanks for noticing.

mjklemm · 2025-02-24T16:42:22Z

Would it make sense to allow this usage irrespective of the standard?

My two cents. If flang supports both forms, it'd better to disallow mixing two forms in a single construct. There is if copyprivate is specified on the single directive, there shouldn't be anything on the end single. I guess that is less confusing.

I would argue that this should be warning instead of a hard error. The code would not be incorrect according to the OpenMP API and therefore should be accepted, and the warning should tell the user to be consistent.

kkwli · 2025-02-26T20:02:23Z

Would it make sense to allow this usage irrespective of the standard?

My two cents. If flang supports both forms, it'd better to disallow mixing two forms in a single construct. There is if copyprivate is specified on the single directive, there shouldn't be anything on the end single. I guess that is less confusing.

I would argue that this should be warning instead of a hard error. The code would not be incorrect according to the OpenMP API and therefore should be accepted, and the warning should tell the user to be consistent.

I fine with lowering to a warning.

Changes: - Throw error for repeated list items in the copyprivate clause - Throw warnings for repeated use of copyprivate and nowait in the end directive

github-actions · 2025-02-28T10:47:40Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Thirumalai-Shaktivel · 2025-02-28T11:41:44Z

@mjklemm, I made the required changes and added some more test cases, do you see anything missing there.
Also, I have a doubt, consider an example:

program single
    !$omp single nowait
        print *, x
    !$omp end single copyprivate(x, y) nowait
end program

Error:

$ flang x.f90 -fopenmp
flang-21: warning: OpenMP support in flang is still experimental [-Wexperimental-option]
error: Semantic errors in x.f90
./x.f90:2:18: error: NOWAIT clause must not be used with COPYPRIVATE clause on the SINGLE directive
      !$omp single nowait
                   ^^^^^^
./x.f90:4:40: warning: NOWAIT clause is already used on the SINGLE directive
      !$omp end single copyprivate(x, y) nowait
                                         ^^^^^^

Is the error and the warning correct and expected?

mjklemm · 2025-03-01T10:37:38Z

Error:

$ flang x.f90 -fopenmp
flang-21: warning: OpenMP support in flang is still experimental [-Wexperimental-option]
error: Semantic errors in x.f90
./x.f90:2:18: error: NOWAIT clause must not be used with COPYPRIVATE clause on the SINGLE directive
      !$omp single nowait
                   ^^^^^^
./x.f90:4:40: warning: NOWAIT clause is already used on the SINGLE directive
      !$omp end single copyprivate(x, y) nowait
                                         ^^^^^^

Is the error and the warning correct and expected?

I think this is a bug. I do not see any restriction in the OpenMP specification that would disallow NOWAIT when COPYPRIVATE is present.

However, the compiler should produce an error that you have NOWAIT at both the begin and end directive. Only for the COPYPRIVATE clause the diagnostic should be a warning. I consider NOWAIT at both directives an actual error, because NOWAIT is a unique clause as per [5.2:308:10].

Thirumalai-Shaktivel · 2025-03-01T13:01:24Z

Thanks for the review!

I think this is a bug. I do not see any restriction in the OpenMP specification that would disallow NOWAIT when COPYPRIVATE is present.

Maybe I think I need to fix the error message there.
Here is the restriction from OpenMP 5.2: 11.1 single Construct, which says "The copyprivate clause must not be used with the nowait clause". But this restriction is removed in OpenMP 6.0.

I actually altered the previous error message, it used to throw the following:

error: Semantic errors in <source>
<source>:7:7: error: Clause NOWAIT is not allowed if clause COPYPRIVATE appears on the END SINGLE directive
  !$omp end single copyprivate(x) nowait
        ^^^^^^^^^^

Let me know, what needs to be done here.

However, the compiler should produce an error that you have NOWAIT at both the begin and end directive. Only for the COPYPRIVATE clause the diagnostic should be a warning. I consider NOWAIT at both directives an actual error, because NOWAIT is a unique clause as per [5.2:308:10].

This seems right, I will make the required changes soon.

Thirumalai-Shaktivel · 2025-03-03T06:39:35Z

Done, now I get:

$ flang-new x.f90 -fopenmp              
flang-21: warning: OpenMP support in flang is still experimental [-Wexperimental-option]
error: Semantic errors in x.f90
./x.f90:2:18: error: NOWAIT clause must not be used with COPYPRIVATE clause on the SINGLE directive
      !$omp single nowait
                   ^^^^^^
./x.f90:4:40: error: At most one NOWAIT clause can appear on the SINGLE directive
      !$omp end single copyprivate(x, y) nowait
                                         ^^^^^^

mjklemm

I think this LGTM.

Thirumalai-Shaktivel · 2025-03-07T03:54:36Z

Thanks for the review!

…llvm#127769) Issue: - Single construct used to throw a semantic error for copyprivate and nowait clause when used in the single directive. - Also, the copyprivate with nowait restriction has been removed from OpenMP 6.0 Fix: - Allow copyprivate and nowait on both single and end single directive - Allow at most one nowait clause - Throw a warning when the same list item is used in the copyprivate clause on the end single directive From Reference guide (OpenMP 5.2, 2.10.2): ``` !$omp single [clause[ [,]clause] ... ] loosely-structured-block !$omp end single [end-clause[ [,]end-clause] ...] clause: copyprivate (list) nowait [...] end-clause: copyprivate (list) nowait ``` Towards: llvm#110008

Thirumalai-Shaktivel requested review from tblah, kiranchandramohan and kparzysz February 19, 2025 08:55

llvmbot added flang Flang issues not falling into any other category flang:openmp flang:semantics labels Feb 19, 2025

Thirumalai-Shaktivel mentioned this pull request Feb 19, 2025

[Flang][OpenMP] Important issues to be fixed prior to removing experimental status of OpenMP #110008

Open

75 tasks

Thirumalai-Shaktivel added 3 commits February 24, 2025 06:18

[Flang] Remove semantic checks for Copyprivate and nowait

2ebb099

Fix failures

656864e

Remove duplicate checks

6cbaf66

Thirumalai-Shaktivel force-pushed the llvm/single_01 branch from 6d30850 to 6cbaf66 Compare February 24, 2025 09:02

Thirumalai-Shaktivel requested a review from mjklemm February 24, 2025 10:04

mjklemm requested changes Feb 24, 2025

View reviewed changes

Thirumalai-Shaktivel added 2 commits February 28, 2025 10:47

Fix Copyprivate clause semantics check

eebd6c6

Changes: - Throw error for repeated list items in the copyprivate clause - Throw warnings for repeated use of copyprivate and nowait in the end directive

Add a test

292203e

Thirumalai-Shaktivel force-pushed the llvm/single_01 branch from 2638d3b to 292203e Compare February 28, 2025 11:36

Fix clang format

5168d4e

Fix: Allow nowait only once

87b7aac

llvmbot added the clang:openmp OpenMP related changes to Clang label Mar 3, 2025

mjklemm approved these changes Mar 6, 2025

View reviewed changes

Thirumalai-Shaktivel merged commit e15545c into llvm:main Mar 7, 2025
12 checks passed

Thirumalai-Shaktivel deleted the llvm/single_01 branch March 7, 2025 03:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flang][OpenMP] Allow copyprivate and nowait on the directive clauses #127769

[Flang][OpenMP] Allow copyprivate and nowait on the directive clauses #127769

Thirumalai-Shaktivel commented Feb 19, 2025

llvmbot commented Feb 19, 2025 •

edited

Loading

kiranchandramohan commented Feb 21, 2025

kkwli commented Feb 21, 2025

Thirumalai-Shaktivel commented Feb 24, 2025

Thirumalai-Shaktivel commented Feb 24, 2025

mjklemm Feb 24, 2025

Thirumalai-Shaktivel Feb 27, 2025

Thirumalai-Shaktivel Feb 28, 2025

mjklemm commented Feb 24, 2025

kkwli commented Feb 26, 2025

github-actions bot commented Feb 28, 2025 •

edited

Loading

Thirumalai-Shaktivel commented Feb 28, 2025 •

edited

Loading

mjklemm commented Mar 1, 2025

Thirumalai-Shaktivel commented Mar 1, 2025

Thirumalai-Shaktivel commented Mar 3, 2025

mjklemm left a comment

Thirumalai-Shaktivel commented Mar 7, 2025

[Flang][OpenMP] Allow copyprivate and nowait on the directive clauses #127769

[Flang][OpenMP] Allow copyprivate and nowait on the directive clauses #127769

Conversation

Thirumalai-Shaktivel commented Feb 19, 2025

llvmbot commented Feb 19, 2025 • edited Loading

kiranchandramohan commented Feb 21, 2025

kkwli commented Feb 21, 2025

Thirumalai-Shaktivel commented Feb 24, 2025

Thirumalai-Shaktivel commented Feb 24, 2025

mjklemm Feb 24, 2025

Choose a reason for hiding this comment

Thirumalai-Shaktivel Feb 27, 2025

Choose a reason for hiding this comment

Thirumalai-Shaktivel Feb 28, 2025

Choose a reason for hiding this comment

mjklemm commented Feb 24, 2025

kkwli commented Feb 26, 2025

github-actions bot commented Feb 28, 2025 • edited Loading

Thirumalai-Shaktivel commented Feb 28, 2025 • edited Loading

mjklemm commented Mar 1, 2025

Thirumalai-Shaktivel commented Mar 1, 2025

Thirumalai-Shaktivel commented Mar 3, 2025

mjklemm left a comment

Choose a reason for hiding this comment

Thirumalai-Shaktivel commented Mar 7, 2025

llvmbot commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 28, 2025 •

edited

Loading

Thirumalai-Shaktivel commented Feb 28, 2025 •

edited

Loading