Skip to content

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

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 13 commits into from
Oct 29, 2024

Conversation

kparzysz
Copy link
Contributor

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
Copy link
Member

llvmbot commented Oct 24, 2024

@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

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.

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unsigned allowedInVersion = 50;
unsigned allowedInVersion{50};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Base automatically changed from users/kparzysz/spr/d02-flang-omp-hint to main October 29, 2024 11:43
@kparzysz kparzysz merged commit d48c849 into main Oct 29, 2024
8 checks passed
@kparzysz kparzysz deleted the users/kparzysz/spr/d03-flang-depend-iterator branch October 29, 2024 13:00
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…13622)

Warn about use of iterators OpenMP versions that didn't have them
(support added in 5.0). Emit a TODO error in lowering.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants