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] Normalize clause modifiers that exist on their own #116655

Merged
merged 4 commits into from
Nov 20, 2024

Conversation

kparzysz
Copy link
Contributor

This is the first part of the effort to make parsing of clause modifiers more uniform and robust. Currently, when multiple modifiers are allowed, the parser will expect them to appear in a hard-coded order. Additionally, modifier properties (such as "ultimate") are checked separately for each case.

The overall plan is

  1. Extract all modifiers into their own top-level classes, and then equip them with sets of common properties that will allow performing the property checks generically, without refering to the specific kind of the modifier.
  2. Define a parser (as a separate class) for each modifier.
  3. For each clause define a union (std::variant) of all allowable modifiers, and parse the modifiers as a list of these unions.

The intent is also to isolate parts of the code that could eventually be auto-generated.

OpenMP modifier overhaul: #1/3

This is the first part of the effort to make parsing of clause modifiers
more uniform and robust. Currently, when multiple modifiers are allowed,
the parser will expect them to appear in a hard-coded order.
Additionally, modifier properties (such as "ultimate") are checked
separately for each case.

The overall plan is
1. Extract all modifiers into their own top-level classes, and then equip
them with sets of common properties that will allow performing the property
checks generically, without refering to the specific kind of the modifier.
2. Define a parser (as a separate class) for each modifier.
3. For each clause define a union (std::variant) of all allowable
modifiers, and parse the modifiers as a list of these unions.

The intent is also to isolate parts of the code that could eventually
be auto-generated.

OpenMP modifier overhaul: #1/3
@llvmbot
Copy link
Member

llvmbot commented Nov 18, 2024

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

@llvm/pr-subscribers-flang-parser

Author: Krzysztof Parzyszek (kparzysz)

Changes

This is the first part of the effort to make parsing of clause modifiers more uniform and robust. Currently, when multiple modifiers are allowed, the parser will expect them to appear in a hard-coded order. Additionally, modifier properties (such as "ultimate") are checked separately for each case.

The overall plan is

  1. Extract all modifiers into their own top-level classes, and then equip them with sets of common properties that will allow performing the property checks generically, without refering to the specific kind of the modifier.
  2. Define a parser (as a separate class) for each modifier.
  3. For each clause define a union (std::variant) of all allowable modifiers, and parse the modifiers as a list of these unions.

The intent is also to isolate parts of the code that could eventually be auto-generated.

OpenMP modifier overhaul: #1/3


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

