Skip to content

[clang][OpenMP] Fix region nesting check for scan directive #98386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

kparzysz
Copy link
Contributor

The previous check was inconsistent. For example, it would allow

#pragma omp target
#pragma omp parallel for
  for (...) {
#pragma omp scan
  }

but not

#pragma omp target parallel for
  for (...) {
#pragma omp scan
  }

Make the check conform to the wording on the specification.

The previous check was inconsistent. For example, it would allow
```
#pragma omp target
#pragma omp parallel for
  for (...) {
#pragma omp scan
  }
```
but not
```
#pragma omp target parallel for
  for (...) {
#pragma omp scan
  }
```

Make the check conform to the wording on the specification.
@llvmbot 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 Jul 10, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 10, 2024

@llvm/pr-subscribers-clang

Author: Krzysztof Parzyszek (kparzysz)

Changes

The previous check was inconsistent. For example, it would allow

#pragma omp target
#pragma omp parallel for
  for (...) {
#pragma omp scan
  }

but not

#pragma omp target parallel for
  for (...) {
#pragma omp scan
  }

Make the check conform to the wording on the specification.


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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaOpenMP.cpp (+13-8)
  • (modified) clang/test/OpenMP/Inputs/nesting_of_regions.cpp (+2-12)
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index bc5c172f1390e..ef09e53077f47 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -4989,14 +4989,19 @@ static bool checkNestingOfRegions(Sema &SemaRef, const DSAStackTy *Stack,
     OrphanSeen = ParentRegion == OMPD_unknown;
     Recommend = ShouldBeInTargetRegion;
   } else if (CurrentRegion == OMPD_scan) {
-    // OpenMP [2.16, Nesting of Regions]
-    // If specified, a teams construct must be contained within a target
-    // construct.
-    NestingProhibited =
-        SemaRef.LangOpts.OpenMP < 50 ||
-        (ParentRegion != OMPD_simd && ParentRegion != OMPD_for &&
-         ParentRegion != OMPD_for_simd && ParentRegion != OMPD_parallel_for &&
-         ParentRegion != OMPD_parallel_for_simd);
+    if (SemaRef.LangOpts.OpenMP >= 50) {
+      SmallVector<OpenMPDirectiveKind, 4> LeafOrComposite;
+      std::ignore = getLeafOrCompositeConstructs(ParentRegion, LeafOrComposite);
+      // OpenMP spec 5.0 and 5.1 require scan to be directly enclosed by for,
+      // simd, or for simd. This has to take into account combined directives.
+      // In 5.2 this seems to be implied by the fact that the specified
+      // separated constructs are do, for, and simd.
+      OpenMPDirectiveKind Enclosing = LeafOrComposite.back();
+      NestingProhibited = Enclosing != OMPD_for && Enclosing != OMPD_simd &&
+                          Enclosing != OMPD_for_simd;
+    } else {
+      NestingProhibited = true;
+    }
     OrphanSeen = ParentRegion == OMPD_unknown;
     Recommend = ShouldBeInLoopSimdRegion;
   }
diff --git a/clang/test/OpenMP/Inputs/nesting_of_regions.cpp b/clang/test/OpenMP/Inputs/nesting_of_regions.cpp
index 969ddfcce4cb0..985cdc0e19adc 100644
--- a/clang/test/OpenMP/Inputs/nesting_of_regions.cpp
+++ b/clang/test/OpenMP/Inputs/nesting_of_regions.cpp
@@ -5346,11 +5346,6 @@ void foo() {
   }
 #pragma omp target parallel for
   for (int i = 0; i < 10; ++i) {
-#pragma omp scan // expected-error {{region cannot be closely nested inside 'target parallel for' region}}
-    bar();
-  }
-#pragma omp target parallel for
-  for (int i = 0; i < 10; ++i) {
 #pragma omp taskwait
     bar();
   }
@@ -7146,7 +7141,7 @@ void foo() {
   }
 #pragma omp target simd
   for (int i = 0; i < 10; ++i) {
-#pragma omp scan // omp45-error {{OpenMP constructs may not be nested inside a simd region}} omp50-error {{region cannot be closely nested inside 'target simd' region; perhaps you forget to enclose 'omp scan' directive into a for, simd, for simd, parallel for, or parallel for simd region?}} omp51-error {{region cannot be closely nested inside 'target simd' region; perhaps you forget to enclose 'omp scan' directive into a for, simd, for simd, parallel for, or parallel for simd region?}}
+#pragma omp scan // omp45-error {{OpenMP constructs may not be nested inside a simd region}} omp50-error {{exactly one of 'inclusive' or 'exclusive' clauses is expected}} omp51-error {{exactly one of 'inclusive' or 'exclusive' clauses is expected}}
     bar();
   }
 #pragma omp target simd
@@ -14583,11 +14578,6 @@ void foo() {
   }
 #pragma omp target parallel for
   for (int i = 0; i < 10; ++i) {
-#pragma omp scan // expected-error {{region cannot be closely nested inside 'target parallel for' region}}
-    bar();
-  }
-#pragma omp target parallel for
-  for (int i = 0; i < 10; ++i) {
 #pragma omp taskwait
     bar();
   }
@@ -16685,7 +16675,7 @@ void foo() {
   }
 #pragma omp target simd
   for (int i = 0; i < 10; ++i) {
-#pragma omp scan // omp45-error {{OpenMP constructs may not be nested inside a simd region}} omp50-error {{region cannot be closely nested inside 'target simd' region; perhaps you forget to enclose 'omp scan' directive into a for, simd, for simd, parallel for, or parallel for simd region?}} omp51-error {{region cannot be closely nested inside 'target simd' region; perhaps you forget to enclose 'omp scan' directive into a for, simd, for simd, parallel for, or parallel for simd region?}}
+#pragma omp scan // omp45-error {{OpenMP constructs may not be nested inside a simd region}} omp50-error {{exactly one of 'inclusive' or 'exclusive' clauses is expected}} omp51-error {{exactly one of 'inclusive' or 'exclusive' clauses is expected}}
     bar();
   }
 #pragma omp target simd

Copy link
Member

@alexey-bataev alexey-bataev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

@kparzysz kparzysz merged commit 81cdf94 into main Jul 11, 2024
11 checks passed
@kparzysz kparzysz deleted the users/kparzysz/spr/c06-scan-nesting branch July 11, 2024 13:08
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
…8386)

The previous check was inconsistent. For example, it would allow
```
#pragma omp target
#pragma omp parallel for
  for (...) {
#pragma omp scan
  }
```
but not
```
#pragma omp target parallel for
  for (...) {
#pragma omp scan
  }
```

Make the check conform to the wording on the specification.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants