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] Parse iterators, add to MAP clause, TODO for lowering #113167

Merged
merged 10 commits into from
Oct 23, 2024

Conversation

kparzysz
Copy link
Contributor

Define OmpIteratorSpecifier and OmpIteratorModifier parser classes, and add parsing for them. Those are reusable between any clauses that use iterator modifiers.

Add support for iterator modifiers to the MAP clause up to lowering, where a TODO message is emitted.

Define `OmpIteratorSpecifier` and `OmpIteratorModifier` parser classes,
and add parsing for them. Those are reusable between any clauses that
use iterator modifiers.

Add support for iterator modifiers to the MAP clause up to lowering,
where a TODO message is emitted.
@llvmbot 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 Oct 21, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 21, 2024

@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-parser

Author: Krzysztof Parzyszek (kparzysz)

Changes

Define OmpIteratorSpecifier and OmpIteratorModifier parser classes, and add parsing for them. Those are reusable between any clauses that use iterator modifiers.

Add support for iterator modifiers to the MAP clause up to lowering, where a TODO message is emitted.


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

17 Files Affected:

  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+2)
  • (modified) flang/include/flang/Parser/parse-tree.h (+25-6)
  • (modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+56-49)
  • (modified) flang/lib/Lower/OpenMP/Clauses.cpp (+63-9)
  • (modified) flang/lib/Lower/OpenMP/Clauses.h (+5-6)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+154-60)
  • (modified) flang/lib/Parser/type-parsers.h (+1)
  • (modified) flang/lib/Parser/unparse.cpp (+41-14)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+97-4)
  • (modified) flang/lib/Semantics/check-omp-structure.h (+25)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+5-2)
  • (modified) flang/lib/Semantics/resolve-names.cpp (+35)
  • (modified) flang/test/Parser/OpenMP/map-modifiers.f90 (+190-8)
  • (modified) flang/test/Semantics/OpenMP/map-modifiers.f90 (+75-3)
  • (modified) flang/test/Semantics/OpenMP/symbol08.f90 (+9-9)
  • (modified) llvm/include/llvm/Frontend/OpenMP/OMP.h (+3)
  • (modified) llvm/lib/Frontend/OpenMP/OMP.cpp (+14)
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index ccbe5475d051e0..040065c0cbc029 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -474,6 +474,8 @@ class ParseTreeDumper {
   NODE(parser, NullInit)
   NODE(parser, ObjectDecl)
   NODE(parser, OldParameterStmt)
+  NODE(parser, OmpIteratorSpecifier)
+  NODE(parser, OmpIteratorModifier)
   NODE(parser, OmpAlignedClause)
   NODE(parser, OmpAtomic)
   NODE(parser, OmpAtomicCapture)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 4a3c992c4ec51b..35821288699fdb 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3424,7 +3424,17 @@ struct AssignedGotoStmt {
 
 WRAPPER_CLASS(PauseStmt, std::optional<StopCode>);
 
-// Parse tree nodes for OpenMP 4.5 directives and clauses
+// Parse tree nodes for OpenMP 4.5+ directives and clauses
+
+// [5.0] 2.1.6 iterator-specifier -> type-declaration-stmt = subscript-triple
+//             iterator-modifier -> iterator-specifier-list
+struct OmpIteratorSpecifier {
+  TUPLE_CLASS_BOILERPLATE(OmpIteratorSpecifier);
+  CharBlock source;
+  std::tuple<TypeDeclarationStmt, SubscriptTriplet> t;
+};
+
+WRAPPER_CLASS(OmpIteratorModifier, std::list<OmpIteratorSpecifier>);
 
 // 2.5 proc-bind-clause -> PROC_BIND (MASTER | CLOSE | SPREAD)
 struct OmpProcBindClause {
@@ -3450,16 +3460,25 @@ struct OmpObject {
 WRAPPER_CLASS(OmpObjectList, std::list<OmpObject>);
 
 // 2.15.5.1 map ->
-//    MAP ([ [map-type-modifiers [,] ] map-type : ] variable-name-list)
-// map-type-modifiers -> map-type-modifier [,] [...]
+//    MAP ([[map-type-modifier-list [,]] [iterator-modifier [,]] map-type : ]
+//         variable-name-list)
+// map-type-modifier-list -> map-type-modifier [,] [...]
 // map-type-modifier -> ALWAYS | CLOSE | PRESENT | OMPX_HOLD
 // map-type -> TO | FROM | TOFROM | ALLOC | RELEASE | DELETE
 struct OmpMapClause {
-  ENUM_CLASS(TypeModifier, Always, Close, Present, OmpxHold);
+  ENUM_CLASS(TypeModifier, Always, Close, Present, Ompx_Hold);
   ENUM_CLASS(Type, To, From, Tofrom, Alloc, Release, Delete)
   TUPLE_CLASS_BOILERPLATE(OmpMapClause);
-  std::tuple<std::optional<std::list<TypeModifier>>, std::optional<Type>,
-      OmpObjectList>
+
+  // All modifiers are parsed into optional lists, even if they are unique.
+  // The checks for satisfying those constraints are deferred to semantics.
+  // In OpenMP 5.2 the non-comma syntax has been deprecated: keep the
+  // information about separator presence to emit a diagnostic if needed.
+  std::tuple<std::optional<std::list<TypeModifier>>,
+             std::optional<std::list<OmpIteratorModifier>>, // unique
+             std::optional<std::list<Type>>,                // unique
+             OmpObjectList,
+             bool> // were the modifiers comma-separated
       t;
 };
 
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 88c443b4198ab0..fbc031f3a93d7d 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -936,57 +936,64 @@ bool ClauseProcessor::processMap(
            llvm::SmallVector<OmpMapMemberIndicesData>>
       parentMemberIndices;
 
-  bool clauseFound = findRepeatableClause<omp::clause::Map>(
-      [&](const omp::clause::Map &clause, const parser::CharBlock &source) {
-        using Map = omp::clause::Map;
-        mlir::Location clauseLocation = converter.genLocation(source);
-        const auto &mapType = std::get<std::optional<Map::MapType>>(clause.t);
-        llvm::omp::OpenMPOffloadMappingFlags mapTypeBits =
-            llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE;
-        // If the map type is specified, then process it else Tofrom is the
-        // default.
-        Map::MapType type = mapType.value_or(Map::MapType::Tofrom);
-        switch (type) {
-        case Map::MapType::To:
-          mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO;
-          break;
-        case Map::MapType::From:
-          mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
-          break;
-        case Map::MapType::Tofrom:
-          mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO |
-                         llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
-          break;
-        case Map::MapType::Alloc:
-        case Map::MapType::Release:
-          // alloc and release is the default map_type for the Target Data
-          // Ops, i.e. if no bits for map_type is supplied then alloc/release
-          // is implicitly assumed based on the target directive. Default
-          // value for Target Data and Enter Data is alloc and for Exit Data
-          // it is release.
-          break;
-        case Map::MapType::Delete:
-          mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_DELETE;
-        }
+  auto process = [&](const omp::clause::Map &clause,
+                     const parser::CharBlock &source) {
+    using Map = omp::clause::Map;
+    mlir::Location clauseLocation = converter.genLocation(source);
+    const auto &mapType = std::get<std::optional<Map::MapType>>(clause.t);
+    llvm::omp::OpenMPOffloadMappingFlags mapTypeBits =
+        llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE;
+    // If the map type is specified, then process it else Tofrom is the
+    // default.
+    Map::MapType type = mapType.value_or(Map::MapType::Tofrom);
+    switch (type) {
+    case Map::MapType::To:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO;
+      break;
+    case Map::MapType::From:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+      break;
+    case Map::MapType::Tofrom:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO |
+                     llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+      break;
+    case Map::MapType::Alloc:
+    case Map::MapType::Release:
+      // alloc and release is the default map_type for the Target Data
+      // Ops, i.e. if no bits for map_type is supplied then alloc/release
+      // is implicitly assumed based on the target directive. Default
+      // value for Target Data and Enter Data is alloc and for Exit Data
+      // it is release.
+      break;
+    case Map::MapType::Delete:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_DELETE;
+    }
 
-        auto &modTypeMods =
-            std::get<std::optional<Map::MapTypeModifiers>>(clause.t);
-        if (modTypeMods) {
-          if (llvm::is_contained(*modTypeMods, Map::MapTypeModifier::Always))
-            mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS;
-          // Diagnose unimplemented map-type-modifiers.
-          if (llvm::any_of(*modTypeMods, [](Map::MapTypeModifier m) {
-                return m != Map::MapTypeModifier::Always;
-              })) {
-            TODO(currentLocation, "Map type modifiers (other than 'ALWAYS')"
-                                  " are not implemented yet");
-          }
-        }
-        processMapObjects(stmtCtx, clauseLocation,
-                          std::get<omp::ObjectList>(clause.t), mapTypeBits,
-                          parentMemberIndices, result.mapVars, *ptrMapSyms);
-      });
+    auto &modTypeMods =
+        std::get<std::optional<Map::MapTypeModifiers>>(clause.t);
+    if (modTypeMods) {
+      if (llvm::is_contained(*modTypeMods, Map::MapTypeModifier::Always))
+        mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS;
+      // Diagnose unimplemented map-type-modifiers.
+      if (llvm::any_of(*modTypeMods, [](Map::MapTypeModifier m) {
+            return m != Map::MapTypeModifier::Always;
+          })) {
+        TODO(currentLocation, "Map type modifiers (other than 'ALWAYS')"
+                              " are not implemented yet");
+      }
+    }
+
+    if (std::get<std::optional<omp::clause::Iterator>>(clause.t)) {
+      TODO(currentLocation,
+           "Support for iterator modifiers is not implemented yet");
+    }
+
+    processMapObjects(stmtCtx, clauseLocation,
+                      std::get<omp::ObjectList>(clause.t), mapTypeBits,
+                      parentMemberIndices, result.mapVars, *ptrMapSyms);
+  };
 
+  bool clauseFound = findRepeatableClause<omp::clause::Map>(process);
   insertChildMapInfoIntoParent(converter, parentMemberIndices, result.mapVars,
                                *ptrMapSyms);
 
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index 64d661256a187e..8974b4211b9684 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -232,6 +232,46 @@ MAKE_INCOMPLETE_CLASS(Match, Match);
 // MAKE_INCOMPLETE_CLASS(Otherwise, );   // missing-in-parser
 MAKE_INCOMPLETE_CLASS(When, When);
 
+List<IteratorSpecifier>
+makeIteratorSpecifiers(const parser::OmpIteratorSpecifier &inp,
+                       semantics::SemanticsContext &semaCtx) {
+  List<IteratorSpecifier> specifiers;
+
+  auto &[begin, end, step] = std::get<parser::SubscriptTriplet>(inp.t).t;
+  assert(begin && end && "Expecting begin/end values");
+  evaluate::ExpressionAnalyzer ea{semaCtx};
+
+  MaybeExpr rbegin{ea.Analyze(*begin)}, rend{ea.Analyze(*end)};
+  MaybeExpr rstep;
+  if (step)
+    rstep = ea.Analyze(*step);
+
+  assert(rbegin && rend && "Unable to get range bounds");
+  Range range{{*rbegin, *rend, rstep}};
+
+  auto &tds = std::get<parser::TypeDeclarationStmt>(inp.t);
+  auto &entities = std::get<std::list<parser::EntityDecl>>(tds.t);
+  for (const parser::EntityDecl &ed : entities) {
+    auto &name = std::get<parser::ObjectName>(ed.t);
+    assert(name.symbol && "Expecting symbol for iterator variable");
+    auto *stype = name.symbol->GetType();
+    assert(stype && "Expecting symbol type");
+    IteratorSpecifier spec{{evaluate::DynamicType::From(*stype),
+                            makeObject(name, semaCtx), range}};
+    specifiers.emplace_back(std::move(spec));
+  }
+
+  return specifiers;
+}
+
+Iterator makeIterator(const parser::OmpIteratorModifier &inp,
+                      semantics::SemanticsContext &semaCtx) {
+  Iterator iterator;
+  for (auto &&spec : inp.v)
+    llvm::append_range(iterator, makeIteratorSpecifiers(spec, semaCtx));
+  return iterator;
+}
+
 DefinedOperator makeDefinedOperator(const parser::DefinedOperator &inp,
                                     semantics::SemanticsContext &semaCtx) {
   CLAUSET_ENUM_CONVERT( //
@@ -843,18 +883,32 @@ Map make(const parser::OmpClause::Map &inp,
   CLAUSET_ENUM_CONVERT( //
       convert2, parser::OmpMapClause::TypeModifier, Map::MapTypeModifier,
       // clang-format off
-      MS(Always,   Always)
-      MS(Close,    Close)
-      MS(OmpxHold, OmpxHold)
-      MS(Present,  Present)
+      MS(Always,    Always)
+      MS(Close,     Close)
+      MS(Ompx_Hold, OmpxHold)
+      MS(Present,   Present)
       // clang-format on
   );
 
   auto &t0 = std::get<std::optional<std::list<wrapped::TypeModifier>>>(inp.v.t);
-  auto &t1 = std::get<std::optional<wrapped::Type>>(inp.v.t);
-  auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
+  auto &t1 =
+      std::get<std::optional<std::list<parser::OmpIteratorModifier>>>(inp.v.t);
+  auto &t2 = std::get<std::optional<std::list<wrapped::Type>>>(inp.v.t);
+  auto &t3 = std::get<parser::OmpObjectList>(inp.v.t);
+
+  // These should have been diagnosed already.
+  assert((!t1 || t1->size() == 1) && "Only one iterator modifier is allowed");
+  assert((!t2 || t2->size() == 1) && "Only one map type is allowed");
+
+  auto iterator = [&]() -> std::optional<Iterator> {
+    if (t1)
+      return makeIterator(t1->front(), semaCtx);
+    return std::nullopt;
+  }();
 
-  std::optional<Map::MapType> maybeType = maybeApply(convert1, t1);
+  std::optional<Map::MapType> maybeType;
+  if (t2)
+    maybeType = maybeApply(convert1, std::optional<wrapped::Type>(t2->front()));
 
   std::optional<Map::MapTypeModifiers> maybeTypeMods = maybeApply(
       [&](const std::list<wrapped::TypeModifier> &typeMods) {
@@ -867,8 +921,8 @@ Map make(const parser::OmpClause::Map &inp,
 
   return Map{{/*MapType=*/maybeType,
               /*MapTypeModifiers=*/maybeTypeMods,
-              /*Mapper=*/std::nullopt, /*Iterator=*/std::nullopt,
-              /*LocatorList=*/makeObjects(t2, semaCtx)}};
+              /*Mapper=*/std::nullopt, /*Iterator=*/std::move(iterator),
+              /*LocatorList=*/makeObjects(t3, semaCtx)}};
 }
 
 // Match: incomplete
diff --git a/flang/lib/Lower/OpenMP/Clauses.h b/flang/lib/Lower/OpenMP/Clauses.h
index 62f3df3e3ee952..1e911a20468575 100644
--- a/flang/lib/Lower/OpenMP/Clauses.h
+++ b/flang/lib/Lower/OpenMP/Clauses.h
@@ -9,6 +9,7 @@
 #define FORTRAN_LOWER_OPENMP_CLAUSES_H
 
 #include "flang/Evaluate/expression.h"
+#include "flang/Evaluate/type.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/expression.h"
 #include "flang/Semantics/semantics.h"
@@ -29,12 +30,7 @@ namespace Fortran::lower::omp {
 using namespace Fortran;
 using SomeExpr = semantics::SomeExpr;
 using MaybeExpr = semantics::MaybeExpr;
-
-// evaluate::SomeType doesn't provide == operation. It's not really used in
-// flang's clauses so far, so a trivial implementation is sufficient.
-struct TypeTy : public evaluate::SomeType {
-  bool operator==(const TypeTy &t) const { return true; }
-};
+using TypeTy = evaluate::DynamicType;
 
 template <typename ExprTy>
 struct IdTyTemplate {
@@ -150,6 +146,9 @@ std::optional<Object> getBaseObject(const Object &object,
                                     semantics::SemanticsContext &semaCtx);
 
 namespace clause {
+using Range = tomp::type::RangeT<ExprTy>;
+using Iterator = tomp::type::IteratorT<TypeTy, IdTy, ExprTy>;
+using IteratorSpecifier = tomp::type::IteratorSpecifierT<TypeTy, IdTy, ExprTy>;
 using DefinedOperator = tomp::type::DefinedOperatorT<IdTy, ExprTy>;
 using ProcedureDesignator = tomp::type::ProcedureDesignatorT<IdTy, ExprTy>;
 using ReductionOperator = tomp::type::ReductionIdentifierT<IdTy, ExprTy>;
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 52c7529369dfb5..26ee1653cc1d41 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -23,71 +23,162 @@ namespace Fortran::parser {
 constexpr auto startOmpLine = skipStuffBeforeStatement >> "!$OMP "_sptok;
 constexpr auto endOmpLine = space >> endOfLine;
 
-// Map modifiers come from two categories: map-type-modifier and map-type.
-// There can be zero or more map-type-modifiers, and zero or one map-type.
+// Helper class to deal with a list of modifiers of various types.
+// The list (to be parsed) is assumed to start with all modifiers of the
+// first type, followed by a list of modifiers of the second type, etc.
+// Each list can be empty, e.g.
+//   mod_of_kind_2, mod_of_kind_3, mod_of_kind_5, ...
+// The result type is a tuple of optional lists of each modifier type.
+template <typename Separator, typename Parser, typename... Parsers>
+struct ConcatSeparated {
+  template <typename P>
+  using OptListOf = std::optional<std::list<typename P::resultType>>;
+  template <typename P> using TupleFor = std::tuple<OptListOf<P>>;
+
+  using resultType = std::tuple<OptListOf<Parser>, OptListOf<Parsers>...>;
+
+  constexpr ConcatSeparated(ConcatSeparated &&) = default;
+  constexpr ConcatSeparated(const ConcatSeparated &) = default;
+  constexpr ConcatSeparated(Separator sep, Parser p, Parsers... ps)
+      : parser_(p), sepAndParsers_(sep, ps...) {}
+
+  std::optional<resultType> Parse(ParseState &state) const {
+    // firstParser is a list parser, it returns optional<list>.
+    auto firstParser =
+        attempt(nonemptySeparated(parser_, std::get<0>(sepAndParsers_)));
+
+    if constexpr (sizeof...(Parsers) == 0) {
+      return TupleFor<Parser>{std::move(firstParser.Parse(state))};
+    } else {
+      using restParserType = ConcatSeparated<Separator, Parsers...>;
+      auto restParser = std::make_from_tuple<restParserType>(sepAndParsers_);
+
+      if (auto first{firstParser.Parse(state)}) {
+        if (attempt(std::get<0>(sepAndParsers_)).Parse(state)) {
+          return std::tuple_cat(TupleFor<Parser>(std::move(*first)),
+                                std::move(*restParser.Parse(state)));
+        }
+        return std::tuple_cat(TupleFor<Parser>{std::move(*first)},
+                              std::tuple<OptListOf<Parsers>...>{});
+      }
+      return std::tuple_cat(TupleFor<Parser>{},
+                            std::move(*restParser.Parse(state)));
+    }
+  }
+
+private:
+  const Parser parser_;
+  const std::tuple<Separator, Parsers...> sepAndParsers_;
+};
+
+// Map modifiers come from four categories:
+// - map-type-modifier,
+// - mapper (not parsed yet),
+// - iterator,
+// - map-type.
+// There can be zero or more map-type-modifiers, and zero or one modifier
+// of every other kind.
 // Syntax-wise they look like a single list, where the last element could
 // be a map-type, and all elements in that list are comma-separated[1].
 // Only if there was at least one modifier (of any kind) specified, the
 // list must end with ":".
-// [1] Any of the commas are optional, but that syntax has been deprecated,
-// and the parsing code is intended to identify that. There are complications
-// coming from the fact that the comma separating the two kinds of modifiers
-// is only allowed if there is at least one modifier of each kind.
-// The MapModifiers parser parses the modifier list as a whole, and returns
-// a tuple with the (optional) map-type-modifier list, and the (optional)
-// type modifier as its members.
-// The list is then parsed, first with a mandatory separator, and if that
-// fails, with an optional one. If the latter succeeds, a deprecation
-// message is printed.
-template <typename Separator> struct MapModifiers {
-  constexpr MapModifiers(
-      Separator sep, std::optional<MessageFixedText> msg = std::nullopt)
-      : sep_(sep), msg_(msg) {}
+// There are complications coming from the fact that the comma separating the
+// two kinds of modifiers is only allowed if there is at least one modifier of
+// each kind. The MapModifiers parser utilizes the ConcatSeparated parser, which
+// takes care of that. ConcatSeparated returns a tuple with optional lists of
+// modifiers for every type.
+// [1] Any of the commas are optional, but that syntax has been deprecated
+// in OpenMP 5.2, and the parsing code keeps a record of whether the commas
+// were present.
+template <typename Separator>
+struct MapModifiers {
+  constexpr MapModifiers(Separator sep) : sep_(sep) {}
   constexpr MapModifiers(const MapModifiers &) = default;
   constexpr MapModifiers(MapModifiers &&) = default;
 
-  using resultType =
-      std::tuple<std::optional<std::list<OmpMapClause::TypeModifier>>,
-          std::optional<OmpMapClause::Type>>;
+  // Parsing of mappers is not supported yet.
+  using TypeModParser = Parser<OmpMapClause::TypeModifier>;
+  using IterParser = Parser<OmpIteratorModifier>;
+  using TypeParser = Parser<OmpMapClause::Type>;
+  using ModParser =
+      ConcatSeparated<Separator, TypeModParser, IterParser, TypeParser>;
+
+  using resultType = typename ModParser::resultType;
 
   std::optional<resultType> Parse(ParseState &state) const {
-    auto pmod{Parser<OmpMapClause::TypeModifier>{}};
-    auto ptype{Parser<OmpMapClause::Type>{}};
-    auto startLoc{state.GetLocation()};
-
-    auto &&[mods, type] = [&]() -> resultType {
-      // The 'maybe' will return optional<optional<list>>, and the outer
-      // optional will never be nullopt.
-      if (auto mods{
-              *maybe(attempt(nonemptySeparated(pmod, sep_))).Parse(state)}) {
-        // mods = optional<list>, and the list is nonempty.
-        return attempt(sep_).Parse(state)
-            ? resultType(mods, *maybe(attempt(ptype)).Parse(state))
-            : resultType(mods, std::nullopt);
+    auto mp = ModParser(sep_, TypeModParser{}, IterParser{}, TypeParser{});
+    auto mods = mp.Parse(state);
+    // The ModParser always "succeeds", i.e. even if the input is junk, it
+    // will return a tuple filled with nullopts. If any of the components
+    // is not a nullopt, expect a ":".
+    if (std::apply([](auto &&...opt...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Oct 21, 2024

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

Author: Krzysztof Parzyszek (kparzysz)

Changes

Define OmpIteratorSpecifier and OmpIteratorModifier parser classes, and add parsing for them. Those are reusable between any clauses that use iterator modifiers.

Add support for iterator modifiers to the MAP clause up to lowering, where a TODO message is emitted.


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

17 Files Affected:

  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+2)
  • (modified) flang/include/flang/Parser/parse-tree.h (+25-6)
  • (modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+56-49)
  • (modified) flang/lib/Lower/OpenMP/Clauses.cpp (+63-9)
  • (modified) flang/lib/Lower/OpenMP/Clauses.h (+5-6)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+154-60)
  • (modified) flang/lib/Parser/type-parsers.h (+1)
  • (modified) flang/lib/Parser/unparse.cpp (+41-14)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+97-4)
  • (modified) flang/lib/Semantics/check-omp-structure.h (+25)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+5-2)
  • (modified) flang/lib/Semantics/resolve-names.cpp (+35)
  • (modified) flang/test/Parser/OpenMP/map-modifiers.f90 (+190-8)
  • (modified) flang/test/Semantics/OpenMP/map-modifiers.f90 (+75-3)
  • (modified) flang/test/Semantics/OpenMP/symbol08.f90 (+9-9)
  • (modified) llvm/include/llvm/Frontend/OpenMP/OMP.h (+3)
  • (modified) llvm/lib/Frontend/OpenMP/OMP.cpp (+14)
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index ccbe5475d051e0..040065c0cbc029 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -474,6 +474,8 @@ class ParseTreeDumper {
   NODE(parser, NullInit)
   NODE(parser, ObjectDecl)
   NODE(parser, OldParameterStmt)
+  NODE(parser, OmpIteratorSpecifier)
+  NODE(parser, OmpIteratorModifier)
   NODE(parser, OmpAlignedClause)
   NODE(parser, OmpAtomic)
   NODE(parser, OmpAtomicCapture)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 4a3c992c4ec51b..35821288699fdb 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3424,7 +3424,17 @@ struct AssignedGotoStmt {
 
 WRAPPER_CLASS(PauseStmt, std::optional<StopCode>);
 
-// Parse tree nodes for OpenMP 4.5 directives and clauses
+// Parse tree nodes for OpenMP 4.5+ directives and clauses
+
+// [5.0] 2.1.6 iterator-specifier -> type-declaration-stmt = subscript-triple
+//             iterator-modifier -> iterator-specifier-list
+struct OmpIteratorSpecifier {
+  TUPLE_CLASS_BOILERPLATE(OmpIteratorSpecifier);
+  CharBlock source;
+  std::tuple<TypeDeclarationStmt, SubscriptTriplet> t;
+};
+
+WRAPPER_CLASS(OmpIteratorModifier, std::list<OmpIteratorSpecifier>);
 
 // 2.5 proc-bind-clause -> PROC_BIND (MASTER | CLOSE | SPREAD)
 struct OmpProcBindClause {
@@ -3450,16 +3460,25 @@ struct OmpObject {
 WRAPPER_CLASS(OmpObjectList, std::list<OmpObject>);
 
 // 2.15.5.1 map ->
-//    MAP ([ [map-type-modifiers [,] ] map-type : ] variable-name-list)
-// map-type-modifiers -> map-type-modifier [,] [...]
+//    MAP ([[map-type-modifier-list [,]] [iterator-modifier [,]] map-type : ]
+//         variable-name-list)
+// map-type-modifier-list -> map-type-modifier [,] [...]
 // map-type-modifier -> ALWAYS | CLOSE | PRESENT | OMPX_HOLD
 // map-type -> TO | FROM | TOFROM | ALLOC | RELEASE | DELETE
 struct OmpMapClause {
-  ENUM_CLASS(TypeModifier, Always, Close, Present, OmpxHold);
+  ENUM_CLASS(TypeModifier, Always, Close, Present, Ompx_Hold);
   ENUM_CLASS(Type, To, From, Tofrom, Alloc, Release, Delete)
   TUPLE_CLASS_BOILERPLATE(OmpMapClause);
-  std::tuple<std::optional<std::list<TypeModifier>>, std::optional<Type>,
-      OmpObjectList>
+
+  // All modifiers are parsed into optional lists, even if they are unique.
+  // The checks for satisfying those constraints are deferred to semantics.
+  // In OpenMP 5.2 the non-comma syntax has been deprecated: keep the
+  // information about separator presence to emit a diagnostic if needed.
+  std::tuple<std::optional<std::list<TypeModifier>>,
+             std::optional<std::list<OmpIteratorModifier>>, // unique
+             std::optional<std::list<Type>>,                // unique
+             OmpObjectList,
+             bool> // were the modifiers comma-separated
       t;
 };
 
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 88c443b4198ab0..fbc031f3a93d7d 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -936,57 +936,64 @@ bool ClauseProcessor::processMap(
            llvm::SmallVector<OmpMapMemberIndicesData>>
       parentMemberIndices;
 
-  bool clauseFound = findRepeatableClause<omp::clause::Map>(
-      [&](const omp::clause::Map &clause, const parser::CharBlock &source) {
-        using Map = omp::clause::Map;
-        mlir::Location clauseLocation = converter.genLocation(source);
-        const auto &mapType = std::get<std::optional<Map::MapType>>(clause.t);
-        llvm::omp::OpenMPOffloadMappingFlags mapTypeBits =
-            llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE;
-        // If the map type is specified, then process it else Tofrom is the
-        // default.
-        Map::MapType type = mapType.value_or(Map::MapType::Tofrom);
-        switch (type) {
-        case Map::MapType::To:
-          mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO;
-          break;
-        case Map::MapType::From:
-          mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
-          break;
-        case Map::MapType::Tofrom:
-          mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO |
-                         llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
-          break;
-        case Map::MapType::Alloc:
-        case Map::MapType::Release:
-          // alloc and release is the default map_type for the Target Data
-          // Ops, i.e. if no bits for map_type is supplied then alloc/release
-          // is implicitly assumed based on the target directive. Default
-          // value for Target Data and Enter Data is alloc and for Exit Data
-          // it is release.
-          break;
-        case Map::MapType::Delete:
-          mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_DELETE;
-        }
+  auto process = [&](const omp::clause::Map &clause,
+                     const parser::CharBlock &source) {
+    using Map = omp::clause::Map;
+    mlir::Location clauseLocation = converter.genLocation(source);
+    const auto &mapType = std::get<std::optional<Map::MapType>>(clause.t);
+    llvm::omp::OpenMPOffloadMappingFlags mapTypeBits =
+        llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE;
+    // If the map type is specified, then process it else Tofrom is the
+    // default.
+    Map::MapType type = mapType.value_or(Map::MapType::Tofrom);
+    switch (type) {
+    case Map::MapType::To:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO;
+      break;
+    case Map::MapType::From:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+      break;
+    case Map::MapType::Tofrom:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO |
+                     llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+      break;
+    case Map::MapType::Alloc:
+    case Map::MapType::Release:
+      // alloc and release is the default map_type for the Target Data
+      // Ops, i.e. if no bits for map_type is supplied then alloc/release
+      // is implicitly assumed based on the target directive. Default
+      // value for Target Data and Enter Data is alloc and for Exit Data
+      // it is release.
+      break;
+    case Map::MapType::Delete:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_DELETE;
+    }
 
-        auto &modTypeMods =
-            std::get<std::optional<Map::MapTypeModifiers>>(clause.t);
-        if (modTypeMods) {
-          if (llvm::is_contained(*modTypeMods, Map::MapTypeModifier::Always))
-            mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS;
-          // Diagnose unimplemented map-type-modifiers.
-          if (llvm::any_of(*modTypeMods, [](Map::MapTypeModifier m) {
-                return m != Map::MapTypeModifier::Always;
-              })) {
-            TODO(currentLocation, "Map type modifiers (other than 'ALWAYS')"
-                                  " are not implemented yet");
-          }
-        }
-        processMapObjects(stmtCtx, clauseLocation,
-                          std::get<omp::ObjectList>(clause.t), mapTypeBits,
-                          parentMemberIndices, result.mapVars, *ptrMapSyms);
-      });
+    auto &modTypeMods =
+        std::get<std::optional<Map::MapTypeModifiers>>(clause.t);
+    if (modTypeMods) {
+      if (llvm::is_contained(*modTypeMods, Map::MapTypeModifier::Always))
+        mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS;
+      // Diagnose unimplemented map-type-modifiers.
+      if (llvm::any_of(*modTypeMods, [](Map::MapTypeModifier m) {
+            return m != Map::MapTypeModifier::Always;
+          })) {
+        TODO(currentLocation, "Map type modifiers (other than 'ALWAYS')"
+                              " are not implemented yet");
+      }
+    }
+
+    if (std::get<std::optional<omp::clause::Iterator>>(clause.t)) {
+      TODO(currentLocation,
+           "Support for iterator modifiers is not implemented yet");
+    }
+
+    processMapObjects(stmtCtx, clauseLocation,
+                      std::get<omp::ObjectList>(clause.t), mapTypeBits,
+                      parentMemberIndices, result.mapVars, *ptrMapSyms);
+  };
 
+  bool clauseFound = findRepeatableClause<omp::clause::Map>(process);
   insertChildMapInfoIntoParent(converter, parentMemberIndices, result.mapVars,
                                *ptrMapSyms);
 
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index 64d661256a187e..8974b4211b9684 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -232,6 +232,46 @@ MAKE_INCOMPLETE_CLASS(Match, Match);
 // MAKE_INCOMPLETE_CLASS(Otherwise, );   // missing-in-parser
 MAKE_INCOMPLETE_CLASS(When, When);
 
+List<IteratorSpecifier>
+makeIteratorSpecifiers(const parser::OmpIteratorSpecifier &inp,
+                       semantics::SemanticsContext &semaCtx) {
+  List<IteratorSpecifier> specifiers;
+
+  auto &[begin, end, step] = std::get<parser::SubscriptTriplet>(inp.t).t;
+  assert(begin && end && "Expecting begin/end values");
+  evaluate::ExpressionAnalyzer ea{semaCtx};
+
+  MaybeExpr rbegin{ea.Analyze(*begin)}, rend{ea.Analyze(*end)};
+  MaybeExpr rstep;
+  if (step)
+    rstep = ea.Analyze(*step);
+
+  assert(rbegin && rend && "Unable to get range bounds");
+  Range range{{*rbegin, *rend, rstep}};
+
+  auto &tds = std::get<parser::TypeDeclarationStmt>(inp.t);
+  auto &entities = std::get<std::list<parser::EntityDecl>>(tds.t);
+  for (const parser::EntityDecl &ed : entities) {
+    auto &name = std::get<parser::ObjectName>(ed.t);
+    assert(name.symbol && "Expecting symbol for iterator variable");
+    auto *stype = name.symbol->GetType();
+    assert(stype && "Expecting symbol type");
+    IteratorSpecifier spec{{evaluate::DynamicType::From(*stype),
+                            makeObject(name, semaCtx), range}};
+    specifiers.emplace_back(std::move(spec));
+  }
+
+  return specifiers;
+}
+
+Iterator makeIterator(const parser::OmpIteratorModifier &inp,
+                      semantics::SemanticsContext &semaCtx) {
+  Iterator iterator;
+  for (auto &&spec : inp.v)
+    llvm::append_range(iterator, makeIteratorSpecifiers(spec, semaCtx));
+  return iterator;
+}
+
 DefinedOperator makeDefinedOperator(const parser::DefinedOperator &inp,
                                     semantics::SemanticsContext &semaCtx) {
   CLAUSET_ENUM_CONVERT( //
@@ -843,18 +883,32 @@ Map make(const parser::OmpClause::Map &inp,
   CLAUSET_ENUM_CONVERT( //
       convert2, parser::OmpMapClause::TypeModifier, Map::MapTypeModifier,
       // clang-format off
-      MS(Always,   Always)
-      MS(Close,    Close)
-      MS(OmpxHold, OmpxHold)
-      MS(Present,  Present)
+      MS(Always,    Always)
+      MS(Close,     Close)
+      MS(Ompx_Hold, OmpxHold)
+      MS(Present,   Present)
       // clang-format on
   );
 
   auto &t0 = std::get<std::optional<std::list<wrapped::TypeModifier>>>(inp.v.t);
-  auto &t1 = std::get<std::optional<wrapped::Type>>(inp.v.t);
-  auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
+  auto &t1 =
+      std::get<std::optional<std::list<parser::OmpIteratorModifier>>>(inp.v.t);
+  auto &t2 = std::get<std::optional<std::list<wrapped::Type>>>(inp.v.t);
+  auto &t3 = std::get<parser::OmpObjectList>(inp.v.t);
+
+  // These should have been diagnosed already.
+  assert((!t1 || t1->size() == 1) && "Only one iterator modifier is allowed");
+  assert((!t2 || t2->size() == 1) && "Only one map type is allowed");
+
+  auto iterator = [&]() -> std::optional<Iterator> {
+    if (t1)
+      return makeIterator(t1->front(), semaCtx);
+    return std::nullopt;
+  }();
 
-  std::optional<Map::MapType> maybeType = maybeApply(convert1, t1);
+  std::optional<Map::MapType> maybeType;
+  if (t2)
+    maybeType = maybeApply(convert1, std::optional<wrapped::Type>(t2->front()));
 
   std::optional<Map::MapTypeModifiers> maybeTypeMods = maybeApply(
       [&](const std::list<wrapped::TypeModifier> &typeMods) {
@@ -867,8 +921,8 @@ Map make(const parser::OmpClause::Map &inp,
 
   return Map{{/*MapType=*/maybeType,
               /*MapTypeModifiers=*/maybeTypeMods,
-              /*Mapper=*/std::nullopt, /*Iterator=*/std::nullopt,
-              /*LocatorList=*/makeObjects(t2, semaCtx)}};
+              /*Mapper=*/std::nullopt, /*Iterator=*/std::move(iterator),
+              /*LocatorList=*/makeObjects(t3, semaCtx)}};
 }
 
 // Match: incomplete
diff --git a/flang/lib/Lower/OpenMP/Clauses.h b/flang/lib/Lower/OpenMP/Clauses.h
index 62f3df3e3ee952..1e911a20468575 100644
--- a/flang/lib/Lower/OpenMP/Clauses.h
+++ b/flang/lib/Lower/OpenMP/Clauses.h
@@ -9,6 +9,7 @@
 #define FORTRAN_LOWER_OPENMP_CLAUSES_H
 
 #include "flang/Evaluate/expression.h"
+#include "flang/Evaluate/type.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/expression.h"
 #include "flang/Semantics/semantics.h"
@@ -29,12 +30,7 @@ namespace Fortran::lower::omp {
 using namespace Fortran;
 using SomeExpr = semantics::SomeExpr;
 using MaybeExpr = semantics::MaybeExpr;
-
-// evaluate::SomeType doesn't provide == operation. It's not really used in
-// flang's clauses so far, so a trivial implementation is sufficient.
-struct TypeTy : public evaluate::SomeType {
-  bool operator==(const TypeTy &t) const { return true; }
-};
+using TypeTy = evaluate::DynamicType;
 
 template <typename ExprTy>
 struct IdTyTemplate {
@@ -150,6 +146,9 @@ std::optional<Object> getBaseObject(const Object &object,
                                     semantics::SemanticsContext &semaCtx);
 
 namespace clause {
+using Range = tomp::type::RangeT<ExprTy>;
+using Iterator = tomp::type::IteratorT<TypeTy, IdTy, ExprTy>;
+using IteratorSpecifier = tomp::type::IteratorSpecifierT<TypeTy, IdTy, ExprTy>;
 using DefinedOperator = tomp::type::DefinedOperatorT<IdTy, ExprTy>;
 using ProcedureDesignator = tomp::type::ProcedureDesignatorT<IdTy, ExprTy>;
 using ReductionOperator = tomp::type::ReductionIdentifierT<IdTy, ExprTy>;
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 52c7529369dfb5..26ee1653cc1d41 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -23,71 +23,162 @@ namespace Fortran::parser {
 constexpr auto startOmpLine = skipStuffBeforeStatement >> "!$OMP "_sptok;
 constexpr auto endOmpLine = space >> endOfLine;
 
-// Map modifiers come from two categories: map-type-modifier and map-type.
-// There can be zero or more map-type-modifiers, and zero or one map-type.
+// Helper class to deal with a list of modifiers of various types.
+// The list (to be parsed) is assumed to start with all modifiers of the
+// first type, followed by a list of modifiers of the second type, etc.
+// Each list can be empty, e.g.
+//   mod_of_kind_2, mod_of_kind_3, mod_of_kind_5, ...
+// The result type is a tuple of optional lists of each modifier type.
+template <typename Separator, typename Parser, typename... Parsers>
+struct ConcatSeparated {
+  template <typename P>
+  using OptListOf = std::optional<std::list<typename P::resultType>>;
+  template <typename P> using TupleFor = std::tuple<OptListOf<P>>;
+
+  using resultType = std::tuple<OptListOf<Parser>, OptListOf<Parsers>...>;
+
+  constexpr ConcatSeparated(ConcatSeparated &&) = default;
+  constexpr ConcatSeparated(const ConcatSeparated &) = default;
+  constexpr ConcatSeparated(Separator sep, Parser p, Parsers... ps)
+      : parser_(p), sepAndParsers_(sep, ps...) {}
+
+  std::optional<resultType> Parse(ParseState &state) const {
+    // firstParser is a list parser, it returns optional<list>.
+    auto firstParser =
+        attempt(nonemptySeparated(parser_, std::get<0>(sepAndParsers_)));
+
+    if constexpr (sizeof...(Parsers) == 0) {
+      return TupleFor<Parser>{std::move(firstParser.Parse(state))};
+    } else {
+      using restParserType = ConcatSeparated<Separator, Parsers...>;
+      auto restParser = std::make_from_tuple<restParserType>(sepAndParsers_);
+
+      if (auto first{firstParser.Parse(state)}) {
+        if (attempt(std::get<0>(sepAndParsers_)).Parse(state)) {
+          return std::tuple_cat(TupleFor<Parser>(std::move(*first)),
+                                std::move(*restParser.Parse(state)));
+        }
+        return std::tuple_cat(TupleFor<Parser>{std::move(*first)},
+                              std::tuple<OptListOf<Parsers>...>{});
+      }
+      return std::tuple_cat(TupleFor<Parser>{},
+                            std::move(*restParser.Parse(state)));
+    }
+  }
+
+private:
+  const Parser parser_;
+  const std::tuple<Separator, Parsers...> sepAndParsers_;
+};
+
+// Map modifiers come from four categories:
+// - map-type-modifier,
+// - mapper (not parsed yet),
+// - iterator,
+// - map-type.
+// There can be zero or more map-type-modifiers, and zero or one modifier
+// of every other kind.
 // Syntax-wise they look like a single list, where the last element could
 // be a map-type, and all elements in that list are comma-separated[1].
 // Only if there was at least one modifier (of any kind) specified, the
 // list must end with ":".
-// [1] Any of the commas are optional, but that syntax has been deprecated,
-// and the parsing code is intended to identify that. There are complications
-// coming from the fact that the comma separating the two kinds of modifiers
-// is only allowed if there is at least one modifier of each kind.
-// The MapModifiers parser parses the modifier list as a whole, and returns
-// a tuple with the (optional) map-type-modifier list, and the (optional)
-// type modifier as its members.
-// The list is then parsed, first with a mandatory separator, and if that
-// fails, with an optional one. If the latter succeeds, a deprecation
-// message is printed.
-template <typename Separator> struct MapModifiers {
-  constexpr MapModifiers(
-      Separator sep, std::optional<MessageFixedText> msg = std::nullopt)
-      : sep_(sep), msg_(msg) {}
+// There are complications coming from the fact that the comma separating the
+// two kinds of modifiers is only allowed if there is at least one modifier of
+// each kind. The MapModifiers parser utilizes the ConcatSeparated parser, which
+// takes care of that. ConcatSeparated returns a tuple with optional lists of
+// modifiers for every type.
+// [1] Any of the commas are optional, but that syntax has been deprecated
+// in OpenMP 5.2, and the parsing code keeps a record of whether the commas
+// were present.
+template <typename Separator>
+struct MapModifiers {
+  constexpr MapModifiers(Separator sep) : sep_(sep) {}
   constexpr MapModifiers(const MapModifiers &) = default;
   constexpr MapModifiers(MapModifiers &&) = default;
 
-  using resultType =
-      std::tuple<std::optional<std::list<OmpMapClause::TypeModifier>>,
-          std::optional<OmpMapClause::Type>>;
+  // Parsing of mappers is not supported yet.
+  using TypeModParser = Parser<OmpMapClause::TypeModifier>;
+  using IterParser = Parser<OmpIteratorModifier>;
+  using TypeParser = Parser<OmpMapClause::Type>;
+  using ModParser =
+      ConcatSeparated<Separator, TypeModParser, IterParser, TypeParser>;
+
+  using resultType = typename ModParser::resultType;
 
   std::optional<resultType> Parse(ParseState &state) const {
-    auto pmod{Parser<OmpMapClause::TypeModifier>{}};
-    auto ptype{Parser<OmpMapClause::Type>{}};
-    auto startLoc{state.GetLocation()};
-
-    auto &&[mods, type] = [&]() -> resultType {
-      // The 'maybe' will return optional<optional<list>>, and the outer
-      // optional will never be nullopt.
-      if (auto mods{
-              *maybe(attempt(nonemptySeparated(pmod, sep_))).Parse(state)}) {
-        // mods = optional<list>, and the list is nonempty.
-        return attempt(sep_).Parse(state)
-            ? resultType(mods, *maybe(attempt(ptype)).Parse(state))
-            : resultType(mods, std::nullopt);
+    auto mp = ModParser(sep_, TypeModParser{}, IterParser{}, TypeParser{});
+    auto mods = mp.Parse(state);
+    // The ModParser always "succeeds", i.e. even if the input is junk, it
+    // will return a tuple filled with nullopts. If any of the components
+    // is not a nullopt, expect a ":".
+    if (std::apply([](auto &&...opt...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Oct 21, 2024

@llvm/pr-subscribers-flang-semantics

Author: Krzysztof Parzyszek (kparzysz)

Changes

Define OmpIteratorSpecifier and OmpIteratorModifier parser classes, and add parsing for them. Those are reusable between any clauses that use iterator modifiers.

Add support for iterator modifiers to the MAP clause up to lowering, where a TODO message is emitted.


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

17 Files Affected:

  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+2)
  • (modified) flang/include/flang/Parser/parse-tree.h (+25-6)
  • (modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+56-49)
  • (modified) flang/lib/Lower/OpenMP/Clauses.cpp (+63-9)
  • (modified) flang/lib/Lower/OpenMP/Clauses.h (+5-6)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+154-60)
  • (modified) flang/lib/Parser/type-parsers.h (+1)
  • (modified) flang/lib/Parser/unparse.cpp (+41-14)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+97-4)
  • (modified) flang/lib/Semantics/check-omp-structure.h (+25)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+5-2)
  • (modified) flang/lib/Semantics/resolve-names.cpp (+35)
  • (modified) flang/test/Parser/OpenMP/map-modifiers.f90 (+190-8)
  • (modified) flang/test/Semantics/OpenMP/map-modifiers.f90 (+75-3)
  • (modified) flang/test/Semantics/OpenMP/symbol08.f90 (+9-9)
  • (modified) llvm/include/llvm/Frontend/OpenMP/OMP.h (+3)
  • (modified) llvm/lib/Frontend/OpenMP/OMP.cpp (+14)
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index ccbe5475d051e0..040065c0cbc029 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -474,6 +474,8 @@ class ParseTreeDumper {
   NODE(parser, NullInit)
   NODE(parser, ObjectDecl)
   NODE(parser, OldParameterStmt)
+  NODE(parser, OmpIteratorSpecifier)
+  NODE(parser, OmpIteratorModifier)
   NODE(parser, OmpAlignedClause)
   NODE(parser, OmpAtomic)
   NODE(parser, OmpAtomicCapture)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 4a3c992c4ec51b..35821288699fdb 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3424,7 +3424,17 @@ struct AssignedGotoStmt {
 
 WRAPPER_CLASS(PauseStmt, std::optional<StopCode>);
 
-// Parse tree nodes for OpenMP 4.5 directives and clauses
+// Parse tree nodes for OpenMP 4.5+ directives and clauses
+
+// [5.0] 2.1.6 iterator-specifier -> type-declaration-stmt = subscript-triple
+//             iterator-modifier -> iterator-specifier-list
+struct OmpIteratorSpecifier {
+  TUPLE_CLASS_BOILERPLATE(OmpIteratorSpecifier);
+  CharBlock source;
+  std::tuple<TypeDeclarationStmt, SubscriptTriplet> t;
+};
+
+WRAPPER_CLASS(OmpIteratorModifier, std::list<OmpIteratorSpecifier>);
 
 // 2.5 proc-bind-clause -> PROC_BIND (MASTER | CLOSE | SPREAD)
 struct OmpProcBindClause {
@@ -3450,16 +3460,25 @@ struct OmpObject {
 WRAPPER_CLASS(OmpObjectList, std::list<OmpObject>);
 
 // 2.15.5.1 map ->
-//    MAP ([ [map-type-modifiers [,] ] map-type : ] variable-name-list)
-// map-type-modifiers -> map-type-modifier [,] [...]
+//    MAP ([[map-type-modifier-list [,]] [iterator-modifier [,]] map-type : ]
+//         variable-name-list)
+// map-type-modifier-list -> map-type-modifier [,] [...]
 // map-type-modifier -> ALWAYS | CLOSE | PRESENT | OMPX_HOLD
 // map-type -> TO | FROM | TOFROM | ALLOC | RELEASE | DELETE
 struct OmpMapClause {
-  ENUM_CLASS(TypeModifier, Always, Close, Present, OmpxHold);
+  ENUM_CLASS(TypeModifier, Always, Close, Present, Ompx_Hold);
   ENUM_CLASS(Type, To, From, Tofrom, Alloc, Release, Delete)
   TUPLE_CLASS_BOILERPLATE(OmpMapClause);
-  std::tuple<std::optional<std::list<TypeModifier>>, std::optional<Type>,
-      OmpObjectList>
+
+  // All modifiers are parsed into optional lists, even if they are unique.
+  // The checks for satisfying those constraints are deferred to semantics.
+  // In OpenMP 5.2 the non-comma syntax has been deprecated: keep the
+  // information about separator presence to emit a diagnostic if needed.
+  std::tuple<std::optional<std::list<TypeModifier>>,
+             std::optional<std::list<OmpIteratorModifier>>, // unique
+             std::optional<std::list<Type>>,                // unique
+             OmpObjectList,
+             bool> // were the modifiers comma-separated
       t;
 };
 
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 88c443b4198ab0..fbc031f3a93d7d 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -936,57 +936,64 @@ bool ClauseProcessor::processMap(
            llvm::SmallVector<OmpMapMemberIndicesData>>
       parentMemberIndices;
 
-  bool clauseFound = findRepeatableClause<omp::clause::Map>(
-      [&](const omp::clause::Map &clause, const parser::CharBlock &source) {
-        using Map = omp::clause::Map;
-        mlir::Location clauseLocation = converter.genLocation(source);
-        const auto &mapType = std::get<std::optional<Map::MapType>>(clause.t);
-        llvm::omp::OpenMPOffloadMappingFlags mapTypeBits =
-            llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE;
-        // If the map type is specified, then process it else Tofrom is the
-        // default.
-        Map::MapType type = mapType.value_or(Map::MapType::Tofrom);
-        switch (type) {
-        case Map::MapType::To:
-          mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO;
-          break;
-        case Map::MapType::From:
-          mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
-          break;
-        case Map::MapType::Tofrom:
-          mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO |
-                         llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
-          break;
-        case Map::MapType::Alloc:
-        case Map::MapType::Release:
-          // alloc and release is the default map_type for the Target Data
-          // Ops, i.e. if no bits for map_type is supplied then alloc/release
-          // is implicitly assumed based on the target directive. Default
-          // value for Target Data and Enter Data is alloc and for Exit Data
-          // it is release.
-          break;
-        case Map::MapType::Delete:
-          mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_DELETE;
-        }
+  auto process = [&](const omp::clause::Map &clause,
+                     const parser::CharBlock &source) {
+    using Map = omp::clause::Map;
+    mlir::Location clauseLocation = converter.genLocation(source);
+    const auto &mapType = std::get<std::optional<Map::MapType>>(clause.t);
+    llvm::omp::OpenMPOffloadMappingFlags mapTypeBits =
+        llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE;
+    // If the map type is specified, then process it else Tofrom is the
+    // default.
+    Map::MapType type = mapType.value_or(Map::MapType::Tofrom);
+    switch (type) {
+    case Map::MapType::To:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO;
+      break;
+    case Map::MapType::From:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+      break;
+    case Map::MapType::Tofrom:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO |
+                     llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+      break;
+    case Map::MapType::Alloc:
+    case Map::MapType::Release:
+      // alloc and release is the default map_type for the Target Data
+      // Ops, i.e. if no bits for map_type is supplied then alloc/release
+      // is implicitly assumed based on the target directive. Default
+      // value for Target Data and Enter Data is alloc and for Exit Data
+      // it is release.
+      break;
+    case Map::MapType::Delete:
+      mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_DELETE;
+    }
 
-        auto &modTypeMods =
-            std::get<std::optional<Map::MapTypeModifiers>>(clause.t);
-        if (modTypeMods) {
-          if (llvm::is_contained(*modTypeMods, Map::MapTypeModifier::Always))
-            mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS;
-          // Diagnose unimplemented map-type-modifiers.
-          if (llvm::any_of(*modTypeMods, [](Map::MapTypeModifier m) {
-                return m != Map::MapTypeModifier::Always;
-              })) {
-            TODO(currentLocation, "Map type modifiers (other than 'ALWAYS')"
-                                  " are not implemented yet");
-          }
-        }
-        processMapObjects(stmtCtx, clauseLocation,
-                          std::get<omp::ObjectList>(clause.t), mapTypeBits,
-                          parentMemberIndices, result.mapVars, *ptrMapSyms);
-      });
+    auto &modTypeMods =
+        std::get<std::optional<Map::MapTypeModifiers>>(clause.t);
+    if (modTypeMods) {
+      if (llvm::is_contained(*modTypeMods, Map::MapTypeModifier::Always))
+        mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS;
+      // Diagnose unimplemented map-type-modifiers.
+      if (llvm::any_of(*modTypeMods, [](Map::MapTypeModifier m) {
+            return m != Map::MapTypeModifier::Always;
+          })) {
+        TODO(currentLocation, "Map type modifiers (other than 'ALWAYS')"
+                              " are not implemented yet");
+      }
+    }
+
+    if (std::get<std::optional<omp::clause::Iterator>>(clause.t)) {
+      TODO(currentLocation,
+           "Support for iterator modifiers is not implemented yet");
+    }
+
+    processMapObjects(stmtCtx, clauseLocation,
+                      std::get<omp::ObjectList>(clause.t), mapTypeBits,
+                      parentMemberIndices, result.mapVars, *ptrMapSyms);
+  };
 
+  bool clauseFound = findRepeatableClause<omp::clause::Map>(process);
   insertChildMapInfoIntoParent(converter, parentMemberIndices, result.mapVars,
                                *ptrMapSyms);
 
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index 64d661256a187e..8974b4211b9684 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -232,6 +232,46 @@ MAKE_INCOMPLETE_CLASS(Match, Match);
 // MAKE_INCOMPLETE_CLASS(Otherwise, );   // missing-in-parser
 MAKE_INCOMPLETE_CLASS(When, When);
 
+List<IteratorSpecifier>
+makeIteratorSpecifiers(const parser::OmpIteratorSpecifier &inp,
+                       semantics::SemanticsContext &semaCtx) {
+  List<IteratorSpecifier> specifiers;
+
+  auto &[begin, end, step] = std::get<parser::SubscriptTriplet>(inp.t).t;
+  assert(begin && end && "Expecting begin/end values");
+  evaluate::ExpressionAnalyzer ea{semaCtx};
+
+  MaybeExpr rbegin{ea.Analyze(*begin)}, rend{ea.Analyze(*end)};
+  MaybeExpr rstep;
+  if (step)
+    rstep = ea.Analyze(*step);
+
+  assert(rbegin && rend && "Unable to get range bounds");
+  Range range{{*rbegin, *rend, rstep}};
+
+  auto &tds = std::get<parser::TypeDeclarationStmt>(inp.t);
+  auto &entities = std::get<std::list<parser::EntityDecl>>(tds.t);
+  for (const parser::EntityDecl &ed : entities) {
+    auto &name = std::get<parser::ObjectName>(ed.t);
+    assert(name.symbol && "Expecting symbol for iterator variable");
+    auto *stype = name.symbol->GetType();
+    assert(stype && "Expecting symbol type");
+    IteratorSpecifier spec{{evaluate::DynamicType::From(*stype),
+                            makeObject(name, semaCtx), range}};
+    specifiers.emplace_back(std::move(spec));
+  }
+
+  return specifiers;
+}
+
+Iterator makeIterator(const parser::OmpIteratorModifier &inp,
+                      semantics::SemanticsContext &semaCtx) {
+  Iterator iterator;
+  for (auto &&spec : inp.v)
+    llvm::append_range(iterator, makeIteratorSpecifiers(spec, semaCtx));
+  return iterator;
+}
+
 DefinedOperator makeDefinedOperator(const parser::DefinedOperator &inp,
                                     semantics::SemanticsContext &semaCtx) {
   CLAUSET_ENUM_CONVERT( //
@@ -843,18 +883,32 @@ Map make(const parser::OmpClause::Map &inp,
   CLAUSET_ENUM_CONVERT( //
       convert2, parser::OmpMapClause::TypeModifier, Map::MapTypeModifier,
       // clang-format off
-      MS(Always,   Always)
-      MS(Close,    Close)
-      MS(OmpxHold, OmpxHold)
-      MS(Present,  Present)
+      MS(Always,    Always)
+      MS(Close,     Close)
+      MS(Ompx_Hold, OmpxHold)
+      MS(Present,   Present)
       // clang-format on
   );
 
   auto &t0 = std::get<std::optional<std::list<wrapped::TypeModifier>>>(inp.v.t);
-  auto &t1 = std::get<std::optional<wrapped::Type>>(inp.v.t);
-  auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
+  auto &t1 =
+      std::get<std::optional<std::list<parser::OmpIteratorModifier>>>(inp.v.t);
+  auto &t2 = std::get<std::optional<std::list<wrapped::Type>>>(inp.v.t);
+  auto &t3 = std::get<parser::OmpObjectList>(inp.v.t);
+
+  // These should have been diagnosed already.
+  assert((!t1 || t1->size() == 1) && "Only one iterator modifier is allowed");
+  assert((!t2 || t2->size() == 1) && "Only one map type is allowed");
+
+  auto iterator = [&]() -> std::optional<Iterator> {
+    if (t1)
+      return makeIterator(t1->front(), semaCtx);
+    return std::nullopt;
+  }();
 
-  std::optional<Map::MapType> maybeType = maybeApply(convert1, t1);
+  std::optional<Map::MapType> maybeType;
+  if (t2)
+    maybeType = maybeApply(convert1, std::optional<wrapped::Type>(t2->front()));
 
   std::optional<Map::MapTypeModifiers> maybeTypeMods = maybeApply(
       [&](const std::list<wrapped::TypeModifier> &typeMods) {
@@ -867,8 +921,8 @@ Map make(const parser::OmpClause::Map &inp,
 
   return Map{{/*MapType=*/maybeType,
               /*MapTypeModifiers=*/maybeTypeMods,
-              /*Mapper=*/std::nullopt, /*Iterator=*/std::nullopt,
-              /*LocatorList=*/makeObjects(t2, semaCtx)}};
+              /*Mapper=*/std::nullopt, /*Iterator=*/std::move(iterator),
+              /*LocatorList=*/makeObjects(t3, semaCtx)}};
 }
 
 // Match: incomplete
diff --git a/flang/lib/Lower/OpenMP/Clauses.h b/flang/lib/Lower/OpenMP/Clauses.h
index 62f3df3e3ee952..1e911a20468575 100644
--- a/flang/lib/Lower/OpenMP/Clauses.h
+++ b/flang/lib/Lower/OpenMP/Clauses.h
@@ -9,6 +9,7 @@
 #define FORTRAN_LOWER_OPENMP_CLAUSES_H
 
 #include "flang/Evaluate/expression.h"
+#include "flang/Evaluate/type.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/expression.h"
 #include "flang/Semantics/semantics.h"
@@ -29,12 +30,7 @@ namespace Fortran::lower::omp {
 using namespace Fortran;
 using SomeExpr = semantics::SomeExpr;
 using MaybeExpr = semantics::MaybeExpr;
-
-// evaluate::SomeType doesn't provide == operation. It's not really used in
-// flang's clauses so far, so a trivial implementation is sufficient.
-struct TypeTy : public evaluate::SomeType {
-  bool operator==(const TypeTy &t) const { return true; }
-};
+using TypeTy = evaluate::DynamicType;
 
 template <typename ExprTy>
 struct IdTyTemplate {
@@ -150,6 +146,9 @@ std::optional<Object> getBaseObject(const Object &object,
                                     semantics::SemanticsContext &semaCtx);
 
 namespace clause {
+using Range = tomp::type::RangeT<ExprTy>;
+using Iterator = tomp::type::IteratorT<TypeTy, IdTy, ExprTy>;
+using IteratorSpecifier = tomp::type::IteratorSpecifierT<TypeTy, IdTy, ExprTy>;
 using DefinedOperator = tomp::type::DefinedOperatorT<IdTy, ExprTy>;
 using ProcedureDesignator = tomp::type::ProcedureDesignatorT<IdTy, ExprTy>;
 using ReductionOperator = tomp::type::ReductionIdentifierT<IdTy, ExprTy>;
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 52c7529369dfb5..26ee1653cc1d41 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -23,71 +23,162 @@ namespace Fortran::parser {
 constexpr auto startOmpLine = skipStuffBeforeStatement >> "!$OMP "_sptok;
 constexpr auto endOmpLine = space >> endOfLine;
 
-// Map modifiers come from two categories: map-type-modifier and map-type.
-// There can be zero or more map-type-modifiers, and zero or one map-type.
+// Helper class to deal with a list of modifiers of various types.
+// The list (to be parsed) is assumed to start with all modifiers of the
+// first type, followed by a list of modifiers of the second type, etc.
+// Each list can be empty, e.g.
+//   mod_of_kind_2, mod_of_kind_3, mod_of_kind_5, ...
+// The result type is a tuple of optional lists of each modifier type.
+template <typename Separator, typename Parser, typename... Parsers>
+struct ConcatSeparated {
+  template <typename P>
+  using OptListOf = std::optional<std::list<typename P::resultType>>;
+  template <typename P> using TupleFor = std::tuple<OptListOf<P>>;
+
+  using resultType = std::tuple<OptListOf<Parser>, OptListOf<Parsers>...>;
+
+  constexpr ConcatSeparated(ConcatSeparated &&) = default;
+  constexpr ConcatSeparated(const ConcatSeparated &) = default;
+  constexpr ConcatSeparated(Separator sep, Parser p, Parsers... ps)
+      : parser_(p), sepAndParsers_(sep, ps...) {}
+
+  std::optional<resultType> Parse(ParseState &state) const {
+    // firstParser is a list parser, it returns optional<list>.
+    auto firstParser =
+        attempt(nonemptySeparated(parser_, std::get<0>(sepAndParsers_)));
+
+    if constexpr (sizeof...(Parsers) == 0) {
+      return TupleFor<Parser>{std::move(firstParser.Parse(state))};
+    } else {
+      using restParserType = ConcatSeparated<Separator, Parsers...>;
+      auto restParser = std::make_from_tuple<restParserType>(sepAndParsers_);
+
+      if (auto first{firstParser.Parse(state)}) {
+        if (attempt(std::get<0>(sepAndParsers_)).Parse(state)) {
+          return std::tuple_cat(TupleFor<Parser>(std::move(*first)),
+                                std::move(*restParser.Parse(state)));
+        }
+        return std::tuple_cat(TupleFor<Parser>{std::move(*first)},
+                              std::tuple<OptListOf<Parsers>...>{});
+      }
+      return std::tuple_cat(TupleFor<Parser>{},
+                            std::move(*restParser.Parse(state)));
+    }
+  }
+
+private:
+  const Parser parser_;
+  const std::tuple<Separator, Parsers...> sepAndParsers_;
+};
+
+// Map modifiers come from four categories:
+// - map-type-modifier,
+// - mapper (not parsed yet),
+// - iterator,
+// - map-type.
+// There can be zero or more map-type-modifiers, and zero or one modifier
+// of every other kind.
 // Syntax-wise they look like a single list, where the last element could
 // be a map-type, and all elements in that list are comma-separated[1].
 // Only if there was at least one modifier (of any kind) specified, the
 // list must end with ":".
-// [1] Any of the commas are optional, but that syntax has been deprecated,
-// and the parsing code is intended to identify that. There are complications
-// coming from the fact that the comma separating the two kinds of modifiers
-// is only allowed if there is at least one modifier of each kind.
-// The MapModifiers parser parses the modifier list as a whole, and returns
-// a tuple with the (optional) map-type-modifier list, and the (optional)
-// type modifier as its members.
-// The list is then parsed, first with a mandatory separator, and if that
-// fails, with an optional one. If the latter succeeds, a deprecation
-// message is printed.
-template <typename Separator> struct MapModifiers {
-  constexpr MapModifiers(
-      Separator sep, std::optional<MessageFixedText> msg = std::nullopt)
-      : sep_(sep), msg_(msg) {}
+// There are complications coming from the fact that the comma separating the
+// two kinds of modifiers is only allowed if there is at least one modifier of
+// each kind. The MapModifiers parser utilizes the ConcatSeparated parser, which
+// takes care of that. ConcatSeparated returns a tuple with optional lists of
+// modifiers for every type.
+// [1] Any of the commas are optional, but that syntax has been deprecated
+// in OpenMP 5.2, and the parsing code keeps a record of whether the commas
+// were present.
+template <typename Separator>
+struct MapModifiers {
+  constexpr MapModifiers(Separator sep) : sep_(sep) {}
   constexpr MapModifiers(const MapModifiers &) = default;
   constexpr MapModifiers(MapModifiers &&) = default;
 
-  using resultType =
-      std::tuple<std::optional<std::list<OmpMapClause::TypeModifier>>,
-          std::optional<OmpMapClause::Type>>;
+  // Parsing of mappers is not supported yet.
+  using TypeModParser = Parser<OmpMapClause::TypeModifier>;
+  using IterParser = Parser<OmpIteratorModifier>;
+  using TypeParser = Parser<OmpMapClause::Type>;
+  using ModParser =
+      ConcatSeparated<Separator, TypeModParser, IterParser, TypeParser>;
+
+  using resultType = typename ModParser::resultType;
 
   std::optional<resultType> Parse(ParseState &state) const {
-    auto pmod{Parser<OmpMapClause::TypeModifier>{}};
-    auto ptype{Parser<OmpMapClause::Type>{}};
-    auto startLoc{state.GetLocation()};
-
-    auto &&[mods, type] = [&]() -> resultType {
-      // The 'maybe' will return optional<optional<list>>, and the outer
-      // optional will never be nullopt.
-      if (auto mods{
-              *maybe(attempt(nonemptySeparated(pmod, sep_))).Parse(state)}) {
-        // mods = optional<list>, and the list is nonempty.
-        return attempt(sep_).Parse(state)
-            ? resultType(mods, *maybe(attempt(ptype)).Parse(state))
-            : resultType(mods, std::nullopt);
+    auto mp = ModParser(sep_, TypeModParser{}, IterParser{}, TypeParser{});
+    auto mods = mp.Parse(state);
+    // The ModParser always "succeeds", i.e. even if the input is junk, it
+    // will return a tuple filled with nullopts. If any of the components
+    // is not a nullopt, expect a ":".
+    if (std::apply([](auto &&...opt...
[truncated]

Copy link

github-actions bot commented Oct 21, 2024

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

@kparzysz
Copy link
Contributor Author

One thing that will still need to change is the parsing of modifiers. Currently, if modifiers are present, they are expected to follow the order in which they are listed in the OpenMP spec. In reality, unless specified otherwise, the spec allows modifiers to be listed in any order. There will be another patch to follow that fixes this.

@kparzysz
Copy link
Contributor Author

The Windows build failures are unrelated to this PR (they are some pre-existing issues).

clang flags some issues with inline templates being undefined.
The problem may be legitimate, but in order to save time, the
template-lambda was removed altogether.
@kiranchandramohan
Copy link
Contributor

Nice work @kparzysz

@Leporacanthicus could you have a look at this patch? This patch also has an instance of a type in its syntax.

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.

A few nit comments to start with.

std::optional<std::list<OmpIteratorModifier>>, // unique
std::optional<std::list<Type>>, // unique
OmpObjectList,
bool> // were the modifiers comma-separated
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
bool> // were the modifiers comma-separated
bool> // where the modifiers comma-separated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually "Were they comma-separated or not?"

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah "were" is correct. Question mark would be good! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a ?

@@ -3424,7 +3424,17 @@ struct AssignedGotoStmt {

WRAPPER_CLASS(PauseStmt, std::optional<StopCode>);

// Parse tree nodes for OpenMP 4.5 directives and clauses
// Parse tree nodes for OpenMP 4.5+ directives and clauses
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a general update then may be saying OpenMP 5.2 is better.

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.

Comment on lines 196 to 208
bool canHaveIterator(Clause C) {
// [5.2:67:5]
switch (C) {
case OMPC_affinity:
case OMPC_depend:
case OMPC_from:
case OMPC_map:
case OMPC_to:
return true;
default:
return false;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this patch, but can this be specified in OMP.td and generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but that's a really specific check. The question is whether a clause modifier can introduce new symbols, and if so, what the scope is. Maybe this can be the generalized query, but I haven't looked into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Maybe define in the header - it's a pretty trivial function.

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.

@@ -1547,6 +1548,17 @@ class OmpVisitor : public virtual DeclarationVisitor {
void Post(const parser::OpenMPAtomicConstruct &) {
messageHandler().set_currStmtSource(std::nullopt);
}
bool Pre(const parser::OmpClause &x) {
if (NeedsScope(x)) {
PushScope(Scope::Kind::OtherConstruct, nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it work if we add OtherClause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introduced Scope::Kind::OtherClause and It seems to be working. The changes in symbol08.f90 are no longer needed.

Copy link
Contributor

@Leporacanthicus Leporacanthicus left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. I think this is good to go, modulo the minor comments from Kiran and me.

// map-type-modifier -> ALWAYS | CLOSE | PRESENT | OMPX_HOLD
// map-type -> TO | FROM | TOFROM | ALLOC | RELEASE | DELETE
struct OmpMapClause {
ENUM_CLASS(TypeModifier, Always, Close, Present, OmpxHold);
ENUM_CLASS(TypeModifier, Always, Close, Present, Ompx_Hold);
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking, I think this should be a separate commit (along with it's "friend" down below), as it is a minor fix for a previous change (from what I can tell).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The separate PR: #113366.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "friend" below is only used in this PR. PRs introducing code that is not executed are frowned upon, unless they are in a stack.

Comment on lines 196 to 208
bool canHaveIterator(Clause C) {
// [5.2:67:5]
switch (C) {
case OMPC_affinity:
case OMPC_depend:
case OMPC_from:
case OMPC_map:
case OMPC_to:
return true;
default:
return false;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Maybe define in the header - it's a pretty trivial function.

Copy link
Contributor

@Leporacanthicus Leporacanthicus left a comment

Choose a reason for hiding this comment

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

LGTM

@kparzysz kparzysz merged commit 973fa98 into main Oct 23, 2024
8 checks passed
@kparzysz kparzysz deleted the users/kparzysz/spr/i01-flang-iterator-modifier branch October 23, 2024 13:31
@kparzysz
Copy link
Contributor Author

Thanks everyone!

@frobtech frobtech mentioned this pull request Oct 25, 2024
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…llvm#113167)

Define `OmpIteratorSpecifier` and `OmpIteratorModifier` parser classes,
and add parsing for them. Those are reusable between any clauses that
use iterator modifiers.

Add support for iterator modifiers to the MAP clause up to lowering,
where a TODO message is emitted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:openmp OpenMP related changes to Clang flang:fir-hlfir flang:openmp flang:parser 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