[flang][OpenMP] Parsing support for iterator in DEPEND clause #113622

kparzysz · 2024-10-24T21:26:46Z

Warn about use of iterators OpenMP versions that didn't have them (support added in 5.0). Emit a TODO error in lowering.

Parse the locator list in OmpDependClause as an OmpObjectList (instead of a list of Designators). When a common block appears in the locator list, show an informative message. Implement resolving symbols in DependSinkVec in a dedicated visitor instead of having a visitor for OmpDependClause. Resolve unresolved names common blocks in OmpObjectList. Minor changes to the code organization: - rename OmpDependenceType to OmpTaskDependenceType (to follow 5.2 terminology), - rename Depend::WithLocators to Depend::DepType, - add comments with more detailed spec references to parse-tree.h.

Warn about use of iterators OpenMP versions that didn't have them (support added in 5.0). Emit a TODO error in lowering.

llvmbot · 2024-10-24T21:27:20Z

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

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

Author: Krzysztof Parzyszek (kparzysz)

Changes

Warn about use of iterators OpenMP versions that didn't have them (support added in 5.0). Emit a TODO error in lowering.

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

7 Files Affected:

(modified) flang/include/flang/Parser/parse-tree.h (+3-1)
(modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+34-28)
(modified) flang/lib/Lower/OpenMP/Clauses.cpp (+10-6)
(modified) flang/lib/Parser/openmp-parsers.cpp (+2-1)
(modified) flang/lib/Semantics/check-omp-structure.cpp (+9)
(added) flang/test/Lower/OpenMP/Todo/depend-clause.f90 (+10)
(added) flang/test/Semantics/OpenMP/depend05.f90 (+9)

diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 25e2f42ca29bd5..227fb50fcf7e7e 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3566,7 +3566,9 @@ struct OmpDependClause {
   WRAPPER_CLASS(Sink, std::list<OmpDependSinkVec>);
   struct InOut {
     TUPLE_CLASS_BOILERPLATE(InOut);
-    std::tuple<OmpTaskDependenceType, OmpObjectList> t;
+    std::tuple<std::optional<OmpIteratorModifier>, OmpTaskDependenceType,
+        OmpObjectList>
+        t;
   };
   std::variant<Source, Sink, InOut> u;
 };
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 12fe3b572244f0..c71fbded443dee 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -795,35 +795,41 @@ bool ClauseProcessor::processCopyprivate(
 bool ClauseProcessor::processDepend(mlir::omp::DependClauseOps &result) const {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
 
-  return findRepeatableClause<omp::clause::Depend>(
-      [&](const omp::clause::Depend &clause, const parser::CharBlock &) {
-        using Depend = omp::clause::Depend;
-        assert(std::holds_alternative<Depend::DepType>(clause.u) &&
-               "Only the form with depenence type is handled at the moment");
-        auto &depType = std::get<Depend::DepType>(clause.u);
-        auto kind = std::get<Depend::TaskDependenceType>(depType.t);
-        auto &objects = std::get<omp::ObjectList>(depType.t);
-
-        mlir::omp::ClauseTaskDependAttr dependTypeOperand =
-            genDependKindAttr(firOpBuilder, kind);
-        result.dependKinds.append(objects.size(), dependTypeOperand);
-
-        for (const omp::Object &object : objects) {
-          assert(object.ref() && "Expecting designator");
-
-          if (evaluate::ExtractSubstring(*object.ref())) {
-            TODO(converter.getCurrentLocation(),
-                 "substring not supported for task depend");
-          } else if (evaluate::IsArrayElement(*object.ref())) {
-            TODO(converter.getCurrentLocation(),
-                 "array sections not supported for task depend");
-          }
+  auto process = [&](const omp::clause::Depend &clause,
+                     const parser::CharBlock &) {
+    using Depend = omp::clause::Depend;
+    assert(std::holds_alternative<Depend::DepType>(clause.u) &&
+           "Only the form with depenence type is handled at the moment");
+    auto &depType = std::get<Depend::DepType>(clause.u);
+    auto kind = std::get<Depend::TaskDependenceType>(depType.t);
+    auto &objects = std::get<omp::ObjectList>(depType.t);
+
+    if (std::get<std::optional<omp::clause::Iterator>>(depType.t)) {
+      TODO(converter.getCurrentLocation(),
+           "Support for iterator modifiers is not implemented yet");
+    }
+    mlir::omp::ClauseTaskDependAttr dependTypeOperand =
+        genDependKindAttr(firOpBuilder, kind);
+    result.dependKinds.append(objects.size(), dependTypeOperand);
+
+    for (const omp::Object &object : objects) {
+      assert(object.ref() && "Expecting designator");
+
+      if (evaluate::ExtractSubstring(*object.ref())) {
+        TODO(converter.getCurrentLocation(),
+             "substring not supported for task depend");
+      } else if (evaluate::IsArrayElement(*object.ref())) {
+        TODO(converter.getCurrentLocation(),
+             "array sections not supported for task depend");
+      }
 
-          semantics::Symbol *sym = object.sym();
-          const mlir::Value variable = converter.getSymbolAddress(*sym);
-          result.dependVars.push_back(variable);
-        }
-      });
+      semantics::Symbol *sym = object.sym();
+      const mlir::Value variable = converter.getSymbolAddress(*sym);
+      result.dependVars.push_back(variable);
+    }
+  };
+
+  return findRepeatableClause<omp::clause::Depend>(process);
 }
 
 bool ClauseProcessor::processHasDeviceAddr(
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index 57667fbbf7d4aa..21a246815782c9 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -595,12 +595,16 @@ Depend make(const parser::OmpClause::Depend &inp,
           },
           // Depend::DepType
           [&](const wrapped::InOut &s) -> Variant {
-            auto &t0 = std::get<parser::OmpTaskDependenceType>(s.t);
-            auto &t1 = std::get<parser::OmpObjectList>(s.t);
-            return Depend::DepType{
-                {/*TaskDependenceType=*/convert1(t0.v),
-                 /*Iterator=*/std::nullopt,
-                 /*LocatorList=*/makeObjects(t1, semaCtx)}};
+            auto &t0 =
+                std::get<std::optional<parser::OmpIteratorModifier>>(s.t);
+            auto &t1 = std::get<parser::OmpTaskDependenceType>(s.t);
+            auto &t2 = std::get<parser::OmpObjectList>(s.t);
+
+            auto &&maybeIter = maybeApply(
+                [&](auto &&s) { return makeIterator(s, semaCtx); }, t0);
+            return Depend::DepType{{/*TaskDependenceType=*/convert1(t1.v),
+                                    /*Iterator=*/std::move(maybeIter),
+                                    /*LocatorList=*/makeObjects(t2, semaCtx)}};
           },
       },
       inp.v.u)};
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 3a19d44559dccd..869419b00bf867 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -376,7 +376,8 @@ TYPE_CONTEXT_PARSER("Omp Depend clause"_en_US,
         construct<OmpDependClause>(
             construct<OmpDependClause::Source>("SOURCE"_tok)) ||
         construct<OmpDependClause>(construct<OmpDependClause::InOut>(
-            Parser<OmpTaskDependenceType>{}, ":" >> Parser<OmpObjectList>{})))
+            maybe(Parser<OmpIteratorModifier>{} / ","_tok),
+            Parser<OmpTaskDependenceType>{} / ":", Parser<OmpObjectList>{})))
 
 // 2.15.3.7 LINEAR (linear-list: linear-step)
 //          linear-list -> list | modifier(list)
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index e9a2fcecd9efe6..99f208f1280c2a 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -3307,6 +3307,15 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Depend &x) {
         }
       }
     }
+    if (std::get<std::optional<parser::OmpIteratorModifier>>(inOut->t)) {
+      unsigned version{context_.langOptions().OpenMPVersion};
+      unsigned allowedInVersion = 50;
+      if (version < allowedInVersion) {
+        context_.Say(GetContext().clauseSource,
+            "Iterator modifiers are not supported in %s, %s"_warn_en_US,
+            ThisVersion(version), TryVersion(allowedInVersion));
+      }
+    }
   }
 }
 
diff --git a/flang/test/Lower/OpenMP/Todo/depend-clause.f90 b/flang/test/Lower/OpenMP/Todo/depend-clause.f90
new file mode 100644
index 00000000000000..74525888c91d6d
--- /dev/null
+++ b/flang/test/Lower/OpenMP/Todo/depend-clause.f90
@@ -0,0 +1,10 @@
+!RUN: %not_todo_cmd bbc -emit-hlfir -fopenmp -fopenmp-version=50 -o - %s 2>&1 | FileCheck %s
+!RUN: %not_todo_cmd %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 -o - %s 2>&1 | FileCheck %s
+
+!CHECK: Support for iterator modifiers is not implemented yet
+subroutine f00(x)
+  integer :: x(10)
+  !$omp task depend(iterator(i = 1:10), in: x(i))
+  x = 0
+  !$omp end task
+end
diff --git a/flang/test/Semantics/OpenMP/depend05.f90 b/flang/test/Semantics/OpenMP/depend05.f90
new file mode 100644
index 00000000000000..53fd82bd08a9eb
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/depend05.f90
@@ -0,0 +1,9 @@
+!RUN: %python %S/../test_errors.py %s %flang -fopenmp -fopenmp-version=45 -Werror
+
+subroutine f00(x)
+  integer :: x(10)
+!WARNING: Iterator modifiers are not supported in OpenMP v4.5, try -fopenmp-version=50
+  !$omp task depend(iterator(i = 1:10), in: x(i))
+  x = 0
+  !$omp end task
+end

