[Flang][OpenMP][Sema] Adding parsing and semantic support for scan directive. #102792

anchuraj · 2024-08-11T06:01:40Z

Scan directive specifies the scan computations. This PR adds the initial parsing and semantic support in flang.

scan is parsed as a stand alone directive as done in clang.

llvmbot · 2024-08-11T06:02:13Z

@llvm/pr-subscribers-flang-parser
@llvm/pr-subscribers-flang-semantics
@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-fir-hlfir

Author: Anchu Rajendran S (anchuraj)

Changes

Scan directive specifies the scan computations. This PR adds the initial parsing and semantic support in flang.

Patch is 28.78 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/102792.diff

13 Files Affected:

(modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+3)
(modified) flang/lib/Parser/openmp-parsers.cpp (+5)
(modified) flang/lib/Parser/unparse.cpp (+3)
(modified) flang/lib/Semantics/check-omp-structure.cpp (+20-12)
(modified) flang/lib/Semantics/resolve-directives.cpp (+16)
(added) flang/test/Parser/OpenMP/scan.f90 (+40)
(modified) flang/test/Semantics/OpenMP/do05.f90 (+4-4)
(modified) flang/test/Semantics/OpenMP/nested-barrier.f90 (+2-2)
(modified) flang/test/Semantics/OpenMP/nested-master.f90 (+1-1)
(modified) flang/test/Semantics/OpenMP/nested-simd.f90 (+17-17)
(modified) flang/test/Semantics/OpenMP/ordered-simd.f90 (+1-1)
(added) flang/test/Semantics/OpenMP/scan.f90 (+19)
(modified) llvm/include/llvm/Frontend/OpenMP/OMP.td (+2)

diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index bbde77c14f36a1..52cc15d4ac6946 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -2189,6 +2189,9 @@ static void genOMPDispatch(lower::AbstractConverter &converter,
   case llvm::omp::Directive::OMPD_parallel:
     genStandaloneParallel(converter, symTable, semaCtx, eval, loc, queue, item);
     break;
+  case llvm::omp::Directive::OMPD_scan:
+    TODO(loc, "Unhandled directive " + llvm::omp::getOpenMPDirectiveName(dir));
+    break;
   case llvm::omp::Directive::OMPD_section:
     llvm_unreachable("genOMPDispatch: OMPD_section");
     // Lowered in the enclosing genSectionsOp.
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 52789d6e5f0f6b..7c10099fe0da8b 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -267,6 +267,8 @@ TYPE_PARSER(
         construct<OmpClause>(construct<OmpClause::DynamicAllocators>()) ||
     "ENTER" >> construct<OmpClause>(construct<OmpClause::Enter>(
                    parenthesized(Parser<OmpObjectList>{}))) ||
+    "EXCLUSIVE" >> construct<OmpClause>(construct<OmpClause::Exclusive>(
+                         parenthesized(Parser<OmpObjectList>{}))) ||
     "FILTER" >> construct<OmpClause>(construct<OmpClause::Filter>(
                     parenthesized(scalarIntExpr))) ||
     "FINAL" >> construct<OmpClause>(construct<OmpClause::Final>(
@@ -286,6 +288,8 @@ TYPE_PARSER(
     "IF" >> construct<OmpClause>(construct<OmpClause::If>(
                 parenthesized(Parser<OmpIfClause>{}))) ||
     "INBRANCH" >> construct<OmpClause>(construct<OmpClause::Inbranch>()) ||
+    "INCLUSIVE" >> construct<OmpClause>(construct<OmpClause::Inclusive>(
+                         parenthesized(Parser<OmpObjectList>{}))) ||
     "IS_DEVICE_PTR" >> construct<OmpClause>(construct<OmpClause::IsDevicePtr>(
                            parenthesized(Parser<OmpObjectList>{}))) ||
     "LASTPRIVATE" >> construct<OmpClause>(construct<OmpClause::Lastprivate>(
@@ -481,6 +485,7 @@ TYPE_PARSER(sourced(construct<OpenMPFlushConstruct>(verbatim("FLUSH"_tok),
 TYPE_PARSER(sourced(construct<OmpSimpleStandaloneDirective>(first(
     "BARRIER" >> pure(llvm::omp::Directive::OMPD_barrier),
     "ORDERED" >> pure(llvm::omp::Directive::OMPD_ordered),
+    "SCAN" >> pure(llvm::omp::Directive::OMPD_scan),
     "TARGET ENTER DATA" >> pure(llvm::omp::Directive::OMPD_target_enter_data),
     "TARGET EXIT DATA" >> pure(llvm::omp::Directive::OMPD_target_exit_data),
     "TARGET UPDATE" >> pure(llvm::omp::Directive::OMPD_target_update),
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 2511a5dda9d095..f252512a41f655 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2295,6 +2295,9 @@ class UnparseVisitor {
     case llvm::omp::Directive::OMPD_barrier:
       Word("BARRIER ");
       break;
+    case llvm::omp::Directive::OMPD_scan:
+      Word("SCAN ");
+      break;
     case llvm::omp::Directive::OMPD_taskwait:
       Word("TASKWAIT ");
       break;
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 50840898438c78..f38b929fb78b3d 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -550,17 +550,21 @@ void OmpStructureChecker::CheckSIMDNest(const parser::OpenMPConstruct &c) {
                         }
                       }
                     },
-                    [&](const parser::OpenMPSimpleStandaloneConstruct &c) {
-                      const auto &dir{
-                          std::get<parser::OmpSimpleStandaloneDirective>(c.t)};
-                      if (dir.v == llvm::omp::Directive::OMPD_ordered) {
-                        const auto &clauses{
-                            std::get<parser::OmpClauseList>(c.t)};
-                        for (const auto &clause : clauses.v) {
-                          if (std::get_if<parser::OmpClause::Simd>(&clause.u)) {
-                            eligibleSIMD = true;
-                            break;
+                    [&](const parser::OpenMPStandaloneConstruct &c) {
+                      if(const auto &simpleConstruct = std::get_if<parser::OpenMPSimpleStandaloneConstruct>(&c.u)) {
+                        const auto &dir{
+                            std::get<parser::OmpSimpleStandaloneDirective>(simpleConstruct->t)};
+                        if (dir.v == llvm::omp::Directive::OMPD_ordered) {
+                          const auto &clauses{
+                              std::get<parser::OmpClauseList>(simpleConstruct->t)};
+                          for (const auto &clause : clauses.v) {
+                            if (std::get_if<parser::OmpClause::Simd>(&clause.u)) {
+                              eligibleSIMD = true;
+                              break;
+                            }
                           }
+                        } else if(dir.v == llvm::omp::Directive::OMPD_scan) {
+                          eligibleSIMD = true;
                         }
                       }
                     },
@@ -571,6 +575,7 @@ void OmpStructureChecker::CheckSIMDNest(const parser::OpenMPConstruct &c) {
                       const auto &beginDir{
                           std::get<parser::OmpLoopDirective>(beginLoopDir.t)};
                       if ((beginDir.v == llvm::omp::Directive::OMPD_simd) ||
+                          (beginDir.v == llvm::omp::Directive::OMPD_parallel_do_simd) ||
                           (beginDir.v == llvm::omp::Directive::OMPD_do_simd)) {
                         eligibleSIMD = true;
                       }
@@ -586,8 +591,9 @@ void OmpStructureChecker::CheckSIMDNest(const parser::OpenMPConstruct &c) {
     context_.Say(parser::FindSourceLocation(c),
         "The only OpenMP constructs that can be encountered during execution "
         "of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, "
-        "the `SIMD` construct and the `ORDERED` construct with the `SIMD` "
-        "clause."_err_en_US);
+        "the `SIMD` construct, the `SCAN` construct and the `ORDERED` "
+        "construct "
+        "with the `SIMD` clause."_err_en_US);
   }
 }
 
@@ -2531,6 +2537,8 @@ void OmpStructureChecker::CheckReductionModifier(
     switch (dirCtx.directive) {
     case llvm::omp::Directive::OMPD_do: // worksharing-loop
     case llvm::omp::Directive::OMPD_do_simd: // worksharing-loop simd
+    case llvm::omp::Directive::OMPD_parallel_do_simd: // worksharing-parallel-loop
+                                                      // simd
     case llvm::omp::Directive::OMPD_simd: // "simd"
       break;
     default:
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index d635a7b8b7874f..018352f072a112 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -1578,9 +1578,11 @@ bool OmpAttributeVisitor::Pre(
     const parser::OpenMPSimpleStandaloneConstruct &x) {
   const auto &standaloneDir{
       std::get<parser::OmpSimpleStandaloneDirective>(x.t)};
+  const auto &parentContext{GetContextIf()};
   switch (standaloneDir.v) {
   case llvm::omp::Directive::OMPD_barrier:
   case llvm::omp::Directive::OMPD_ordered:
+  case llvm::omp::Directive::OMPD_scan:
   case llvm::omp::Directive::OMPD_target_enter_data:
   case llvm::omp::Directive::OMPD_target_exit_data:
   case llvm::omp::Directive::OMPD_target_update:
@@ -1591,6 +1593,20 @@ bool OmpAttributeVisitor::Pre(
   default:
     break;
   }
+  if (standaloneDir.v == llvm::omp::Directive::OMPD_scan) {
+    if ((std::get<parser::OmpClauseList>(x.t).v.size() != 1)) {
+      context_.Say(standaloneDir.source,
+          "Exactly one of `exclusive` or `inclusive` clause is expected"_err_en_US);
+    }
+    if (!parentContext ||
+        (llvm::omp::getDirectiveAssociation(parentContext->directive) !=
+            llvm::omp::Association::Loop)) {
+      context_.Say(standaloneDir.source,
+          "Orphaned `omp scan` directives are prohibited; perhaps you forgot to "
+          "enclose the directive in to a worksharing loop, a worksharing loop "
+          "simd or a simd directive."_err_en_US);
+    }
+  }
   ClearDataSharingAttributeObjects();
   return true;
 }
diff --git a/flang/test/Parser/OpenMP/scan.f90 b/flang/test/Parser/OpenMP/scan.f90
new file mode 100644
index 00000000000000..a6e23ac9404882
--- /dev/null
+++ b/flang/test/Parser/OpenMP/scan.f90
@@ -0,0 +1,40 @@
+! RUN: %flang_fc1 -fdebug-unparse -fopenmp %s | FileCheck --ignore-case %s
+! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp %s | FileCheck --check-prefix="PARSE-TREE" %s
+
+! Check for parsing scan directive
+subroutine test_scan_inclusive()
+  implicit none
+  integer, parameter :: n = 100
+  integer a(n), b(n)
+  integer x, k
+
+  ! initialization
+  x = 0
+  do k = 1, n
+   a(k) = k
+  end do
+
+  ! a(k) is included in the computation of producing results in b(k)
+  !$omp parallel do simd reduction(inscan,+: x)
+  do k = 1, n
+    x = x + a(k)
+    !PARSE-TREE: ExecutionPartConstruct -> ExecutableConstruct -> OpenMPConstruct -> OpenMPStandaloneConstruct -> OpenMPSimpleStandaloneConstruct
+    !PARSE-TREE-NEXT: OmpSimpleStandaloneDirective -> llvm::omp::Directive = scan
+    !PARSE-TREE-NEXT: OmpClauseList -> OmpClause -> Inclusive -> OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'x'
+    !CHECK: !$omp scan inclusive(x)
+    !$omp scan inclusive(x)
+      b(k) = x
+  end do
+
+  ! a(k) is not included in the computation of producing results in b(k)
+  !$omp parallel do simd reduction(inscan,+: x)
+  do k = 1, n
+    b(k) = x
+    !PARSE-TREE: ExecutionPartConstruct -> ExecutableConstruct -> OpenMPConstruct -> OpenMPStandaloneConstruct -> OpenMPSimpleStandaloneConstruct
+    !PARSE-TREE-NEXT: OmpSimpleStandaloneDirective -> llvm::omp::Directive = scan
+    !PARSE-TREE-NEXT: OmpClauseList -> OmpClause -> Exclusive -> OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'x'
+    !CHECK: !$omp scan exclusive(x)
+    !$omp scan exclusive(x)
+    x = x + a(k)
+  end do
+end subroutine
diff --git a/flang/test/Semantics/OpenMP/do05.f90 b/flang/test/Semantics/OpenMP/do05.f90
index c0f240db57b65b..24844f9fe4f62a 100644
--- a/flang/test/Semantics/OpenMP/do05.f90
+++ b/flang/test/Semantics/OpenMP/do05.f90
@@ -39,7 +39,7 @@ program omp_do
     if( i == 5 ) then
         cycle
     end if
-    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
     !ERROR: A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region
     !$omp single
     do j=1,10
@@ -70,7 +70,7 @@ program omp_do
     if( i == 3 ) then
         cycle
     end if
-    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
     !ERROR: A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region
     !$omp single
     do j=1,10
@@ -93,7 +93,7 @@ program omp_do
 
   !$omp target parallel do simd
   do i=1,10
-    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
     !ERROR: A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region
     !$omp single
     do j=1,10
@@ -116,7 +116,7 @@ program omp_do
 
   !$omp target teams distribute parallel do simd
   do i=1,10
-    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
     !ERROR: A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region
     !$omp single
     do j=1,10
diff --git a/flang/test/Semantics/OpenMP/nested-barrier.f90 b/flang/test/Semantics/OpenMP/nested-barrier.f90
index aae283229e330d..ed13c98539c93c 100644
--- a/flang/test/Semantics/OpenMP/nested-barrier.f90
+++ b/flang/test/Semantics/OpenMP/nested-barrier.f90
@@ -17,7 +17,7 @@ program omp_nest_barrier
   !$omp do simd
   do i = 1, 10
     k = k + 1
-    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
     !ERROR: `BARRIER` region may not be closely nested inside of `WORKSHARING`, `LOOP`, `TASK`, `TASKLOOP`,`CRITICAL`, `ORDERED`, `ATOMIC` or `MASTER` region.
     !$omp barrier
     j = j -1
@@ -34,7 +34,7 @@ program omp_nest_barrier
   !$omp parallel do simd
   do i = 1, 10
     k = k + 1
-    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
     !ERROR: `BARRIER` region may not be closely nested inside of `WORKSHARING`, `LOOP`, `TASK`, `TASKLOOP`,`CRITICAL`, `ORDERED`, `ATOMIC` or `MASTER` region.
     !$omp barrier
     j = j -1
diff --git a/flang/test/Semantics/OpenMP/nested-master.f90 b/flang/test/Semantics/OpenMP/nested-master.f90
index 069de67cafae28..0a5118bb947550 100644
--- a/flang/test/Semantics/OpenMP/nested-master.f90
+++ b/flang/test/Semantics/OpenMP/nested-master.f90
@@ -64,7 +64,7 @@ program omp_nest_master
   do i = 1, 10
     k = k + 1
     !WARNING: OpenMP directive 'master' has been deprecated, please use 'masked' instead.
-    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+    !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
     !ERROR: `MASTER` region may not be closely nested inside of `WORKSHARING`, `LOOP`, `TASK`, `TASKLOOP`, or `ATOMIC` region.
     !$omp master
     j = j -1
diff --git a/flang/test/Semantics/OpenMP/nested-simd.f90 b/flang/test/Semantics/OpenMP/nested-simd.f90
index 4149b6d97e9dc7..c9fb90cdeceb25 100644
--- a/flang/test/Semantics/OpenMP/nested-simd.f90
+++ b/flang/test/Semantics/OpenMP/nested-simd.f90
@@ -40,7 +40,7 @@ SUBROUTINE NESTED_BAD(N)
       !$OMP ORDERED SIMD
       DO J = 1,N
         print *, "Hi"
-        !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+        !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
         !ERROR: TEAMS region can only be strictly nested within the implicit parallel region or TARGET region
         !$omp teams 
          DO K = 1,N
@@ -58,25 +58,25 @@ SUBROUTINE NESTED_BAD(N)
     !$OMP ATOMIC
     K =  K + 1
     IF (I <= 10) THEN
-      !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+      !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
       !$omp task 
       do J = 1, N
         K = 2
       end do
       !$omp end task
-      !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+      !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
       !$omp target 
       do J = 1, N
         K = 2
       end do
       !$omp end target
-      !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+      !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
       !$OMP DO
       DO J = 1,N
         A(J) = J
       END DO
       !$OMP END DO
-      !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+      !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
       !$OMP PARALLEL DO
       DO J = 1,N
         A(J) = J
@@ -91,26 +91,26 @@ SUBROUTINE NESTED_BAD(N)
     !$OMP ATOMIC
     K =  K + 1
     IF (I <= 10) THEN
-      !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+      !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct, the `SCAN` construct and the `ORDERED` construct with the `SIMD` clause.
       !$omp task 
       do J = 1, N
         K = 2
       end do
       !$omp end task
-      !ERROR: The only OpenMP constructs that can be encountered during execution of a 'SIMD' region are the `ATOMIC` construct, the `LOOP` construct, the `SIMD` construct and the `ORDERED` construct with the `SIMD` clause.
+      !ERROR:...
[truncated]

github-actions · 2024-08-11T06:05:17Z

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

mjklemm · 2024-08-19T12:55:38Z

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

+  common::visit(
+      Fortran::common::visitors{
+          // Allow `!$OMP ORDERED SIMD`
+          [&](const parser::OpenMPBlockConstruct &c) {
+            const auto &beginBlockDir{
+                std::get<parser::OmpBeginBlockDirective>(c.t)};
+            const auto &beginDir{
+                std::get<parser::OmpBlockDirective>(beginBlockDir.t)};
+            if (beginDir.v == llvm::omp::Directive::OMPD_ordered) {
+              const auto &clauses{
+                  std::get<parser::OmpClauseList>(beginBlockDir.t)};
+              for (const auto &clause : clauses.v) {
+                if (std::get_if<parser::OmpClause::Simd>(&clause.u)) {
+                  eligibleSIMD = true;


This an the lines below are pure formatting changes. Can you please do those after the review?

Thank you! Replied here. I am unable to remove them at the moment. Please let me know your suggestion.

skatrak

Thank you Anchu, this generally looks good to me. I only have some minor comments.

flang/test/Parser/OpenMP/scan.f90

skatrak · 2024-08-21T15:21:48Z

flang/test/Parser/OpenMP/scan.f90

+    !$omp scan exclusive(x)
+    x = x + a(k)
+  end do
+end subroutine


Add an instance with multiple variables: !$omp scan inclusive(x, y).

Updated. Thank you!

Shouldn't these new tests trigger a semantics error for y? It doesn't appear in a reduction clause with the inscan modifier. I guess we're missing that check at the moment.

Yeah, it is yet to be added. I will add it in the next PR.

I have modified this PR to include the semantic check. :)

flang/lib/Semantics/resolve-directives.cpp

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

flang/test/Semantics/OpenMP/scan.f90

anchuraj · 2024-08-28T23:53:42Z

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

                          }
+                        } else if (dir.v == llvm::omp::Directive::OMPD_scan) {
+                          eligibleSIMD = true;


Hi @mjklemm , Thank you for reviewing. This is the least change I wanted to make (with manual formatting). However clang-format complains that the whole lambda is not formatted when I add this. My R2 commit does not pass the clang-format. So I am updating. Please let me know if you have an alternate suggestion.

diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp index 0cbfdb4516..66149d0495 100644 --- a/flang/lib/Semantics/check-omp-structure.cpp +++ b/flang/lib/Semantics/check-omp-structure.cpp @@ -533,61 +533,62 @@ void OmpStructureChecker::CheckSIMDNest(const parser::OpenMPConstruct &c) { // current context yet. // TODO: Check for declare simd regions. bool eligibleSIMD{false}; - common::visit(Fortran::common::visitors{ - // Allow `!$OMP ORDERED SIMD` - [&](const parser::OpenMPBlockConstruct &c) { - const auto &beginBlockDir{ - std::get<parser::OmpBeginBlockDirective>(c.t)}; - const auto &beginDir{ - std::get<parser::OmpBlockDirective>(beginBlockDir.t)}; - if (beginDir.v == llvm::omp::Directive::OMPD_ordered) { - const auto &clauses{ - std::get<parser::OmpClauseList>(beginBlockDir.t)}; - for (const auto &clause : clauses.v) { - if (std::get_if<parser::OmpClause::Simd>(&clause.u)) { - eligibleSIMD = true; - break; - } - } - } - }, - [&](const parser::OpenMPStandaloneConstruct &c) { - if (const auto &simpleConstruct = - std::get_if<parser::OpenMPSimpleStandaloneConstruct>( - &c.u)) { - const auto &dir{std::get<parser::OmpSimpleStandaloneDirective>( - simpleConstruct->t)}; - if (dir.v == llvm::omp::Directive::OMPD_ordered) { - const auto &clauses{ - std::get<parser::OmpClauseList>(simpleConstruct->t)}; - for (const auto &clause : clauses.v) { - if (std::get_if<parser::OmpClause::Simd>(&clause.u)) { - eligibleSIMD = true; - break; - } - } - } else if (dir.v == llvm::omp::Directive::OMPD_scan) { - eligibleSIMD = true; - } - } - }, - // Allowing SIMD construct - [&](const parser::OpenMPLoopConstruct &c) { - const auto &beginLoopDir{ - std::get<parser::OmpBeginLoopDirective>(c.t)}; - const auto &beginDir{ - std::get<parser::OmpLoopDirective>(beginLoopDir.t)}; - if ((beginDir.v == llvm::omp::Directive::OMPD_simd) || - (beginDir.v == llvm::omp::Directive::OMPD_do_simd)) { - eligibleSIMD = true; - } - }, - [&](const parser::OpenMPAtomicConstruct &c) { - // Allow `!$OMP ATOMIC` - eligibleSIMD = true; - }, - [&](const auto &c) {}, - }, + common::visit( + Fortran::common::visitors{ + // Allow `!$OMP ORDERED SIMD` + [&](const parser::OpenMPBlockConstruct &c) { + const auto &beginBlockDir{ + std::get<parser::OmpBeginBlockDirective>(c.t)}; + const auto &beginDir{ + std::get<parser::OmpBlockDirective>(beginBlockDir.t)}; + if (beginDir.v == llvm::omp::Directive::OMPD_ordered) { + const auto &clauses{ + std::get<parser::OmpClauseList>(beginBlockDir.t)}; + for (const auto &clause : clauses.v) { + if (std::get_if<parser::OmpClause::Simd>(&clause.u)) { + eligibleSIMD = true; + break; + } + } + } + }, + [&](const parser::OpenMPStandaloneConstruct &c) { + if (const auto &simpleConstruct = + std::get_if<parser::OpenMPSimpleStandaloneConstruct>( + &c.u)) { + const auto &dir{std::get<parser::OmpSimpleStandaloneDirective>( + simpleConstruct->t)}; + if (dir.v == llvm::omp::Directive::OMPD_ordered) { + const auto &clauses{ + std::get<parser::OmpClauseList>(simpleConstruct->t)}; + for (const auto &clause : clauses.v) { + if (std::get_if<parser::OmpClause::Simd>(&clause.u)) { + eligibleSIMD = true; + break; + } + } + } else if (dir.v == llvm::omp::Directive::OMPD_scan) { + eligibleSIMD = true; + } + } + }, + // Allowing SIMD construct + [&](const parser::OpenMPLoopConstruct &c) { + const auto &beginLoopDir{ + std::get<parser::OmpBeginLoopDirective>(c.t)}; + const auto &beginDir{ + std::get<parser::OmpLoopDirective>(beginLoopDir.t)}; + if ((beginDir.v == llvm::omp::Directive::OMPD_simd) || + (beginDir.v == llvm::omp::Directive::OMPD_do_simd)) { + eligibleSIMD = true; + } + }, + [&](const parser::OpenMPAtomicConstruct &c) { + // Allow `!$OMP ATOMIC` + eligibleSIMD = true; + }, + [&](const auto &c) {}, + }, c.u); if (!eligibleSIMD) { context_.Say(parser::FindSourceLocation(c), error: some formatters failed: clang-format Error: Process completed with exit code 1.

I think you can ignore the formatter for a commit and later do a commit that only has the formatting changes. Anyways, looks good to me.

Thank you for the suggestion. If I am understanding this right, I did the same. For R2, I ignored the formatter and R3 had only formatting change. :)

anchuraj · 2024-08-28T23:59:56Z

Thank you @skatrak for reviewing. I have made the requested changes+

skatrak

Thank you again Anchu for working on my comments. I've got a couple more minor nits.

skatrak · 2024-08-29T10:16:51Z

flang/include/flang/Semantics/openmp-directive-sets.h

@@ -194,6 +194,7 @@ static const OmpDirectiveSet allDistributeParallelDoSimdSet{
 static const OmpDirectiveSet allDistributeSimdSet{
    allDistributeSet & allSimdSet};
 static const OmpDirectiveSet allDoSimdSet{allDoSet & allSimdSet};
+static const OmpDirectiveSet scanAllowedSet{allDoSet | allSimdSet};


I'd suggest placing this into a new section in this header, since it doesn't belong in any of the ones that already exist. Maybe call the variable scanParentAllowedSet and the section "Directive sets for allowed/not allowed parent directives" or something like this.

Made the change. Thank you for the suggestion.

flang/lib/Semantics/resolve-directives.cpp

skatrak · 2024-08-29T10:27:21Z

flang/test/Parser/OpenMP/scan.f90

+    !$omp scan exclusive(x)
+    x = x + a(k)
+  end do
+end subroutine


Shouldn't these new tests trigger a semantics error for y? It doesn't appear in a reduction clause with the inscan modifier. I guess we're missing that check at the moment.

flang/test/Semantics/OpenMP/out

flang/test/Semantics/OpenMP/scan.f90

mjklemm

LGTM

mjklemm · 2024-09-03T12:14:16Z

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

                          }
+                        } else if (dir.v == llvm::omp::Directive::OMPD_scan) {
+                          eligibleSIMD = true;


I think you can ignore the formatter for a commit and later do a commit that only has the formatting changes. Anyways, looks good to me.

…eck-omp-structure

skatrak

Thank you again for your improvements to this PR! I have some comments on the implementation changes, but they should be relatively simple to address.

skatrak · 2024-09-13T12:01:40Z

flang/include/flang/Semantics/openmp-directive-sets.h

+//===----------------------------------------------------------------------===//
+// Directive sets for parent directives that do allow/not allow a construct
+static const OmpDirectiveSet scanParentAllowedSet{allDoSet | allSimdSet};
+//===----------------------------------------------------------------------===//


Suggested change

//===----------------------------------------------------------------------===//

// Directive sets for parent directives that do allow/not allow a construct

static const OmpDirectiveSet scanParentAllowedSet{allDoSet | allSimdSet};

//===----------------------------------------------------------------------===//

//===----------------------------------------------------------------------===//

// Directive sets for parent directives that do allow/not allow a construct

//===----------------------------------------------------------------------===//

static const OmpDirectiveSet scanParentAllowedSet{allDoSet | allSimdSet};

skatrak · 2024-09-13T12:03:26Z

flang/include/flang/Semantics/openmp-directive-sets.h

@@ -186,7 +186,7 @@ static const OmpDirectiveSet allTeamsSet{
 // Directive sets for groups of multiple directives
 //===----------------------------------------------------------------------===//

-// Composite constructs
+// Directive sets that form Composite constructs


Nit: Not sure if the change is necessary, but in that case "composite" should be lowercase.

skatrak · 2024-09-13T12:30:01Z

flang/lib/Semantics/check-directive-structure.h

@@ -198,6 +198,10 @@ class DirectiveStructureChecker : public virtual BaseChecker {
    ClauseMapTy clauseInfo;
    std::list<C> actualClauses;
    std::list<C> crtGroup;
+    std::set<std::string> usedInScanDirective;


I think std::set<const Symbol *> would be a better data type choice for this. Another issue is that this structure is used for both OpenMP and OpenACC, so I think this should be moved to the OmpStructureChecker subclass (in check-omp-structure.h). Same issue with the reductionMod below.

Hi Sergio, I think it is better to get represented in directive context stack . So once the control goes out of the reduction (directive context), the symbol is no longer marked. Otherwise, some of the semantic checks (like checking symbols used in scan directive are used in inscan reduction) may be invalid if we do not account for scope.

The symbol * of reduction variables and scan variables are different now. Reduction declares a private symbol as implemented here. So checking Symbol * would not be appropriate at this stage. Looking to make further modification of using the same Symbol * in both clauses (if required) as I proceed with the implementation.

skatrak · 2024-09-13T12:36:05Z

flang/lib/Semantics/check-directive-structure.h

+    std::set<std::string> usedInScanDirective;
+
+    using ReductionModifier = parser::OmpReductionClause::ReductionModifier;
+    std::map<const std::string, ReductionModifier> reductionMod;


Suggested change

std::map<const std::string, ReductionModifier> reductionMod;

std::map<const Symbol *, ReductionModifier> reductionMod;

skatrak · 2024-09-13T12:37:36Z

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

+  for (const auto &clause : clauseList.v) {
+    if (const auto *reductionClause{
+            std::get_if<parser::OmpClause::Reduction>(&clause.u)}) {
+      using ReductionModifier = parser::OmpReductionClause::ReductionModifier;


This should not be necessary here if it's already defined at the class level.

Suggested change

using ReductionModifier = parser::OmpReductionClause::ReductionModifier;

skatrak · 2024-09-13T12:56:40Z

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

+                      }
+                    }
+                  },
+                  [&](const auto &name) {},


Shouldn't we check name.symbol as well here? I understand this code path is triggered by passing a common block to the reduction clause, which doesn't seem to be forbidden by the spec (I could be wrong, though). In that case, perhaps the simplest solution here would be to extract part of the body of the parser::Designator case above into a lambda taking a Symbol * as parameter to be called from both places.

Modified this. Thankyou.

skatrak · 2024-09-13T13:01:18Z

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

@@ -2314,7 +2378,24 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Reduction &x) {
  if (CheckReductionOperators(x)) {
    CheckReductionTypeList(x);
  }
-  CheckReductionModifier(x);
+  using ReductionModifier = parser::OmpReductionClause::ReductionModifier;


Suggested change

using ReductionModifier = parser::OmpReductionClause::ReductionModifier;

skatrak · 2024-09-13T13:08:51Z

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

+                }
+              }
+            },
+            [&](const auto &name) {},


Same as the comment above: support common blocks (if allowed) by sharing implementation with the case above.

skatrak · 2024-09-13T13:09:50Z

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

-  if (!maybeModifier || *maybeModifier == ReductionModifier::Default) {
-    // No modifier, or the default one is always ok.
+  if (modifier == ReductionModifier::Default) {
+    // the default one is always ok.


Suggested change

// the default one is always ok.

// The default one is always ok.

skatrak · 2024-09-13T13:19:41Z

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

+  void checkAndAddSymbolsToUsedInScanList(const parser::OmpObjectList &x);
+  void addModifiertoMap(const parser::OmpObjectList &x,
+      parser::OmpReductionClause::ReductionModifier &modifier);


Nit: Follow capitalization conventions on this file. Also, I think the name for one of these methods can be improved, but feel free to name it differently from what I'm suggesting if you think it's better.

Suggested change

void checkAndAddSymbolsToUsedInScanList(const parser::OmpObjectList &x);

void addModifiertoMap(const parser::OmpObjectList &x,

parser::OmpReductionClause::ReductionModifier &modifier);

void CheckAndRegisterScanSymbols(const parser::OmpObjectList &x);

void AddModifierToMap(const parser::OmpObjectList &x,

parser::OmpReductionClause::ReductionModifier &modifier);

anchuraj requested review from shraiysh and kparzysz August 11, 2024 06:01

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp flang:semantics flang:parser clang:openmp OpenMP related changes to Clang labels Aug 11, 2024

anchuraj requested review from skatrak, mjklemm and klausler August 11, 2024 06:02

anchuraj removed request for skatrak, mjklemm, klausler, shraiysh and kparzysz August 12, 2024 03:52

anchuraj force-pushed the scan-parse branch 4 times, most recently from 9a045a6 to 5bf0f84 Compare August 13, 2024 19:30

anchuraj requested review from skatrak, mjklemm, klausler, shraiysh and kparzysz and removed request for skatrak August 14, 2024 16:37

klausler removed their request for review August 17, 2024 19:58

mjklemm reviewed Aug 19, 2024

View reviewed changes

skatrak requested review from tblah, kiranchandramohan and raghavendhra August 21, 2024 15:00

skatrak reviewed Aug 21, 2024

View reviewed changes

anchuraj commented Aug 28, 2024

View reviewed changes

anchuraj changed the title ~~Adding parsing and semantic support for scan directive.~~ [Flang][OpenMP][Sema] Adding parsing and semantic support for scan directive. Aug 29, 2024

skatrak reviewed Aug 29, 2024

View reviewed changes

mjklemm approved these changes Sep 3, 2024

View reviewed changes

anchuraj requested a review from skatrak September 3, 2024 16:58

anchuraj force-pushed the scan-parse branch 2 times, most recently from da82c48 to 7e1d4cf Compare September 9, 2024 18:53

anchuraj added 6 commits September 9, 2024 14:40

Adding parsing support for scan directive.

c9942cc

R2: Addressing review comments

e061e42

R3: Adding formatted file

d687339

R4: Addressing review comments

7087dcf

R5: Addressing a few more review comments

23fd963

R6: Reworked to add a few more semantic checks and moved checks to ch…

d71e947

…eck-omp-structure

anchuraj force-pushed the scan-parse branch from 7e1d4cf to d71e947 Compare September 9, 2024 19:44

anchuraj mentioned this pull request Sep 9, 2024

[MLIR][OpenMP]Adding MLIR Op definition for scan #107925

Open

skatrak reviewed Sep 13, 2024

View reviewed changes

R7: Addressed a few review comments

693760e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flang][OpenMP][Sema] Adding parsing and semantic support for scan directive. #102792

[Flang][OpenMP][Sema] Adding parsing and semantic support for scan directive. #102792

anchuraj commented Aug 11, 2024 •

edited

Loading

llvmbot commented Aug 11, 2024 •

edited

Loading

github-actions bot commented Aug 11, 2024 •

edited

Loading

mjklemm Aug 19, 2024

anchuraj Aug 28, 2024

skatrak left a comment

skatrak Aug 21, 2024

anchuraj Aug 28, 2024

skatrak Aug 29, 2024

anchuraj Sep 3, 2024

anchuraj Sep 9, 2024

anchuraj Aug 28, 2024

mjklemm Sep 3, 2024

anchuraj Sep 11, 2024

anchuraj commented Aug 28, 2024

skatrak left a comment

skatrak Aug 29, 2024

anchuraj Aug 29, 2024

skatrak Aug 29, 2024

mjklemm left a comment

mjklemm Sep 3, 2024

skatrak left a comment

skatrak Sep 13, 2024

skatrak Sep 13, 2024

skatrak Sep 13, 2024

anchuraj Sep 20, 2024 •

edited

Loading

anchuraj Sep 23, 2024

skatrak Sep 13, 2024

skatrak Sep 13, 2024

skatrak Sep 13, 2024

anchuraj Sep 23, 2024

skatrak Sep 13, 2024

skatrak Sep 13, 2024

skatrak Sep 13, 2024

skatrak Sep 13, 2024

	std::map<const std::string, ReductionModifier> reductionMod;
	std::map<const Symbol *, ReductionModifier> reductionMod;

	// the default one is always ok.
	// The default one is always ok.

[Flang][OpenMP][Sema] Adding parsing and semantic support for scan directive. #102792

Are you sure you want to change the base?

[Flang][OpenMP][Sema] Adding parsing and semantic support for scan directive. #102792

Conversation

anchuraj commented Aug 11, 2024 • edited Loading

llvmbot commented Aug 11, 2024 • edited Loading

github-actions bot commented Aug 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skatrak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anchuraj commented Aug 28, 2024

skatrak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjklemm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skatrak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anchuraj Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anchuraj commented Aug 11, 2024 •

edited

Loading

llvmbot commented Aug 11, 2024 •

edited

Loading

github-actions bot commented Aug 11, 2024 •

edited

Loading

anchuraj Sep 20, 2024 •

edited

Loading