16 Files Affected:

  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+5-5)
  • (modified) flang/include/flang/Parser/parse-tree.h (+70-43)
  • (modified) flang/lib/Lower/OpenMP/Clauses.cpp (+21-23)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+38-34)
  • (modified) flang/lib/Parser/parse-tree.cpp (+4-4)
  • (modified) flang/lib/Parser/unparse.cpp (+10-10)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+20-20)
  • (modified) flang/lib/Semantics/check-omp-structure.h (+3-3)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+2-2)
  • (modified) flang/test/Parser/OpenMP/affinity-clause.f90 (+1-1)
  • (modified) flang/test/Parser/OpenMP/depobj-construct.f90 (+2-2)
  • (modified) flang/test/Parser/OpenMP/from-clause.f90 (+2-2)
  • (modified) flang/test/Parser/OpenMP/in-reduction-clause.f90 (+3-3)
  • (modified) flang/test/Parser/OpenMP/map-modifiers.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/reduction-modifier.f90 (+1-1)
  • (modified) flang/test/Parser/OpenMP/target-update-to-clause.f90 (+2-2)
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index 5886e384b986b6..df5bf1d8d3200e 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -477,7 +477,7 @@ class ParseTreeDumper {
   NODE(parser, ObjectDecl)
   NODE(parser, OldParameterStmt)
   NODE(parser, OmpIteratorSpecifier)
-  NODE(parser, OmpIteratorModifier)
+  NODE(parser, OmpIterator)
   NODE(parser, OmpAffinityClause)
   NODE(parser, OmpAlignedClause)
   NODE(parser, OmpAtomic)
@@ -513,9 +513,9 @@ class ParseTreeDumper {
   NODE_ENUM(OmpDefaultmapClause, ImplicitBehavior)
   NODE_ENUM(OmpDefaultmapClause, VariableCategory)
   NODE(parser, OmpDependenceType)
-  NODE_ENUM(OmpDependenceType, Type)
+  NODE_ENUM(OmpDependenceType, Value)
   NODE(parser, OmpTaskDependenceType)
-  NODE_ENUM(OmpTaskDependenceType, Type)
+  NODE_ENUM(OmpTaskDependenceType, Value)
   NODE(parser, OmpIterationOffset)
   NODE(parser, OmpIteration)
   NODE(parser, OmpIterationVector)
@@ -543,7 +543,7 @@ class ParseTreeDumper {
   NODE(OmpLinearClause, WithModifier)
   NODE(OmpLinearClause, WithoutModifier)
   NODE(parser, OmpLinearModifier)
-  NODE_ENUM(OmpLinearModifier, Type)
+  NODE_ENUM(OmpLinearModifier, Value)
   NODE(parser, OmpLoopDirective)
   NODE(parser, OmpMapClause)
   NODE_ENUM(OmpMapClause, TypeModifier)
@@ -573,7 +573,7 @@ class ParseTreeDumper {
   NODE(parser, OmpReductionCombiner)
   NODE(OmpReductionCombiner, FunctionCombiner)
   NODE(parser, OmpReductionInitializerClause)
-  NODE(parser, OmpReductionOperator)
+  NODE(parser, OmpReductionIdentifier)
   NODE(parser, OmpAllocateClause)
   NODE(OmpAllocateClause, AllocateModifier)
   NODE(OmpAllocateClause::AllocateModifier, Allocator)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 8f50809599a589..ef49a36578270e 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3440,13 +3440,33 @@ struct OmpObject {
 
 WRAPPER_CLASS(OmpObjectList, std::list<OmpObject>);
 
+inline namespace modifier {
+// For uniformity, in all keyword modifiers the name of the type defined
+// by ENUM_CLASS is "Value", e.g.
+// struct Foo {
+//   ENUM_CLASS(Value, Keyword1, Keyword2);
+// };
+
+// Ref: [5.0:47-49], [5.1:49-51], [5.2:67-69]
+//
+// iterator-specifier ->
+//    [iterator-type] iterator-identifier
+//        = range-specification |                   // since 5.0
+//    [iterator-type ::] iterator-identifier
+//        = range-specification                     // since 5.2
+struct OmpIteratorSpecifier {
+  TUPLE_CLASS_BOILERPLATE(OmpIteratorSpecifier);
+  CharBlock source;
+  std::tuple<TypeDeclarationStmt, SubscriptTriplet> t;
+};
+
 // Ref: [4.5:169-170], [5.0:255-256], [5.1:288-289]
 //
 // dependence-type ->
-//    SINK | SOURCE |           // since 4.5
-//    IN | OUT | INOUT |        // since 4.5, until 5.1
-//    MUTEXINOUTSET | DEPOBJ |  // since 5.0, until 5.1
-//    INOUTSET                  // since 5.1, until 5.1
+//    SINK | SOURCE |                               // since 4.5
+//    IN | OUT | INOUT |                            // since 4.5, until 5.1
+//    MUTEXINOUTSET | DEPOBJ |                      // since 5.0, until 5.1
+//    INOUTSET                                      // since 5.1, until 5.1
 //
 // All of these, except SINK and SOURCE became task-dependence-type in 5.2.
 //
@@ -3457,45 +3477,59 @@ WRAPPER_CLASS(OmpObjectList, std::list<OmpObject>);
 // vector). This would accept the vector "i, j, k" (although interpreted
 // incorrectly), while flagging a syntax error for "i+1, j, k".
 struct OmpDependenceType {
-  ENUM_CLASS(Type, Sink, Source);
-  WRAPPER_CLASS_BOILERPLATE(OmpDependenceType, Type);
+  ENUM_CLASS(Value, Sink, Source);
+  WRAPPER_CLASS_BOILERPLATE(OmpDependenceType, Value);
 };
 
-// Ref: [4.5:169-170], [5.0:254-256], [5.1:287-289], [5.2:321]
+// Ref: [5.0:47-49], [5.1:49-51], [5.2:67-69]
 //
-// task-dependence-type -> // "dependence-type" in 5.1 and before
-//    IN | OUT | INOUT |        // since 4.5
-//    MUTEXINOUTSET | DEPOBJ |  // since 5.0
-//    INOUTSET                  // since 5.2
-struct OmpTaskDependenceType {
-  ENUM_CLASS(Type, In, Out, Inout, Inoutset, Mutexinoutset, Depobj)
-  WRAPPER_CLASS_BOILERPLATE(OmpTaskDependenceType, Type);
+// iterator-modifier ->
+//    ITERATOR(iterator-specifier [, ...])          // since 5.0
+struct OmpIterator {
+  WRAPPER_CLASS_BOILERPLATE(OmpIterator, std::list<OmpIteratorSpecifier>);
 };
 
-// [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;
+// Ref: [4.5:207-210], [5.0:290-293], [5.1:323-325], [5.2:117-120]
+//
+// linear-modifier ->
+//    REF | UVAL | VAL                              // since 4.5
+struct OmpLinearModifier {
+  ENUM_CLASS(Value, Ref, Uval, Val);
+  WRAPPER_CLASS_BOILERPLATE(OmpLinearModifier, Value);
 };
 
-WRAPPER_CLASS(OmpIteratorModifier, std::list<OmpIteratorSpecifier>);
-
-// 2.15.3.6 reduction-identifier -> + | - | * | .AND. | .OR. | .EQV. | .NEQV. |
-//                         MAX | MIN | IAND | IOR | IEOR
-struct OmpReductionOperator {
-  UNION_CLASS_BOILERPLATE(OmpReductionOperator);
+// Ref: [4.5:201-207], [5.0:293-299], [5.1:325-331], [5.2:124]
+//
+// reduction-identifier ->
+//   base-language-identifier |                     // since 4.5
+//   - |                                            // since 4.5, until 5.2
+//   + | * | .AND. | .OR. | .EQV. | .NEQV. |        // since 4.5
+//   MIN | MAX | IAND | IOR | IEOR                  // since 4.5
+//
+struct OmpReductionIdentifier {
+  UNION_CLASS_BOILERPLATE(OmpReductionIdentifier);
   std::variant<DefinedOperator, ProcedureDesignator> u;
 };
 
+// Ref: [4.5:169-170], [5.0:254-256], [5.1:287-289], [5.2:321]
+//
+// task-dependence-type -> // "dependence-type" in 5.1 and before
+//    IN | OUT | INOUT |                            // since 4.5
+//    MUTEXINOUTSET | DEPOBJ |                      // since 5.0
+//    INOUTSET                                      // since 5.2
+struct OmpTaskDependenceType {
+  ENUM_CLASS(Value, In, Out, Inout, Inoutset, Mutexinoutset, Depobj)
+  WRAPPER_CLASS_BOILERPLATE(OmpTaskDependenceType, Value);
+};
+} // namespace modifier
+
 // --- Clauses
 
 // OMP 5.0 2.10.1 affinity([aff-modifier:] locator-list)
 //                aff-modifier: interator-modifier
 struct OmpAffinityClause {
   TUPLE_CLASS_BOILERPLATE(OmpAffinityClause);
-  std::tuple<std::optional<OmpIteratorModifier>, OmpObjectList> t;
+  std::tuple<std::optional<OmpIterator>, OmpObjectList> t;
 };
 
 // 2.8.1 aligned-clause -> ALIGNED (variable-name-list[ : scalar-constant])
@@ -3566,7 +3600,7 @@ WRAPPER_CLASS(OmpIterationVector, std::list<OmpIteration>);
 // OmpDoacrossClause), so that the context in TYPE_CONTEXT_PARSER can be set
 // separately for OmpDependClause and OmpDoacrossClause.
 struct OmpDoacross {
-  OmpDependenceType::Type GetDepType() const;
+  OmpDependenceType::Value GetDepType() const;
 
   WRAPPER_CLASS(Sink, OmpIterationVector);
   EMPTY_CLASS(Source);
@@ -3586,10 +3620,9 @@ struct OmpDoacross {
 struct OmpDependClause {
   UNION_CLASS_BOILERPLATE(OmpDependClause);
   struct TaskDep {
-    OmpTaskDependenceType::Type GetTaskDepType() const;
+    OmpTaskDependenceType::Value GetTaskDepType() const;
     TUPLE_CLASS_BOILERPLATE(TaskDep);
-    std::tuple<std::optional<OmpIteratorModifier>, OmpTaskDependenceType,
-        OmpObjectList>
+    std::tuple<std::optional<OmpIterator>, OmpTaskDependenceType, OmpObjectList>
         t;
   };
   std::variant<TaskDep, OmpDoacross> u;
@@ -3632,7 +3665,7 @@ struct OmpFromClause {
   // As in the case of MAP, modifiers are parsed as lists, even if they
   // are unique. These restrictions will be checked in semantic checks.
   std::tuple<std::optional<std::list<Expectation>>,
-      std::optional<std::list<OmpIteratorModifier>>, OmpObjectList,
+      std::optional<std::list<OmpIterator>>, OmpObjectList,
       bool> // were the modifiers comma-separated?
       t;
 };
@@ -3661,7 +3694,7 @@ struct OmpDetachClause {
 //                                         variable-name-list)
 struct OmpInReductionClause {
   TUPLE_CLASS_BOILERPLATE(OmpInReductionClause);
-  std::tuple<OmpReductionOperator, OmpObjectList> t;
+  std::tuple<OmpReductionIdentifier, OmpObjectList> t;
 };
 
 // OMP 5.0 2.19.4.5 lastprivate-clause ->
@@ -3673,12 +3706,6 @@ struct OmpLastprivateClause {
   std::tuple<std::optional<LastprivateModifier>, OmpObjectList> t;
 };
 
-// 2.15.3.7 linear-modifier -> REF | VAL | UVAL
-struct OmpLinearModifier {
-  ENUM_CLASS(Type, Ref, Val, Uval)
-  WRAPPER_CLASS_BOILERPLATE(OmpLinearModifier, Type);
-};
-
 // 2.15.3.7 linear-clause -> LINEAR (linear-list[ : linear-step])
 //          linear-list -> list | linear-modifier(list)
 struct OmpLinearClause {
@@ -3719,7 +3746,7 @@ struct OmpMapClause {
   // 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<OmpIterator>>, // unique
       std::optional<std::list<Type>>, // unique
       OmpObjectList,
       bool> // were the modifiers comma-separated?
@@ -3749,7 +3776,7 @@ struct OmpProcBindClause {
 struct OmpReductionClause {
   TUPLE_CLASS_BOILERPLATE(OmpReductionClause);
   ENUM_CLASS(ReductionModifier, Inscan, Task, Default)
-  std::tuple<std::optional<ReductionModifier>, OmpReductionOperator,
+  std::tuple<std::optional<ReductionModifier>, OmpReductionIdentifier,
       OmpObjectList>
       t;
 };
@@ -3794,7 +3821,7 @@ struct OmpToClause {
   // As in the case of MAP, modifiers are parsed as lists, even if they
   // are unique. These restrictions will be checked in semantic checks.
   std::tuple<std::optional<std::list<Expectation>>,
-      std::optional<std::list<OmpIteratorModifier>>, OmpObjectList,
+      std::optional<std::list<OmpIterator>>, OmpObjectList,
       bool> // were the modifiers comma-separated?
       t;
 };
@@ -3942,7 +3969,7 @@ WRAPPER_CLASS(OmpReductionInitializerClause, Expr);
 struct OpenMPDeclareReductionConstruct {
   TUPLE_CLASS_BOILERPLATE(OpenMPDeclareReductionConstruct);
   CharBlock source;
-  std::tuple<Verbatim, OmpReductionOperator, std::list<DeclarationTypeSpec>,
+  std::tuple<Verbatim, OmpReductionIdentifier, std::list<DeclarationTypeSpec>,
       OmpReductionCombiner, std::optional<OmpReductionInitializerClause>>
       t;
 };
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index f897022ef9512b..5e514b4ba9f0da 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -264,7 +264,7 @@ makeIteratorSpecifiers(const parser::OmpIteratorSpecifier &inp,
   return specifiers;
 }
 
-Iterator makeIterator(const parser::OmpIteratorModifier &inp,
+Iterator makeIterator(const parser::OmpIterator &inp,
                       semantics::SemanticsContext &semaCtx) {
   Iterator iterator;
   for (auto &&spec : inp.v)
@@ -324,8 +324,9 @@ makeProcedureDesignator(const parser::ProcedureDesignator &inp,
       inp.u)};
 }
 
-ReductionOperator makeReductionOperator(const parser::OmpReductionOperator &inp,
-                                        semantics::SemanticsContext &semaCtx) {
+ReductionOperator
+makeReductionOperator(const parser::OmpReductionIdentifier &inp,
+                      semantics::SemanticsContext &semaCtx) {
   return Fortran::common::visit(
       common::visitors{
           [&](const parser::DefinedOperator &s) {
@@ -340,9 +341,9 @@ ReductionOperator makeReductionOperator(const parser::OmpReductionOperator &inp,
 
 clause::DependenceType makeDepType(const parser::OmpDependenceType &inp) {
   switch (inp.v) {
-  case parser::OmpDependenceType::Type::Sink:
+  case parser::OmpDependenceType::Value::Sink:
     return clause::DependenceType::Sink;
-  case parser::OmpDependenceType::Type::Source:
+  case parser::OmpDependenceType::Value::Source:
     return clause::DependenceType::Source;
   }
   llvm_unreachable("Unexpected dependence type");
@@ -350,17 +351,17 @@ clause::DependenceType makeDepType(const parser::OmpDependenceType &inp) {
 
 clause::DependenceType makeDepType(const parser::OmpTaskDependenceType &inp) {
   switch (inp.v) {
-  case parser::OmpTaskDependenceType::Type::Depobj:
+  case parser::OmpTaskDependenceType::Value::Depobj:
     return clause::DependenceType::Depobj;
-  case parser::OmpTaskDependenceType::Type::In:
+  case parser::OmpTaskDependenceType::Value::In:
     return clause::DependenceType::In;
-  case parser::OmpTaskDependenceType::Type::Inout:
+  case parser::OmpTaskDependenceType::Value::Inout:
     return clause::DependenceType::Inout;
-  case parser::OmpTaskDependenceType::Type::Inoutset:
+  case parser::OmpTaskDependenceType::Value::Inoutset:
     return clause::DependenceType::Inoutset;
-  case parser::OmpTaskDependenceType::Type::Mutexinoutset:
+  case parser::OmpTaskDependenceType::Value::Mutexinoutset:
     return clause::DependenceType::Mutexinoutset;
-  case parser::OmpTaskDependenceType::Type::Out:
+  case parser::OmpTaskDependenceType::Value::Out:
     return clause::DependenceType::Out;
   }
   llvm_unreachable("Unexpected task dependence type");
@@ -381,7 +382,7 @@ Absent make(const parser::OmpClause::Absent &inp,
 Affinity make(const parser::OmpClause::Affinity &inp,
               semantics::SemanticsContext &semaCtx) {
   // inp.v -> parser::OmpAffinityClause
-  auto &t0 = std::get<std::optional<parser::OmpIteratorModifier>>(inp.v.t);
+  auto &t0 = std::get<std::optional<parser::OmpIterator>>(inp.v.t);
   auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
 
   auto &&maybeIter =
@@ -626,7 +627,7 @@ Depend make(const parser::OmpClause::Depend &inp,
   using Variant = decltype(Depend::u);
 
   auto visitTaskDep = [&](const wrapped::TaskDep &s) -> Variant {
-    auto &t0 = std::get<std::optional<parser::OmpIteratorModifier>>(s.t);
+    auto &t0 = std::get<std::optional<parser::OmpIterator>>(s.t);
     auto &t1 = std::get<parser::OmpTaskDependenceType>(s.t);
     auto &t2 = std::get<parser::OmpObjectList>(s.t);
 
@@ -769,8 +770,7 @@ From make(const parser::OmpClause::From &inp,
   );
 
   auto &t0 = std::get<std::optional<std::list<wrapped::Expectation>>>(inp.v.t);
-  auto &t1 =
-      std::get<std::optional<std::list<parser::OmpIteratorModifier>>>(inp.v.t);
+  auto &t1 = std::get<std::optional<std::list<parser::OmpIterator>>>(inp.v.t);
   auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
 
   assert((!t0 || t0->size() == 1) && "Only one expectation modifier allowed");
@@ -881,7 +881,7 @@ Init make(const parser::OmpClause::Init &inp,
 InReduction make(const parser::OmpClause::InReduction &inp,
                  semantics::SemanticsContext &semaCtx) {
   // inp.v -> parser::OmpInReductionClause
-  auto &t0 = std::get<parser::OmpReductionOperator>(inp.v.t);
+  auto &t0 = std::get<parser::OmpReductionIdentifier>(inp.v.t);
   auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
   return InReduction{
       {/*ReductionIdentifiers=*/{makeReductionOperator(t0, semaCtx)},
@@ -920,7 +920,7 @@ Linear make(const parser::OmpClause::Linear &inp,
   using wrapped = parser::OmpLinearClause;
 
   CLAUSET_ENUM_CONVERT( //
-      convert, parser::OmpLinearModifier::Type, Linear::LinearModifier,
+      convert, parser::OmpLinearModifier::Value, Linear::LinearModifier,
       // clang-format off
       MS(Ref,  Ref)
       MS(Val,  Val)
@@ -984,8 +984,7 @@ Map make(const parser::OmpClause::Map &inp,
   );
 
   auto &t0 = std::get<std::optional<std::list<wrapped::TypeModifier>>>(inp.v.t);
-  auto &t1 =
-      std::get<std::optional<std::list<parser::OmpIteratorModifier>>>(inp.v.t);
+  auto &t1 = std::get<std::optional<std::list<parser::OmpIterator>>>(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);
 
@@ -1188,7 +1187,7 @@ Reduction make(const parser::OmpClause::Reduction &inp,
   auto &t0 =
       std::get<std::optional<parser::OmpReductionClause::ReductionModifier>>(
           inp.v.t);
-  auto &t1 = std::get<parser::OmpReductionOperator>(inp.v.t);
+  auto &t1 = std::get<parser::OmpReductionIdentifier>(inp.v.t);
   auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
   return Reduction{
       {/*ReductionModifier=*/t0
@@ -1315,7 +1314,7 @@ Permutation make(const parser::OmpClause::Permutation &inp,
 TaskReduction make(const parser::OmpClause::TaskReduction &inp,
                    semantics::SemanticsContext &semaCtx) {
   // inp.v -> parser::OmpReductionClause
-  auto &t0 = std::get<parser::OmpReductionOperator>(inp.v.t);
+  auto &t0 = std::get<parser::OmpReductionIdentifier>(inp.v.t);
   auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
   return TaskReduction{
       {/*ReductionIdentifiers=*/{makeReductionOperator(t0, semaCtx)},
@@ -1344,8 +1343,7 @@ To make(const parser::OmpClause::To &inp,
   );
 
   auto &t0 = std::get<std::optional<std::list<wrapped::Expectation>>>(inp.v.t);
-  auto &t1 =
-      std::get<std::optional<std::list<parser::OmpIteratorModifier>>>(inp.v.t);
+  auto &t1 = std::get<std::optional<std::list<parser::OmpIterator>>>(inp.v.t);
   auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
 
   assert((!t0 || t0->size() == 1) && "Only one expectation modifier allowed");
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 5ead9a48fa8969..b4d45873abb3ec 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -97,7 +97,7 @@ template <typename Separator> struct MapModifiers {
 
   // Parsing of mappers is not supported yet.
   using TypeModParser = Parser<OmpMapClause::TypeModifier>;
-  using IterParser = Parser<OmpIteratorModifier>;
+  using IterParser = Parser<OmpIterator>;
   using TypeParser = Parser<OmpMapClause::Type>;
   using ModParser =
       ConcatSeparated<Separator, TypeModParser, IterParser, TypeParser>;
@@ -133,7 +133,7 @@ template <typename Separator> struct MotionModifiers {
 
   // Parsing of mappers if not implemented yet.
   using ExpParser = Parser<OmpFromClause::Expectation>;
-  using IterParser = Parser<OmpIteratorModifier>;
+  using IterParser = Parser<OmpIterator>;
   using ModParser = ConcatSeparated<Separator, ExpParser, IterParser>;
 
   using resultType = typename ModParser::resultType;
@@ -191,6 +191,8 @@ static TypeDeclarationStmt makeIterSpecDecl(std::list<ObjectName> &&names) {
       makeEntityList(std::move(names)));
 }
 
+// --- Parsers for clause modifiers -----------------------------------
+
 TYPE_PARSER(construct<OmpIteratorSpecifier>(
     // Using Parser<TypeDeclarationStmt> or Parser<EntityDecl> has the problem
     // that they will attempt to treat what follows the '=' as initialization.
@@ -207,14 +209,40 @@ TYPE_PARSER(construct<OmpIteratorSpecifier>(
             makeIterSpecDecl, nonemptyList(Parser<ObjectName>{}) / "="_tok)),
     subscriptTriplet))
 
+TYPE_PARSER(construct<OmpDependenceType>(
+    "SINK" >> pure(OmpDependenceType::Value::Sink) ||
+    "SOURCE" >> pure(OmpDependenceType::Value::Source)))
+
 // [5.0] 2.1.6 iterator -> iterator-specifier-list
-TYPE_PARSER(construct<OmpIteratorModifier>("ITERATOR" >>
+TYPE_PARSER(construct<OmpIterator>("ITERATOR" >>
     parenthesized(nonemptyList(sourced(Parser<OmpIteratorSpecifier>{})))))
 
+// 2.15.3.7 LINEAR (linear-list: linear-step)
+//          linear-list -> list | modifier(list)
+//          linear-modifier -> REF | VAL | UVAL
+TYPE_PARSER(construct<OmpLinearModifier>( //
+    "REF" >> pure(OmpLinearModifier::Value::Ref) ||
+    "VAL" >> pure(OmpLinearModifier::Value::Val) ||
+    "UVAL" >> pure(OmpLinearModifier::Value::Uval)))
+
+// 2.15.3.6 REDUCTION (reduction-identifier: variable-name-list)
+TYPE_PARSER(construct<OmpReductionIdentifier>(Parser<DefinedOperator>{}) ||
+    construct<OmpReductionIdentifier>(Parser<ProcedureDesignator>{}))
+
+TYPE_PARSER(construct<OmpTaskDependenceType>(
+    "DEPOBJ" >> pure(OmpTaskDependenceType::Value::Depobj) ||
+    "IN"_id >> pure(OmpTaskDependenceType::Value::In) ||
+    "IN...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Nov 18, 2024

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

Author: Krzysztof Parzyszek (kparzysz)

Changes

This is the first part of the effort to make parsing of clause modifiers more uniform and robust. Currently, when multiple modifiers are allowed, the parser will expect them to appear in a hard-coded order. Additionally, modifier properties (such as "ultimate") are checked separately for each case.

The overall plan is

  1. Extract all modifiers into their own top-level classes, and then equip them with sets of common properties that will allow performing the property checks generically, without refering to the specific kind of the modifier.
  2. Define a parser (as a separate class) for each modifier.
  3. For each clause define a union (std::variant) of all allowable modifiers, and parse the modifiers as a list of these unions.

The intent is also to isolate parts of the code that could eventually be auto-generated.

OpenMP modifier overhaul: #1/3


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

16 Files Affected:

  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+5-5)
  • (modified) flang/include/flang/Parser/parse-tree.h (+70-43)
  • (modified) flang/lib/Lower/OpenMP/Clauses.cpp (+21-23)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+38-34)
  • (modified) flang/lib/Parser/parse-tree.cpp (+4-4)
  • (modified) flang/lib/Parser/unparse.cpp (+10-10)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+20-20)
  • (modified) flang/lib/Semantics/check-omp-structure.h (+3-3)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+2-2)
  • (modified) flang/test/Parser/OpenMP/affinity-clause.f90 (+1-1)
  • (modified) flang/test/Parser/OpenMP/depobj-construct.f90 (+2-2)
  • (modified) flang/test/Parser/OpenMP/from-clause.f90 (+2-2)
  • (modified) flang/test/Parser/OpenMP/in-reduction-clause.f90 (+3-3)
  • (modified) flang/test/Parser/OpenMP/map-modifiers.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/reduction-modifier.f90 (+1-1)
  • (modified) flang/test/Parser/OpenMP/target-update-to-clause.f90 (+2-2)
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index 5886e384b986b6..df5bf1d8d3200e 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -477,7 +477,7 @@ class ParseTreeDumper {
   NODE(parser, ObjectDecl)
   NODE(parser, OldParameterStmt)
   NODE(parser, OmpIteratorSpecifier)
-  NODE(parser, OmpIteratorModifier)
+  NODE(parser, OmpIterator)
   NODE(parser, OmpAffinityClause)
   NODE(parser, OmpAlignedClause)
   NODE(parser, OmpAtomic)
@@ -513,9 +513,9 @@ class ParseTreeDumper {
   NODE_ENUM(OmpDefaultmapClause, ImplicitBehavior)
   NODE_ENUM(OmpDefaultmapClause, VariableCategory)
   NODE(parser, OmpDependenceType)
-  NODE_ENUM(OmpDependenceType, Type)
+  NODE_ENUM(OmpDependenceType, Value)
   NODE(parser, OmpTaskDependenceType)
-  NODE_ENUM(OmpTaskDependenceType, Type)
+  NODE_ENUM(OmpTaskDependenceType, Value)
   NODE(parser, OmpIterationOffset)
   NODE(parser, OmpIteration)
   NODE(parser, OmpIterationVector)
@@ -543,7 +543,7 @@ class ParseTreeDumper {
   NODE(OmpLinearClause, WithModifier)
   NODE(OmpLinearClause, WithoutModifier)
   NODE(parser, OmpLinearModifier)
-  NODE_ENUM(OmpLinearModifier, Type)
+  NODE_ENUM(OmpLinearModifier, Value)
   NODE(parser, OmpLoopDirective)
   NODE(parser, OmpMapClause)
   NODE_ENUM(OmpMapClause, TypeModifier)
@@ -573,7 +573,7 @@ class ParseTreeDumper {
   NODE(parser, OmpReductionCombiner)
   NODE(OmpReductionCombiner, FunctionCombiner)
   NODE(parser, OmpReductionInitializerClause)
-  NODE(parser, OmpReductionOperator)
+  NODE(parser, OmpReductionIdentifier)
   NODE(parser, OmpAllocateClause)
   NODE(OmpAllocateClause, AllocateModifier)
   NODE(OmpAllocateClause::AllocateModifier, Allocator)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 8f50809599a589..ef49a36578270e 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3440,13 +3440,33 @@ struct OmpObject {
 
 WRAPPER_CLASS(OmpObjectList, std::list<OmpObject>);
 
+inline namespace modifier {
+// For uniformity, in all keyword modifiers the name of the type defined
+// by ENUM_CLASS is "Value", e.g.
+// struct Foo {
+//   ENUM_CLASS(Value, Keyword1, Keyword2);
+// };
+
+// Ref: [5.0:47-49], [5.1:49-51], [5.2:67-69]
+//
+// iterator-specifier ->
+//    [iterator-type] iterator-identifier
+//        = range-specification |                   // since 5.0
+//    [iterator-type ::] iterator-identifier
+//        = range-specification                     // since 5.2
+struct OmpIteratorSpecifier {
+  TUPLE_CLASS_BOILERPLATE(OmpIteratorSpecifier);
+  CharBlock source;
+  std::tuple<TypeDeclarationStmt, SubscriptTriplet> t;
+};
+
 // Ref: [4.5:169-170], [5.0:255-256], [5.1:288-289]
 //
 // dependence-type ->
-//    SINK | SOURCE |           // since 4.5
-//    IN | OUT | INOUT |        // since 4.5, until 5.1
-//    MUTEXINOUTSET | DEPOBJ |  // since 5.0, until 5.1
-//    INOUTSET                  // since 5.1, until 5.1
+//    SINK | SOURCE |                               // since 4.5
+//    IN | OUT | INOUT |                            // since 4.5, until 5.1
+//    MUTEXINOUTSET | DEPOBJ |                      // since 5.0, until 5.1
+//    INOUTSET                                      // since 5.1, until 5.1
 //
 // All of these, except SINK and SOURCE became task-dependence-type in 5.2.
 //
@@ -3457,45 +3477,59 @@ WRAPPER_CLASS(OmpObjectList, std::list<OmpObject>);
 // vector). This would accept the vector "i, j, k" (although interpreted
 // incorrectly), while flagging a syntax error for "i+1, j, k".
 struct OmpDependenceType {
-  ENUM_CLASS(Type, Sink, Source);
-  WRAPPER_CLASS_BOILERPLATE(OmpDependenceType, Type);
+  ENUM_CLASS(Value, Sink, Source);
+  WRAPPER_CLASS_BOILERPLATE(OmpDependenceType, Value);
 };
 
-// Ref: [4.5:169-170], [5.0:254-256], [5.1:287-289], [5.2:321]
+// Ref: [5.0:47-49], [5.1:49-51], [5.2:67-69]
 //
-// task-dependence-type -> // "dependence-type" in 5.1 and before
-//    IN | OUT | INOUT |        // since 4.5
-//    MUTEXINOUTSET | DEPOBJ |  // since 5.0
-//    INOUTSET                  // since 5.2
-struct OmpTaskDependenceType {
-  ENUM_CLASS(Type, In, Out, Inout, Inoutset, Mutexinoutset, Depobj)
-  WRAPPER_CLASS_BOILERPLATE(OmpTaskDependenceType, Type);
+// iterator-modifier ->
+//    ITERATOR(iterator-specifier [, ...])          // since 5.0
+struct OmpIterator {
+  WRAPPER_CLASS_BOILERPLATE(OmpIterator, std::list<OmpIteratorSpecifier>);
 };
 
-// [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;
+// Ref: [4.5:207-210], [5.0:290-293], [5.1:323-325], [5.2:117-120]
+//
+// linear-modifier ->
+//    REF | UVAL | VAL                              // since 4.5
+struct OmpLinearModifier {
+  ENUM_CLASS(Value, Ref, Uval, Val);
+  WRAPPER_CLASS_BOILERPLATE(OmpLinearModifier, Value);
 };
 
-WRAPPER_CLASS(OmpIteratorModifier, std::list<OmpIteratorSpecifier>);
-
-// 2.15.3.6 reduction-identifier -> + | - | * | .AND. | .OR. | .EQV. | .NEQV. |
-//                         MAX | MIN | IAND | IOR | IEOR
-struct OmpReductionOperator {
-  UNION_CLASS_BOILERPLATE(OmpReductionOperator);
+// Ref: [4.5:201-207], [5.0:293-299], [5.1:325-331], [5.2:124]
+//
+// reduction-identifier ->
+//   base-language-identifier |                     // since 4.5
+//   - |                                            // since 4.5, until 5.2
+//   + | * | .AND. | .OR. | .EQV. | .NEQV. |        // since 4.5
+//   MIN | MAX | IAND | IOR | IEOR                  // since 4.5
+//
+struct OmpReductionIdentifier {
+  UNION_CLASS_BOILERPLATE(OmpReductionIdentifier);
   std::variant<DefinedOperator, ProcedureDesignator> u;
 };
 
+// Ref: [4.5:169-170], [5.0:254-256], [5.1:287-289], [5.2:321]
+//
+// task-dependence-type -> // "dependence-type" in 5.1 and before
+//    IN | OUT | INOUT |                            // since 4.5
+//    MUTEXINOUTSET | DEPOBJ |                      // since 5.0
+//    INOUTSET                                      // since 5.2
+struct OmpTaskDependenceType {
+  ENUM_CLASS(Value, In, Out, Inout, Inoutset, Mutexinoutset, Depobj)
+  WRAPPER_CLASS_BOILERPLATE(OmpTaskDependenceType, Value);
+};
+} // namespace modifier
+
 // --- Clauses
 
 // OMP 5.0 2.10.1 affinity([aff-modifier:] locator-list)
 //                aff-modifier: interator-modifier
 struct OmpAffinityClause {
   TUPLE_CLASS_BOILERPLATE(OmpAffinityClause);
-  std::tuple<std::optional<OmpIteratorModifier>, OmpObjectList> t;
+  std::tuple<std::optional<OmpIterator>, OmpObjectList> t;
 };
 
 // 2.8.1 aligned-clause -> ALIGNED (variable-name-list[ : scalar-constant])
@@ -3566,7 +3600,7 @@ WRAPPER_CLASS(OmpIterationVector, std::list<OmpIteration>);
 // OmpDoacrossClause), so that the context in TYPE_CONTEXT_PARSER can be set
 // separately for OmpDependClause and OmpDoacrossClause.
 struct OmpDoacross {
-  OmpDependenceType::Type GetDepType() const;
+  OmpDependenceType::Value GetDepType() const;
 
   WRAPPER_CLASS(Sink, OmpIterationVector);
   EMPTY_CLASS(Source);
@@ -3586,10 +3620,9 @@ struct OmpDoacross {
 struct OmpDependClause {
   UNION_CLASS_BOILERPLATE(OmpDependClause);
   struct TaskDep {
-    OmpTaskDependenceType::Type GetTaskDepType() const;
+    OmpTaskDependenceType::Value GetTaskDepType() const;
     TUPLE_CLASS_BOILERPLATE(TaskDep);
-    std::tuple<std::optional<OmpIteratorModifier>, OmpTaskDependenceType,
-        OmpObjectList>
+    std::tuple<std::optional<OmpIterator>, OmpTaskDependenceType, OmpObjectList>
         t;
   };
   std::variant<TaskDep, OmpDoacross> u;
@@ -3632,7 +3665,7 @@ struct OmpFromClause {
   // As in the case of MAP, modifiers are parsed as lists, even if they
   // are unique. These restrictions will be checked in semantic checks.
   std::tuple<std::optional<std::list<Expectation>>,
-      std::optional<std::list<OmpIteratorModifier>>, OmpObjectList,
+      std::optional<std::list<OmpIterator>>, OmpObjectList,
       bool> // were the modifiers comma-separated?
       t;
 };
@@ -3661,7 +3694,7 @@ struct OmpDetachClause {
 //                                         variable-name-list)
 struct OmpInReductionClause {
   TUPLE_CLASS_BOILERPLATE(OmpInReductionClause);
-  std::tuple<OmpReductionOperator, OmpObjectList> t;
+  std::tuple<OmpReductionIdentifier, OmpObjectList> t;
 };
 
 // OMP 5.0 2.19.4.5 lastprivate-clause ->
@@ -3673,12 +3706,6 @@ struct OmpLastprivateClause {
   std::tuple<std::optional<LastprivateModifier>, OmpObjectList> t;
 };
 
-// 2.15.3.7 linear-modifier -> REF | VAL | UVAL
-struct OmpLinearModifier {
-  ENUM_CLASS(Type, Ref, Val, Uval)
-  WRAPPER_CLASS_BOILERPLATE(OmpLinearModifier, Type);
-};
-
 // 2.15.3.7 linear-clause -> LINEAR (linear-list[ : linear-step])
 //          linear-list -> list | linear-modifier(list)
 struct OmpLinearClause {
@@ -3719,7 +3746,7 @@ struct OmpMapClause {
   // 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<OmpIterator>>, // unique
       std::optional<std::list<Type>>, // unique
       OmpObjectList,
       bool> // were the modifiers comma-separated?
@@ -3749,7 +3776,7 @@ struct OmpProcBindClause {
 struct OmpReductionClause {
   TUPLE_CLASS_BOILERPLATE(OmpReductionClause);
   ENUM_CLASS(ReductionModifier, Inscan, Task, Default)
-  std::tuple<std::optional<ReductionModifier>, OmpReductionOperator,
+  std::tuple<std::optional<ReductionModifier>, OmpReductionIdentifier,
       OmpObjectList>
       t;
 };
@@ -3794,7 +3821,7 @@ struct OmpToClause {
   // As in the case of MAP, modifiers are parsed as lists, even if they
   // are unique. These restrictions will be checked in semantic checks.
   std::tuple<std::optional<std::list<Expectation>>,
-      std::optional<std::list<OmpIteratorModifier>>, OmpObjectList,
+      std::optional<std::list<OmpIterator>>, OmpObjectList,
       bool> // were the modifiers comma-separated?
       t;
 };
@@ -3942,7 +3969,7 @@ WRAPPER_CLASS(OmpReductionInitializerClause, Expr);
 struct OpenMPDeclareReductionConstruct {
   TUPLE_CLASS_BOILERPLATE(OpenMPDeclareReductionConstruct);
   CharBlock source;
-  std::tuple<Verbatim, OmpReductionOperator, std::list<DeclarationTypeSpec>,
+  std::tuple<Verbatim, OmpReductionIdentifier, std::list<DeclarationTypeSpec>,
       OmpReductionCombiner, std::optional<OmpReductionInitializerClause>>
       t;
 };
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index f897022ef9512b..5e514b4ba9f0da 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -264,7 +264,7 @@ makeIteratorSpecifiers(const parser::OmpIteratorSpecifier &inp,
   return specifiers;
 }
 
-Iterator makeIterator(const parser::OmpIteratorModifier &inp,
+Iterator makeIterator(const parser::OmpIterator &inp,
                       semantics::SemanticsContext &semaCtx) {
   Iterator iterator;
   for (auto &&spec : inp.v)
@@ -324,8 +324,9 @@ makeProcedureDesignator(const parser::ProcedureDesignator &inp,
       inp.u)};
 }
 
-ReductionOperator makeReductionOperator(const parser::OmpReductionOperator &inp,
-                                        semantics::SemanticsContext &semaCtx) {
+ReductionOperator
+makeReductionOperator(const parser::OmpReductionIdentifier &inp,
+                      semantics::SemanticsContext &semaCtx) {
   return Fortran::common::visit(
       common::visitors{
           [&](const parser::DefinedOperator &s) {
@@ -340,9 +341,9 @@ ReductionOperator makeReductionOperator(const parser::OmpReductionOperator &inp,
 
 clause::DependenceType makeDepType(const parser::OmpDependenceType &inp) {
   switch (inp.v) {
-  case parser::OmpDependenceType::Type::Sink:
+  case parser::OmpDependenceType::Value::Sink:
     return clause::DependenceType::Sink;
-  case parser::OmpDependenceType::Type::Source:
+  case parser::OmpDependenceType::Value::Source:
     return clause::DependenceType::Source;
   }
   llvm_unreachable("Unexpected dependence type");
@@ -350,17 +351,17 @@ clause::DependenceType makeDepType(const parser::OmpDependenceType &inp) {
 
 clause::DependenceType makeDepType(const parser::OmpTaskDependenceType &inp) {
   switch (inp.v) {
-  case parser::OmpTaskDependenceType::Type::Depobj:
+  case parser::OmpTaskDependenceType::Value::Depobj:
     return clause::DependenceType::Depobj;
-  case parser::OmpTaskDependenceType::Type::In:
+  case parser::OmpTaskDependenceType::Value::In:
     return clause::DependenceType::In;
-  case parser::OmpTaskDependenceType::Type::Inout:
+  case parser::OmpTaskDependenceType::Value::Inout:
     return clause::DependenceType::Inout;
-  case parser::OmpTaskDependenceType::Type::Inoutset:
+  case parser::OmpTaskDependenceType::Value::Inoutset:
     return clause::DependenceType::Inoutset;
-  case parser::OmpTaskDependenceType::Type::Mutexinoutset:
+  case parser::OmpTaskDependenceType::Value::Mutexinoutset:
     return clause::DependenceType::Mutexinoutset;
-  case parser::OmpTaskDependenceType::Type::Out:
+  case parser::OmpTaskDependenceType::Value::Out:
     return clause::DependenceType::Out;
   }
   llvm_unreachable("Unexpected task dependence type");
@@ -381,7 +382,7 @@ Absent make(const parser::OmpClause::Absent &inp,
 Affinity make(const parser::OmpClause::Affinity &inp,
               semantics::SemanticsContext &semaCtx) {
   // inp.v -> parser::OmpAffinityClause
-  auto &t0 = std::get<std::optional<parser::OmpIteratorModifier>>(inp.v.t);
+  auto &t0 = std::get<std::optional<parser::OmpIterator>>(inp.v.t);
   auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
 
   auto &&maybeIter =
@@ -626,7 +627,7 @@ Depend make(const parser::OmpClause::Depend &inp,
   using Variant = decltype(Depend::u);
 
   auto visitTaskDep = [&](const wrapped::TaskDep &s) -> Variant {
-    auto &t0 = std::get<std::optional<parser::OmpIteratorModifier>>(s.t);
+    auto &t0 = std::get<std::optional<parser::OmpIterator>>(s.t);
     auto &t1 = std::get<parser::OmpTaskDependenceType>(s.t);
     auto &t2 = std::get<parser::OmpObjectList>(s.t);
 
@@ -769,8 +770,7 @@ From make(const parser::OmpClause::From &inp,
   );
 
   auto &t0 = std::get<std::optional<std::list<wrapped::Expectation>>>(inp.v.t);
-  auto &t1 =
-      std::get<std::optional<std::list<parser::OmpIteratorModifier>>>(inp.v.t);
+  auto &t1 = std::get<std::optional<std::list<parser::OmpIterator>>>(inp.v.t);
   auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
 
   assert((!t0 || t0->size() == 1) && "Only one expectation modifier allowed");
@@ -881,7 +881,7 @@ Init make(const parser::OmpClause::Init &inp,
 InReduction make(const parser::OmpClause::InReduction &inp,
                  semantics::SemanticsContext &semaCtx) {
   // inp.v -> parser::OmpInReductionClause
-  auto &t0 = std::get<parser::OmpReductionOperator>(inp.v.t);
+  auto &t0 = std::get<parser::OmpReductionIdentifier>(inp.v.t);
   auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
   return InReduction{
       {/*ReductionIdentifiers=*/{makeReductionOperator(t0, semaCtx)},
@@ -920,7 +920,7 @@ Linear make(const parser::OmpClause::Linear &inp,
   using wrapped = parser::OmpLinearClause;
 
   CLAUSET_ENUM_CONVERT( //
-      convert, parser::OmpLinearModifier::Type, Linear::LinearModifier,
+      convert, parser::OmpLinearModifier::Value, Linear::LinearModifier,
       // clang-format off
       MS(Ref,  Ref)
       MS(Val,  Val)
@@ -984,8 +984,7 @@ Map make(const parser::OmpClause::Map &inp,
   );
 
   auto &t0 = std::get<std::optional<std::list<wrapped::TypeModifier>>>(inp.v.t);
-  auto &t1 =
-      std::get<std::optional<std::list<parser::OmpIteratorModifier>>>(inp.v.t);
+  auto &t1 = std::get<std::optional<std::list<parser::OmpIterator>>>(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);
 
@@ -1188,7 +1187,7 @@ Reduction make(const parser::OmpClause::Reduction &inp,
   auto &t0 =
       std::get<std::optional<parser::OmpReductionClause::ReductionModifier>>(
           inp.v.t);
-  auto &t1 = std::get<parser::OmpReductionOperator>(inp.v.t);
+  auto &t1 = std::get<parser::OmpReductionIdentifier>(inp.v.t);
   auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
   return Reduction{
       {/*ReductionModifier=*/t0
@@ -1315,7 +1314,7 @@ Permutation make(const parser::OmpClause::Permutation &inp,
 TaskReduction make(const parser::OmpClause::TaskReduction &inp,
                    semantics::SemanticsContext &semaCtx) {
   // inp.v -> parser::OmpReductionClause
-  auto &t0 = std::get<parser::OmpReductionOperator>(inp.v.t);
+  auto &t0 = std::get<parser::OmpReductionIdentifier>(inp.v.t);
   auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
   return TaskReduction{
       {/*ReductionIdentifiers=*/{makeReductionOperator(t0, semaCtx)},
@@ -1344,8 +1343,7 @@ To make(const parser::OmpClause::To &inp,
   );
 
   auto &t0 = std::get<std::optional<std::list<wrapped::Expectation>>>(inp.v.t);
-  auto &t1 =
-      std::get<std::optional<std::list<parser::OmpIteratorModifier>>>(inp.v.t);
+  auto &t1 = std::get<std::optional<std::list<parser::OmpIterator>>>(inp.v.t);
   auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
 
   assert((!t0 || t0->size() == 1) && "Only one expectation modifier allowed");
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 5ead9a48fa8969..b4d45873abb3ec 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -97,7 +97,7 @@ template <typename Separator> struct MapModifiers {
 
   // Parsing of mappers is not supported yet.
   using TypeModParser = Parser<OmpMapClause::TypeModifier>;
-  using IterParser = Parser<OmpIteratorModifier>;
+  using IterParser = Parser<OmpIterator>;
   using TypeParser = Parser<OmpMapClause::Type>;
   using ModParser =
       ConcatSeparated<Separator, TypeModParser, IterParser, TypeParser>;
@@ -133,7 +133,7 @@ template <typename Separator> struct MotionModifiers {
 
   // Parsing of mappers if not implemented yet.
   using ExpParser = Parser<OmpFromClause::Expectation>;
-  using IterParser = Parser<OmpIteratorModifier>;
+  using IterParser = Parser<OmpIterator>;
   using ModParser = ConcatSeparated<Separator, ExpParser, IterParser>;
 
   using resultType = typename ModParser::resultType;
@@ -191,6 +191,8 @@ static TypeDeclarationStmt makeIterSpecDecl(std::list<ObjectName> &&names) {
       makeEntityList(std::move(names)));
 }
 
+// --- Parsers for clause modifiers -----------------------------------
+
 TYPE_PARSER(construct<OmpIteratorSpecifier>(
     // Using Parser<TypeDeclarationStmt> or Parser<EntityDecl> has the problem
     // that they will attempt to treat what follows the '=' as initialization.
@@ -207,14 +209,40 @@ TYPE_PARSER(construct<OmpIteratorSpecifier>(
             makeIterSpecDecl, nonemptyList(Parser<ObjectName>{}) / "="_tok)),
     subscriptTriplet))
 
+TYPE_PARSER(construct<OmpDependenceType>(
+    "SINK" >> pure(OmpDependenceType::Value::Sink) ||
+    "SOURCE" >> pure(OmpDependenceType::Value::Source)))
+
 // [5.0] 2.1.6 iterator -> iterator-specifier-list
-TYPE_PARSER(construct<OmpIteratorModifier>("ITERATOR" >>
+TYPE_PARSER(construct<OmpIterator>("ITERATOR" >>
     parenthesized(nonemptyList(sourced(Parser<OmpIteratorSpecifier>{})))))
 
+// 2.15.3.7 LINEAR (linear-list: linear-step)
+//          linear-list -> list | modifier(list)
+//          linear-modifier -> REF | VAL | UVAL
+TYPE_PARSER(construct<OmpLinearModifier>( //
+    "REF" >> pure(OmpLinearModifier::Value::Ref) ||
+    "VAL" >> pure(OmpLinearModifier::Value::Val) ||
+    "UVAL" >> pure(OmpLinearModifier::Value::Uval)))
+
+// 2.15.3.6 REDUCTION (reduction-identifier: variable-name-list)
+TYPE_PARSER(construct<OmpReductionIdentifier>(Parser<DefinedOperator>{}) ||
+    construct<OmpReductionIdentifier>(Parser<ProcedureDesignator>{}))
+
+TYPE_PARSER(construct<OmpTaskDependenceType>(
+    "DEPOBJ" >> pure(OmpTaskDependenceType::Value::Depobj) ||
+    "IN"_id >> pure(OmpTaskDependenceType::Value::In) ||
+    "IN...
[truncated]

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.

Is this only for modifiers that can appear together or for all modifiers. What about ScheduleModifier?

@@ -218,11 +218,11 @@ void OpenMPCounterVisitor::Post(const OmpScheduleModifierType::ModType &c) {
clauseDetails +=
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: Is OmpReductionOperator missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the OmpReductionOperator to OmpReductionIdentifier to follow the naming from the spec.

@kparzysz
Copy link
Contributor Author

Eventually this is for all modifiers, but I want to do this in smaller steps. One extra goal I'm trying to accomplish is to make it easy to add the new directive-name-modifier.

At the end of all this, every modifier will be a top-level class (i.e. not defined inside of a clause). Then every clause will use the "variant" instead of putting modifiers explicitly in a tuple.

@kparzysz
Copy link
Contributor Author

kparzysz commented Nov 19, 2024

I'm actually working on the schedule clause now. Almost ready.

Edit: (to add mode context)
The schedule modifier in the AST has a somewhat complex structure, but with the 3 PRs in place, it's now possible to parse it directly into the "ordering-modifier" and the "chunk-modifier". So the top-level schedule modifier will go away completely.

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.

LGTM.

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup!

@kparzysz kparzysz merged commit cfd67c2 into main Nov 20, 2024
8 checks passed
@kparzysz kparzysz deleted the users/kparzysz/spr/m01-normalize-existing branch November 20, 2024 14:33
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