Skip to content
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

[Flang][OpenMP] Update semantics checks for 'teams' nesting #126922

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

skatrak
Copy link
Member

@skatrak skatrak commented Feb 12, 2025

This patch introduces a directive set for combined constructs where teams is the last leaf. This is used in a couple places to simplify checks, which is NFC, but it also replaces two incorrect uses of topTeamsSet.

Before, these checks would incorrectly skip combined constructs where teams was the last leaf construct when checking for allowed nested constructs inside of a teams region. Similarly, it would also incorrectly perform these checks whenever a compound teams construct where teams was the first leaf construct was found.

This patch introduces a directive set for combined constructs where `teams` is
the last leaf. This is used in a couple places to simplify checks, which is
NFC, but it also replaces two incorrect uses of `topTeamsSet`.

Before, these checks would incorrectly skip combined constructs where `teams`
was the last leaf construct when checking for allowed nested constructs inside
of a `teams` region. Similarly, it would also incorrectly perform these checks
whenever a compound `teams` construct where `teams` was the first leaf
construct was found.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:openmp flang:semantics labels Feb 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 12, 2025

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Sergio Afonso (skatrak)

Changes

This patch introduces a directive set for combined constructs where teams is the last leaf. This is used in a couple places to simplify checks, which is NFC, but it also replaces two incorrect uses of topTeamsSet.

Before, these checks would incorrectly skip combined constructs where teams was the last leaf construct when checking for allowed nested constructs inside of a teams region. Similarly, it would also incorrectly perform these checks whenever a compound teams construct where teams was the first leaf construct was found.


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

4 Files Affected:

  • (modified) flang/include/flang/Semantics/openmp-directive-sets.h (+7)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+4-6)
  • (modified) flang/test/Semantics/OpenMP/nested-target.f90 (+2)
  • (modified) flang/test/Semantics/OpenMP/nested-teams.f90 (+1)
diff --git a/flang/include/flang/Semantics/openmp-directive-sets.h b/flang/include/flang/Semantics/openmp-directive-sets.h
index 7cdca1214e749..dd610c9702c28 100644
--- a/flang/include/flang/Semantics/openmp-directive-sets.h
+++ b/flang/include/flang/Semantics/openmp-directive-sets.h
@@ -21,6 +21,8 @@ namespace llvm::omp {
 //===----------------------------------------------------------------------===//
 // - top<Directive>Set: The directive appears alone or as the first in a
 //   compound construct.
+// - bottom<Directive>Set: The directive appears alone or as the last in a
+//   compound construct.
 // - all<Directive>Set: All standalone or compound uses of the directive.
 
 static const OmpDirectiveSet topDistributeSet{
@@ -172,6 +174,11 @@ static const OmpDirectiveSet topTeamsSet{
     Directive::OMPD_teams_loop,
 };
 
+static const OmpDirectiveSet bottomTeamsSet{
+    Directive::OMPD_target_teams,
+    Directive::OMPD_teams,
+};
+
 static const OmpDirectiveSet allTeamsSet{
     OmpDirectiveSet{
         Directive::OMPD_target_teams,
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index fd2893998205c..1d6fe6c8d4249 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -487,8 +487,7 @@ void OmpStructureChecker::HasInvalidDistributeNesting(
       violation = true;
     } else {
       // `distribute` region has to be strictly nested inside `teams`
-      if (!OmpDirectiveSet{llvm::omp::OMPD_teams, llvm::omp::OMPD_target_teams}
-              .test(GetContextParent().directive)) {
+      if (!llvm::omp::bottomTeamsSet.test(GetContextParent().directive)) {
         violation = true;
       }
     }
@@ -518,8 +517,7 @@ void OmpStructureChecker::HasInvalidLoopBinding(
 
   if (llvm::omp::Directive::OMPD_loop == beginDir.v &&
       CurrentDirectiveIsNested() &&
-      OmpDirectiveSet{llvm::omp::OMPD_teams, llvm::omp::OMPD_target_teams}.test(
-          GetContextParent().directive)) {
+      llvm::omp::bottomTeamsSet.test(GetContextParent().directive)) {
     teamsBindingChecker(
         "`BIND(TEAMS)` must be specified since the `LOOP` region is "
         "strictly nested inside a `TEAMS` region."_err_en_US);
@@ -726,7 +724,7 @@ void OmpStructureChecker::Enter(const parser::OpenMPLoopConstruct &x) {
   HasInvalidDistributeNesting(x);
   HasInvalidLoopBinding(x);
   if (CurrentDirectiveIsNested() &&
-      llvm::omp::topTeamsSet.test(GetContextParent().directive)) {
+      llvm::omp::bottomTeamsSet.test(GetContextParent().directive)) {
     HasInvalidTeamsNesting(beginDir.v, beginDir.source);
   }
   if ((beginDir.v == llvm::omp::Directive::OMPD_distribute_parallel_do_simd) ||
@@ -1169,7 +1167,7 @@ void OmpStructureChecker::Enter(const parser::OpenMPBlockConstruct &x) {
   }
 
   if (CurrentDirectiveIsNested()) {
-    if (llvm::omp::topTeamsSet.test(GetContextParent().directive)) {
+    if (llvm::omp::bottomTeamsSet.test(GetContextParent().directive)) {
       HasInvalidTeamsNesting(beginDir.v, beginDir.source);
     }
     if (GetContext().directive == llvm::omp::Directive::OMPD_master) {
diff --git a/flang/test/Semantics/OpenMP/nested-target.f90 b/flang/test/Semantics/OpenMP/nested-target.f90
index f42b5dde6a08d..6a56a84f4f570 100644
--- a/flang/test/Semantics/OpenMP/nested-target.f90
+++ b/flang/test/Semantics/OpenMP/nested-target.f90
@@ -54,6 +54,7 @@ program main
   n2 = 10
   !$omp target teams map(to:a)
   !PORTABILITY: If TARGET DATA directive is nested inside TARGET region, the behaviour is unspecified
+  !ERROR: Only `DISTRIBUTE`, `PARALLEL`, or `LOOP` regions are allowed to be strictly nested inside `TEAMS` region.
   !$omp target data map(n1,n2)
   do i=1, n1
      do j=1, n2
@@ -65,6 +66,7 @@ program main
 
   !$omp target teams map(to:a) map(from:n1,n2)
   !PORTABILITY: If TARGET TEAMS DISTRIBUTE PARALLEL DO directive is nested inside TARGET region, the behaviour is unspecified
+  !ERROR: Only `DISTRIBUTE`, `PARALLEL`, or `LOOP` regions are allowed to be strictly nested inside `TEAMS` region.
   !$omp target teams distribute parallel do
   do i=1, n1
      do j=1, n2
diff --git a/flang/test/Semantics/OpenMP/nested-teams.f90 b/flang/test/Semantics/OpenMP/nested-teams.f90
index b1a7c92a6906b..974172ee97175 100644
--- a/flang/test/Semantics/OpenMP/nested-teams.f90
+++ b/flang/test/Semantics/OpenMP/nested-teams.f90
@@ -68,6 +68,7 @@ program main
   !$omp end target
 
   !$omp target teams
+  !ERROR: Only `DISTRIBUTE`, `PARALLEL`, or `LOOP` regions are allowed to be strictly nested inside `TEAMS` region.
   !ERROR: TEAMS region can only be strictly nested within the implicit parallel region or TARGET region
   !$omp teams
   a = 3.14

Copy link
Contributor

@kparzysz kparzysz left a comment

Choose a reason for hiding this comment

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

LGTM.

The whole top/bottom sets will need to be refactored eventually, but this patch is ok.

Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

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

LG.

@skatrak skatrak merged commit f13aea1 into llvm:main Feb 12, 2025
11 of 12 checks passed
@skatrak skatrak deleted the omp-teams-semantics-fix branch February 12, 2025 15:24
flovent pushed a commit to flovent/llvm-project that referenced this pull request Feb 13, 2025
)

This patch introduces a directive set for combined constructs where
`teams` is the last leaf. This is used in a couple places to simplify
checks, which is NFC, but it also replaces two incorrect uses of
`topTeamsSet`.

Before, these checks would incorrectly skip combined constructs where
`teams` was the last leaf construct when checking for allowed nested
constructs inside of a `teams` region. Similarly, it would also
incorrectly perform these checks whenever a compound `teams` construct
where `teams` was the first leaf construct was found.
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Feb 14, 2025
)

This patch introduces a directive set for combined constructs where
`teams` is the last leaf. This is used in a couple places to simplify
checks, which is NFC, but it also replaces two incorrect uses of
`topTeamsSet`.

Before, these checks would incorrectly skip combined constructs where
`teams` was the last leaf construct when checking for allowed nested
constructs inside of a `teams` region. Similarly, it would also
incorrectly perform these checks whenever a compound `teams` construct
where `teams` was the first leaf construct was found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:openmp flang:semantics flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants