Skip to content

[clang][OpenMP] Fix teams nesting of region check #94806

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
Jun 24, 2024

Conversation

mikerice1969
Copy link
Contributor

The static verifier flagged dead code in the check since the loop will only execute once and never reach the iterator increment.

The loop needs to iterate twice to correctly diagnose when a statement is after the teams.

Since there are two iterations again, reset the iterator to the first teams directive when the double teams case is seen so the diagnostic can report both locations.

The static verifier flagged dead code in the check since the loop will
only execute once and never reach the iterator increment.

The loop needs to iterate twice to correctly diagnose when a statement
is after the teams.

Since there are two iterations again, reset the iterator to the first
teams directive when the double teams case is seen so the diagnostic
can report both locations.
@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 Jun 7, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 7, 2024

@llvm/pr-subscribers-clang

Author: Mike Rice (mikerice1969)

Changes

The static verifier flagged dead code in the check since the loop will only execute once and never reach the iterator increment.

The loop needs to iterate twice to correctly diagnose when a statement is after the teams.

Since there are two iterations again, reset the iterator to the first teams directive when the double teams case is seen so the diagnostic can report both locations.


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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaOpenMP.cpp (+7-3)
  • (modified) clang/test/OpenMP/Inputs/nesting_of_regions.cpp (+12)
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index 6e6815328e913..0fe04094c5912 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -13434,10 +13434,14 @@ StmtResult SemaOpenMP::ActOnOpenMPTargetDirective(ArrayRef<OMPClause *> Clauses,
       auto I = CS->body_begin();
       while (I != CS->body_end()) {
         const auto *OED = dyn_cast<OMPExecutableDirective>(*I);
-        if (!OED || !isOpenMPTeamsDirective(OED->getDirectiveKind()) ||
-            OMPTeamsFound) {
-
+        bool IsTeams = OED && isOpenMPTeamsDirective(OED->getDirectiveKind());
+        if (!IsTeams || I != CS->body_begin()) {
           OMPTeamsFound = false;
+          if (IsTeams && I != CS->body_begin()) {
+            // This is the two teams case. Since the InnerTeamsRegionLoc will
+            // point to this second one reset the iterator to the other teams.
+            --I;
+          }
           break;
         }
         ++I;
diff --git a/clang/test/OpenMP/Inputs/nesting_of_regions.cpp b/clang/test/OpenMP/Inputs/nesting_of_regions.cpp
index e671f9b0cf412..969ddfcce4cb0 100644
--- a/clang/test/OpenMP/Inputs/nesting_of_regions.cpp
+++ b/clang/test/OpenMP/Inputs/nesting_of_regions.cpp
@@ -4880,6 +4880,12 @@ void foo() {
 #pragma omp teams  // expected-note {{nested teams construct here}}
     ++a;
   }
+#pragma omp target // expected-error {{target construct with nested teams region contains statements outside of the teams construct}}
+  {
+#pragma omp teams  // expected-note {{nested teams construct here}}
+    ++a;
+    ++a;           // expected-note {{statement outside teams construct here}}
+  }
 #pragma omp target // expected-error {{target construct with nested teams region contains statements outside of the teams construct}}
   {
     while (0)      // expected-note {{statement outside teams construct here}}
@@ -14133,6 +14139,12 @@ void foo() {
 #pragma omp teams // expected-note {{nested teams construct here}}
     ++a;
   }
+#pragma omp target // expected-error {{target construct with nested teams region contains statements outside of the teams construct}}
+  {
+#pragma omp teams // expected-note {{nested teams construct here}}
+    ++a;
+    ++a;          // expected-note {{statement outside teams construct here}}
+  }
 #pragma omp target
   {
 #pragma omp taskloop

Copy link
Contributor

@jyu2-git jyu2-git left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing!

@mikerice1969 mikerice1969 merged commit b097018 into llvm:main Jun 24, 2024
11 checks passed
@mikerice1969 mikerice1969 deleted the fix-dead-teams-check branch June 24, 2024 20:37
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
The static verifier flagged dead code in the check since the loop will
only execute once and never reach the iterator increment.

The loop needs to iterate twice to correctly diagnose when a statement
is after the teams.

Since there are two iterations again, reset the iterator to the first
teams directive when the double teams case is seen so the diagnostic can
report both locations.
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