…kparzysz/spr/d02-flang-omp-hint

…ysz/spr/d03-flang-depend-iterator

kiranchandramohan

LG.

kiranchandramohan · 2024-10-25T12:18:59Z

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

@@ -3307,6 +3307,15 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Depend &x) {
        }
      }
    }
+    if (std::get<std::optional<parser::OmpIteratorModifier>>(inOut->t)) {
+      unsigned version{context_.langOptions().OpenMPVersion};
+      unsigned allowedInVersion = 50;


Suggested change

unsigned allowedInVersion = 50;

unsigned allowedInVersion{50};

tblah · 2024-10-28T10:18:14Z

flang/lib/Lower/OpenMP/ClauseProcessor.cpp

+                     const parser::CharBlock &) {
+    using Depend = omp::clause::Depend;
+    assert(std::holds_alternative<Depend::DepType>(clause.u) &&
+           "Only the form with depenence type is handled at the moment");


Should this be a TODO?

…13622) Warn about use of iterators OpenMP versions that didn't have them (support added in 5.0). Emit a TODO error in lowering.

kparzysz added 3 commits October 24, 2024 15:45

[flang][OpenMP] Extract OMP version hint into helper functions, NFC

eff3d01

[flang][OpenMP] Parsing support for iterator in DEPEND clause

c9cd782

Warn about use of iterators OpenMP versions that didn't have them (support added in 5.0). Emit a TODO error in lowering.

kparzysz requested review from tblah, kiranchandramohan and Leporacanthicus October 24, 2024 21:26

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp flang:semantics flang:parser labels Oct 24, 2024

kparzysz added 7 commits October 24, 2024 16:47

undo leftover changes in symbol.h

72c8f7d

format

09e6720

remove leftover include

952e373

only call ResolveName for unresolved common block names

5c99093

missed OmpDependenceType -> OmpTaskDependenceType

1672f0b

Merge branch 'users/kparzysz/spr/d01-flang-depend-update' into users/…

4482e5d

…kparzysz/spr/d02-flang-omp-hint

Merge branch 'users/kparzysz/spr/d02-flang-omp-hint' into users/kparz…

556c299

…ysz/spr/d03-flang-depend-iterator

kiranchandramohan approved these changes Oct 25, 2024

View reviewed changes

tblah reviewed Oct 28, 2024

View reviewed changes

Base automatically changed from users/kparzysz/spr/d02-flang-omp-hint to main October 29, 2024 11:43

kparzysz added 3 commits October 29, 2024 06:57

convert assertion to todo

97be5c6

use brace initialization

1e0a085

Merge branch 'main' into users/kparzysz/spr/d03-flang-depend-iterator

66afb42

kparzysz merged commit d48c849 into main Oct 29, 2024
8 checks passed

kparzysz deleted the users/kparzysz/spr/d03-flang-depend-iterator branch October 29, 2024 13:00

kparzysz mentioned this pull request Oct 29, 2024

[Flang][OpenMP] Important issues to be fixed prior to removing experimental status of OpenMP #110008

Open

85 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[flang][OpenMP] Parsing support for iterator in DEPEND clause #113622

[flang][OpenMP] Parsing support for iterator in DEPEND clause #113622

Uh oh!

kparzysz commented Oct 24, 2024

Uh oh!

llvmbot commented Oct 24, 2024 •

edited

Loading

Uh oh!

kiranchandramohan left a comment

Uh oh!

kiranchandramohan Oct 25, 2024

Uh oh!

kparzysz Oct 29, 2024

Uh oh!

tblah Oct 28, 2024

Uh oh!

kparzysz Oct 29, 2024

Uh oh!

Uh oh!

Uh oh!

	unsigned allowedInVersion = 50;
	unsigned allowedInVersion{50};

[flang][OpenMP] Parsing support for iterator in DEPEND clause #113622

[flang][OpenMP] Parsing support for iterator in DEPEND clause #113622

Uh oh!

Conversation

kparzysz commented Oct 24, 2024

Uh oh!

llvmbot commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiranchandramohan left a comment

Choose a reason for hiding this comment

Uh oh!

kiranchandramohan Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

kparzysz Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

tblah Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

kparzysz Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Oct 24, 2024 •

edited

Loading