[clang][OpenMP] Fix/enforce order-concurrent-nestable rules #135463

ErnestoSu · 2025-04-12T01:47:42Z

OpenMP has restrictions on directives allowed to be strictly nested inside a
construct with the order(concurrent) clause specified.

OpenMP 5.0, 5.1, and 5.2 allows: 'loop', 'parallel', 'simd', and
combined directives starting with 'parallel'.
OpenMP 6.0 allows: the above directives plus 'atomic' and
all loop-transformation directives.

Furthermore, a region that corresponds to a construct with order(concurrent)
specified may not contain calls to the OpenMP runtime API.

This PR fixes the following issues in the current implementation:

With -fopenmp-version=50: none of the nesting restrictions above were enforced
With -fopenmp-version=60:

Clang did not reject OpenMP runtime APIs encountered in the region.
Clang erroneously rejected combined directives starting with parallel.

github-actions · 2025-04-12T01:48:00Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-04-12T01:48:29Z

@llvm/pr-subscribers-clang

Author: Ernesto Su (ErnestoSu)

Changes

OpenMP has restrictions on directives allowed to be strictly nested inside a
construct with the order(concurrent) clause specified.

OpenMP 5.0, 5.1, and 5.2 allows: 'loop', 'parallel', 'simd', and
combined directives starting with 'parallel'.
OpenMP 6.0 allows: the above directives plus 'atomic' and
all loop-transformation directives.

Furthermore, a region that corresponds to a construct with order(concurrent)
specified may not contain calls to the OpenMP runtime API.

This PR fixes the following issues in the current implementation:

With -fopenmp-version=50: none of the nesting restrictions above were enforced
With -fopenmp-version=60:

Clang did not reject OpenMP runtime APIs encountered in the region.
Clang erroneously rejected combined directives starting with parallel.

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

4 Files Affected:

(modified) clang/include/clang/Basic/OpenMPKinds.h (+3-1)
(modified) clang/lib/Basic/OpenMPKinds.cpp (+14-3)
(modified) clang/lib/Sema/SemaOpenMP.cpp (+21-36)
(modified) clang/test/OpenMP/for_order_messages.cpp (+23-11)

diff --git a/clang/include/clang/Basic/OpenMPKinds.h b/clang/include/clang/Basic/OpenMPKinds.h
index 6ca9f9c550285..1fa27660b1f57 100644
--- a/clang/include/clang/Basic/OpenMPKinds.h
+++ b/clang/include/clang/Basic/OpenMPKinds.h
@@ -411,9 +411,11 @@ bool isOpenMPCapturingDirective(OpenMPDirectiveKind DKind);
 /// directive that can be nested within region corresponding to construct
 /// on which order clause was specified with concurrent as ordering argument.
 /// \param DKind Specified directive.
+/// \param LangOpts Used for getting the OpenMP version
 /// \return true - if the above condition is met for this directive
 /// otherwise - false.
-bool isOpenMPOrderConcurrentNestableDirective(OpenMPDirectiveKind DKind);
+bool isOpenMPOrderConcurrentNestableDirective(OpenMPDirectiveKind DKind,
+                                              const LangOptions &LangOpts);
 }
 
 template <>
diff --git a/clang/lib/Basic/OpenMPKinds.cpp b/clang/lib/Basic/OpenMPKinds.cpp
index 09921e3b1edfc..7b90861c78de0 100644
--- a/clang/lib/Basic/OpenMPKinds.cpp
+++ b/clang/lib/Basic/OpenMPKinds.cpp
@@ -768,9 +768,20 @@ bool clang::isOpenMPCapturingDirective(OpenMPDirectiveKind DKind) {
 }
 
 bool clang::isOpenMPOrderConcurrentNestableDirective(
-    OpenMPDirectiveKind DKind) {
-  return DKind == OMPD_atomic || DKind == OMPD_loop || DKind == OMPD_simd ||
-         DKind == OMPD_parallel || isOpenMPLoopTransformationDirective(DKind);
+    OpenMPDirectiveKind DKind, const LangOptions &LangOpts) {
+  // Directives strictly nestable in a construct with order(concurrent) are:
+  // OpenMP 5.x: loop, parallel, simd, combined directive starting with parallel
+  // OpenMP 6.0: above plus atomic and all loop-transformation directives
+
+  if (DKind == OMPD_loop || DKind == OMPD_parallel || DKind == OMPD_simd ||
+      isOpenMPCombinedParallelADirective(DKind))
+    return true;
+
+  if (LangOpts.OpenMP >= 60)
+    return DKind == OMPD_atomic ||
+           isOpenMPLoopTransformationDirective(DKind);
+
+  return false;
 }
 
 void clang::getOpenMPCaptureRegions(
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index a382947455aef..3fcdd1acb9849 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -4789,26 +4789,12 @@ static bool checkNestingOfRegions(Sema &SemaRef, const DSAStackTy *Stack,
       getLeafOrCompositeConstructs(ParentRegion, LeafOrComposite);
   OpenMPDirectiveKind EnclosingConstruct = ParentLOC.back();
 
-  if (Stack->isParentOrderConcurrent()) {
-    bool InvalidOrderNesting = false;
-    if ((SemaRef.LangOpts.OpenMP == 51 || SemaRef.LangOpts.OpenMP == 52) &&
-        CurrentRegion != OMPD_simd && CurrentRegion != OMPD_loop &&
-        CurrentRegion != OMPD_parallel &&
-        !isOpenMPCombinedParallelADirective(CurrentRegion)) {
-      InvalidOrderNesting = true;
-    } else if (SemaRef.LangOpts.OpenMP >= 60 &&
-               !isOpenMPOrderConcurrentNestableDirective(CurrentRegion)) {
-      // OpenMP 6.0 [12.3 order Clause, Restrictions]
-      // Only regions that correspond to order-concurrent-nestable constructs
-      // or order-concurrent-nestable routines may be strictly nested regions
-      // of regions that correspond to constructs on which the order clause is
-      // specified with concurrent as the ordering argument.
-      InvalidOrderNesting = true;
-    }
-    if (InvalidOrderNesting) {
-      SemaRef.Diag(StartLoc, diag::err_omp_prohibited_region_order)
-          << getOpenMPDirectiveName(CurrentRegion);
-    }
+  if (SemaRef.LangOpts.OpenMP >= 50 && Stack->isParentOrderConcurrent() &&
+      !isOpenMPOrderConcurrentNestableDirective(CurrentRegion,
+                                                SemaRef.LangOpts)) {
+    SemaRef.Diag(StartLoc, diag::err_omp_prohibited_region_order)
+        << getOpenMPDirectiveName(CurrentRegion);
+    return true;
   }
   if (isOpenMPSimdDirective(ParentRegion) &&
       ((SemaRef.LangOpts.OpenMP <= 45 && CurrentRegion != OMPD_ordered) ||
@@ -7132,7 +7118,7 @@ ExprResult SemaOpenMP::ActOnOpenMPCall(ExprResult Call, Scope *Scope,
   if (!CalleeFnDecl)
     return Call;
 
-  if (getLangOpts().OpenMP >= 51 && getLangOpts().OpenMP < 60 &&
+  if (getLangOpts().OpenMP >= 50 && getLangOpts().OpenMP <= 60 &&
       CalleeFnDecl->getIdentifier() &&
       CalleeFnDecl->getName().starts_with_insensitive("omp_")) {
     // checking for any calls inside an Order region
@@ -16373,21 +16359,20 @@ OMPClause *SemaOpenMP::ActOnOpenMPOrderClause(
         << getOpenMPClauseName(OMPC_order);
     return nullptr;
   }
-  if (getLangOpts().OpenMP >= 51) {
-    if (Modifier == OMPC_ORDER_MODIFIER_unknown && MLoc.isValid()) {
-      Diag(MLoc, diag::err_omp_unexpected_clause_value)
-          << getListOfPossibleValues(OMPC_order,
-                                     /*First=*/OMPC_ORDER_MODIFIER_unknown + 1,
-                                     /*Last=*/OMPC_ORDER_MODIFIER_last)
-          << getOpenMPClauseName(OMPC_order);
-    } else {
-      DSAStack->setRegionHasOrderConcurrent(/*HasOrderConcurrent=*/true);
-      if (DSAStack->getCurScope()) {
-        // mark the current scope with 'order' flag
-        unsigned existingFlags = DSAStack->getCurScope()->getFlags();
-        DSAStack->getCurScope()->setFlags(existingFlags |
-                                          Scope::OpenMPOrderClauseScope);
-      }
+  if (getLangOpts().OpenMP >= 51 && Modifier == OMPC_ORDER_MODIFIER_unknown &&
+      MLoc.isValid()) {
+    Diag(MLoc, diag::err_omp_unexpected_clause_value)
+        << getListOfPossibleValues(OMPC_order,
+                                   /*First=*/OMPC_ORDER_MODIFIER_unknown + 1,
+                                   /*Last=*/OMPC_ORDER_MODIFIER_last)
+        << getOpenMPClauseName(OMPC_order);
+  } else if (getLangOpts().OpenMP >= 50) {
+    DSAStack->setRegionHasOrderConcurrent(/*HasOrderConcurrent=*/true);
+    if (DSAStack->getCurScope()) {
+      // mark the current scope with 'order' flag
+      unsigned existingFlags = DSAStack->getCurScope()->getFlags();
+      DSAStack->getCurScope()->setFlags(existingFlags |
+                                        Scope::OpenMPOrderClauseScope);
     }
   }
   return new (getASTContext()) OMPOrderClause(
diff --git a/clang/test/OpenMP/for_order_messages.cpp b/clang/test/OpenMP/for_order_messages.cpp
index 530c051849201..f41d3f7f635ce 100644
--- a/clang/test/OpenMP/for_order_messages.cpp
+++ b/clang/test/OpenMP/for_order_messages.cpp
@@ -8,35 +8,47 @@
 // RUN: %clang_cc1 -fsyntax-only -fopenmp-simd -fopenmp-version=52 -triple x86_64-unknown-unknown -verify=expected,omp51 %s -Wuninitialized
 // RUN: %clang_cc1 -fsyntax-only -fopenmp-simd -fopenmp-version=60 -triple x86_64-unknown-unknown -verify=expected,omp60 %s -Wuninitialized
 
+// Constructs strictly nestable in a construct with order(concurrent) specified vary by OpenMP version:
+// OMP5.0,5.1,5.2: loop, parallel, simd, and combined constructs with parallel as the first component.
+// OMP6.0: in addition to the ones allowed in OMP5.x, also atomic and all loop-transformation constructs.
+
 extern int omp_get_num_threads  (void);
 
 int main(int argc, char **argv) {
   int A = 0;
 #pragma omp parallel for order(concurrent)
   for (int i = 0; i < 10; ++i)
-    omp_get_num_threads(); // omp51-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}}
+    omp_get_num_threads(); // omp50-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}} omp51-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}} omp60-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}}
 
 #pragma omp parallel for order(reproducible:concurrent) // omp50-error {{expected 'concurrent' in OpenMP clause 'order'}}
   for (int i = 0; i < 10; ++i)
-    omp_get_num_threads(); // omp51-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}}
+    omp_get_num_threads(); // omp51-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}} omp60-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}}
 
 #pragma omp parallel for order(unconstrained:concurrent) // omp50-error {{expected 'concurrent' in OpenMP clause 'order'}}
   for (int i = 0; i < 10; ++i)
-    omp_get_num_threads(); // omp51-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}}
+    omp_get_num_threads(); // omp51-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}} omp60-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}}
 
 #pragma omp parallel for order(concurrent)
   for (int i = 0; i < 10; ++i) {
     for (int j = 0; j < 10; ++j) {
-      omp_get_num_threads(); // omp51-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}}
+      omp_get_num_threads(); // omp50-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}} omp51-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}} omp60-error {{calls to OpenMP runtime API are not allowed within a region that corresponds to a construct with an order clause that specifies concurrent}}
     }
   }
 
+// nested atomic: OK in OMP6.0 but not in OMP5.x
 #pragma omp parallel for order(concurrent)
   for (int i = 0; i < 10; ++i) {
-#pragma omp atomic //omp51-error {{construct 'atomic' not allowed in a region associated with a directive with 'order' clause}}
+#pragma omp atomic //omp50-error {{construct 'atomic' not allowed in a region associated with a directive with 'order' clause}} omp51-error {{construct 'atomic' not allowed in a region associated with a directive with 'order' clause}}
       A++;
   }
 
+// nested loop-transformation construct: OK in OMP6.0 but not in OMP5.x
+#pragma omp parallel for order(concurrent)
+  for (int i = 0; i < 10; ++i) {
+#pragma omp unroll //omp50-error {{construct 'unroll' not allowed in a region associated with a directive with 'order' clause}} omp51-error {{construct 'unroll' not allowed in a region associated with a directive with 'order' clause}}
+    for (int j = 0; j < 10; ++j);
+  }
+
 #pragma omp parallel for order(reproducible: concurrent) // omp50-error {{expected 'concurrent' in OpenMP clause 'order'}}
   for (int i = 0; i < 10; ++i) {
 #pragma omp target //omp51-error {{construct 'target' not allowed in a region associated with a directive with 'order' clause}} omp60-error {{construct 'target' not allowed in a region associated with a directive with 'order' clause}}
@@ -51,7 +63,7 @@ int main(int argc, char **argv) {
 
 #pragma omp loop bind(parallel) order(concurrent)
   for (int i = 0; i < 10; ++i) {
-#pragma omp parallel for //omp60-error {{construct 'parallel for' not allowed in a region associated with a directive with 'order' clause}}
+#pragma omp parallel for
     for (int j = 0; j < 10; ++j) {
       A += j;
     }
@@ -59,7 +71,7 @@ int main(int argc, char **argv) {
 
 #pragma omp distribute order(concurrent)
   for (int i = 0; i < 10; ++i) {
-#pragma omp parallel for simd //omp60-error {{construct 'parallel for simd' not allowed in a region associated with a directive with 'order' clause}}
+#pragma omp parallel for simd
     for (int j = 0; j < 10; ++j) {
       A += j;
     }
@@ -67,7 +79,7 @@ int main(int argc, char **argv) {
 
 #pragma omp for order(concurrent)
   for (int i = 0; i < 10; ++i) {
-#pragma omp parallel master //omp60-error {{construct 'parallel master' not allowed in a region associated with a directive with 'order' clause}}
+#pragma omp parallel master
     for (int j = 0; j < 10; ++j) {
       A += j;
     }
@@ -75,7 +87,7 @@ int main(int argc, char **argv) {
 
 #pragma omp for order(concurrent)
   for (int i = 0; i < 10; ++i) {
-#pragma omp parallel master taskloop //omp60-error {{construct 'parallel master taskloop' not allowed in a region associated with a directive with 'order' clause}}
+#pragma omp parallel master taskloop
     for (int j = 0; j < 10; ++j) {
       A += j;
     }
@@ -83,7 +95,7 @@ int main(int argc, char **argv) {
 
 #pragma omp for order(concurrent)
   for (int i = 0; i < 10; ++i) {
-#pragma omp parallel master taskloop simd //omp60-error {{construct 'parallel master taskloop simd' not allowed in a region associated with a directive with 'order' clause}}
+#pragma omp parallel master taskloop simd
     for (int j = 0; j < 10; ++j) {
       A += j;
     }
@@ -91,7 +103,7 @@ int main(int argc, char **argv) {
 
 #pragma omp for order(concurrent)
   for (int i = 0; i < 10; ++i) {
-    #pragma omp parallel sections //omp60-error {{construct 'parallel sections' not allowed in a region associated with a directive with 'order' clause}}
+    #pragma omp parallel sections
     {
       A++;
     }

ErnestoSu · 2025-04-15T16:19:24Z

clang/lib/Sema/SemaOpenMP.cpp

@@ -16373,21 +16359,20 @@ OMPClause *SemaOpenMP::ActOnOpenMPOrderClause(
        << getOpenMPClauseName(OMPC_order);
    return nullptr;
  }
-  if (getLangOpts().OpenMP >= 51) {


Original implementation did not call setRegionHasOrderConcurrent(true) for OpenMP 5.0, causing Clang to ignore all order-concurrent nestable rules for -fopenmp-version=50.

ErnestoSu · 2025-04-15T16:24:24Z

@zahiraam @alexey-bataev Please review

ErnestoSu · 2025-04-15T16:30:31Z

clang/lib/Sema/SemaOpenMP.cpp

@@ -7132,7 +7118,7 @@ ExprResult SemaOpenMP::ActOnOpenMPCall(ExprResult Call, Scope *Scope,
  if (!CalleeFnDecl)
    return Call;

-  if (getLangOpts().OpenMP >= 51 && getLangOpts().OpenMP < 60 &&
+  if (getLangOpts().OpenMP >= 50 && getLangOpts().OpenMP <= 60 &&


OpenMP 5.0 and 6.0 also have the same restrictions as 5.1 and 5.2 w.r.t. OpenMP runtime APIs (e.g., omp_get_thread_num()) inside a construct specified with the order(concurrent) clause.

zahiraam

A few nit things.
Looks good. Thanks.

clang/include/clang/Basic/OpenMPKinds.h

ErnestoSu · 2025-04-17T19:55:10Z

@alexey-bataev @zahiraam Thanks so much for your review and approval! Is there anything else I need to do before this PR can be merged by one of you?

zahiraam · 2025-04-17T20:36:39Z

if @alexey-bataev doesn't have additional comments, I or he can merge it in for you.

github-actions · 2025-04-18T17:58:11Z

@ErnestoSu Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

) OpenMP has restrictions on directives allowed to be strictly nested inside a construct with the order(concurrent) clause specified. - OpenMP 5.0, 5.1, and 5.2 allows: 'loop', 'parallel', 'simd', and combined directives starting with 'parallel'. - OpenMP 6.0 allows: the above directives plus 'atomic' and all loop-transformation directives. Furthermore, a region that corresponds to a construct with order(concurrent) specified may not contain calls to the OpenMP runtime API. This PR fixes the following issues in the current implementation: With -fopenmp-version=50: none of the nesting restrictions above were enforced With -fopenmp-version=60: 1. Clang did not reject OpenMP runtime APIs encountered in the region. 2. Clang erroneously rejected combined directives starting with parallel. --------- Co-authored-by: Zahira Ammarguellat <[email protected]>

[clang][OpenMP] Fix/enforce order-concurrent-nestable rules

51a9237

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:openmp OpenMP related changes to Clang labels Apr 12, 2025

ErnestoSu commented Apr 15, 2025

View reviewed changes

alexey-bataev approved these changes Apr 15, 2025

View reviewed changes

zahiraam approved these changes Apr 15, 2025

View reviewed changes

clang/include/clang/Basic/OpenMPKinds.h Outdated Show resolved Hide resolved

[NFC] Fixed a comment

0d29a01

Merge branch 'main' into order_concurrent_nestable

a93133d

zahiraam merged commit 89f930a into llvm:main Apr 18, 2025
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][OpenMP] Fix/enforce order-concurrent-nestable rules #135463

[clang][OpenMP] Fix/enforce order-concurrent-nestable rules #135463

Uh oh!

ErnestoSu commented Apr 12, 2025

Uh oh!

github-actions bot commented Apr 12, 2025

Uh oh!

llvmbot commented Apr 12, 2025

Uh oh!

ErnestoSu Apr 15, 2025

Uh oh!

ErnestoSu commented Apr 15, 2025

Uh oh!

ErnestoSu Apr 15, 2025

Uh oh!

zahiraam left a comment

Uh oh!

Uh oh!

ErnestoSu commented Apr 17, 2025

Uh oh!

zahiraam commented Apr 17, 2025

Uh oh!

Uh oh!

github-actions bot commented Apr 18, 2025

Uh oh!

Uh oh!

[clang][OpenMP] Fix/enforce order-concurrent-nestable rules #135463

[clang][OpenMP] Fix/enforce order-concurrent-nestable rules #135463

Uh oh!

Conversation

ErnestoSu commented Apr 12, 2025

Uh oh!

github-actions bot commented Apr 12, 2025

Uh oh!

llvmbot commented Apr 12, 2025

Uh oh!

ErnestoSu Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

ErnestoSu commented Apr 15, 2025

Uh oh!

ErnestoSu Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

zahiraam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ErnestoSu commented Apr 17, 2025

Uh oh!

zahiraam commented Apr 17, 2025

Uh oh!

Uh oh!

github-actions bot commented Apr 18, 2025

Uh oh!

Uh oh!