Skip to content

Conversation

@kparzysz
Copy link
Contributor

This actually simplifies the AST node for the schedule clause: the two allowed modifiers can be easily classified as the ordering-modifier and the chunk-modifier during parsing without the need to create additional classes.

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
The main issue to solve is that OpenMP modifiers can be specified
in any order, so the parser cannot expect any specific modifier at
a given position. To solve that, define modifier to be a union of
all allowable specific modifiers for a given clause.

Additionally, implement modifier descriptors: for each modifier the
corresponding descriptor contains a set of properties of the modifier
that allow a common set of semantic checks. Start with the syntactic
properties defined in the spec: Required, Unique, Exclusive, Ultimate,
and implement common checks to verify each of them.

OpenMP modifier overhaul: #2/3
Also, define helper macros in parse-tree.h.

Apply the new modifier representation to the DEFAULTMAP and REDUCTION
clauses, with testcases utilizing the new modifier validation.

OpenMP modifier overhaul: #3/3
This actually simplifies the AST node for the schedule clause: the two
allowed modifiers can be easily classified as the ordering-modifier and
the chunk-modifier during parsing without the need to create additional
classes.
@llvmbot
Copy link
Member

llvmbot commented Nov 21, 2024

@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-parser

Author: Krzysztof Parzyszek (kparzysz)

Changes

This actually simplifies the AST node for the schedule clause: the two allowed modifiers can be easily classified as the ordering-modifier and the chunk-modifier during parsing without the need to create additional classes.


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

14 Files Affected:

  • (modified) flang/examples/FeatureList/FeatureList.cpp (+6-7)
  • (modified) flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp (+7-3)
  • (modified) flang/examples/FlangOmpReport/FlangOmpReportVisitor.h (+2-1)
  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+9-8)
  • (modified) flang/include/flang/Parser/parse-tree.h (+54-27)
  • (modified) flang/include/flang/Semantics/openmp-modifiers.h (+6)
  • (modified) flang/lib/Lower/OpenMP/Clauses.cpp (+24-51)
  • (modified) flang/lib/Lower/OpenMP/Clauses.h (+10)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+38-33)
  • (modified) flang/lib/Parser/unparse.cpp (+11-12)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+23-35)
  • (modified) flang/lib/Semantics/check-omp-structure.h (-2)
  • (modified) flang/lib/Semantics/openmp-modifiers.cpp (+48)
  • (modified) flang/test/Parser/OpenMP/order-clause01.f90 (+25-25)
diff --git a/flang/examples/FeatureList/FeatureList.cpp b/flang/examples/FeatureList/FeatureList.cpp
index 753ecb918a9ccb..e1c42586c62c94 100644
--- a/flang/examples/FeatureList/FeatureList.cpp
+++ b/flang/examples/FeatureList/FeatureList.cpp
@@ -505,9 +505,9 @@ struct NodeVisitor {
   READ_FEATURE(OmpObject)
   READ_FEATURE(OmpObjectList)
   READ_FEATURE(OmpOrderClause)
-  READ_FEATURE(OmpOrderClause::Type)
+  READ_FEATURE(OmpOrderClause::Ordering)
   READ_FEATURE(OmpOrderModifier)
-  READ_FEATURE(OmpOrderModifier::Kind)
+  READ_FEATURE(OmpOrderModifier::Value)
   READ_FEATURE(OmpProcBindClause)
   READ_FEATURE(OmpProcBindClause::Type)
   READ_FEATURE(OmpReductionClause)
@@ -527,11 +527,10 @@ struct NodeVisitor {
   READ_FEATURE(OmpDeviceClause::DeviceModifier)
   READ_FEATURE(OmpDeviceTypeClause)
   READ_FEATURE(OmpDeviceTypeClause::Type)
-  READ_FEATURE(OmpScheduleModifier)
-  READ_FEATURE(OmpScheduleModifier::Modifier1)
-  READ_FEATURE(OmpScheduleModifier::Modifier2)
-  READ_FEATURE(OmpScheduleModifierType)
-  READ_FEATURE(OmpScheduleModifierType::ModType)
+  READ_FEATURE(OmpChunkModifier)
+  READ_FEATURE(OmpChunkModifier::Value)
+  READ_FEATURE(OmpOrderingModifier)
+  READ_FEATURE(OmpOrderingModifier::Value)
   READ_FEATURE(OmpSectionBlocks)
   READ_FEATURE(OmpSectionsDirective)
   READ_FEATURE(OmpSimpleStandaloneDirective)
diff --git a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp
index a9ff163f8243ce..a3d9b0cfdc79b8 100644
--- a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp
+++ b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp
@@ -213,14 +213,18 @@ void OpenMPCounterVisitor::Post(const OmpVariableCategory::Value &c) {
       "variable_category=" + std::string{OmpVariableCategory::EnumToString(c)} +
       ";";
 }
-void OpenMPCounterVisitor::Post(const OmpScheduleModifierType::ModType &c) {
+void OpenMPCounterVisitor::Post(const OmpChunkModifier::Value &c) {
   clauseDetails +=
-      "modifier=" + std::string{OmpScheduleModifierType::EnumToString(c)} + ";";
+      "modifier=" + std::string{OmpChunkModifier::EnumToString(c)} + ";";
 }
 void OpenMPCounterVisitor::Post(const OmpLinearModifier::Value &c) {
   clauseDetails +=
       "modifier=" + std::string{OmpLinearModifier::EnumToString(c)} + ";";
 }
+void OpenMPCounterVisitor::Post(const OmpOrderingModifier::Value &c) {
+  clauseDetails +=
+      "modifier=" + std::string{OmpOrderingModifier::EnumToString(c)} + ";";
+}
 void OpenMPCounterVisitor::Post(const OmpTaskDependenceType::Value &c) {
   clauseDetails +=
       "type=" + std::string{OmpTaskDependenceType::EnumToString(c)} + ";";
@@ -228,7 +232,7 @@ void OpenMPCounterVisitor::Post(const OmpTaskDependenceType::Value &c) {
 void OpenMPCounterVisitor::Post(const OmpMapClause::Type &c) {
   clauseDetails += "type=" + std::string{OmpMapClause::EnumToString(c)} + ";";
 }
-void OpenMPCounterVisitor::Post(const OmpScheduleClause::ScheduleType &c) {
+void OpenMPCounterVisitor::Post(const OmpScheduleClause::Kind &c) {
   clauseDetails +=
       "type=" + std::string{OmpScheduleClause::EnumToString(c)} + ";";
 }
diff --git a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h
index 83bd3644577e1c..608cb5a2241b83 100644
--- a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h
+++ b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h
@@ -71,8 +71,9 @@ struct OpenMPCounterVisitor {
   void Post(const OmpDefaultmapClause::ImplicitBehavior &c);
   void Post(const OmpVariableCategory::Value &c);
   void Post(const OmpDeviceTypeClause::Type &c);
-  void Post(const OmpScheduleModifierType::ModType &c);
+  void Post(const OmpChunkModifier::Value &c);
   void Post(const OmpLinearModifier::Value &c);
+  void Post(const OmpOrderingModifier::Value &c);
   void Post(const OmpTaskDependenceType::Value &c);
   void Post(const OmpMapClause::Type &c);
   void Post(const OmpScheduleClause::ScheduleType &c);
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index e9c149758c1493..6d1e7329d5cce8 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -559,9 +559,10 @@ class ParseTreeDumper {
   NODE(parser, OmpObject)
   NODE(parser, OmpObjectList)
   NODE(parser, OmpOrderClause)
-  NODE_ENUM(OmpOrderClause, Type)
+  NODE(OmpOrderClause, Modifier)
+  NODE_ENUM(OmpOrderClause, Ordering)
   NODE(parser, OmpOrderModifier)
-  NODE_ENUM(OmpOrderModifier, Kind)
+  NODE_ENUM(OmpOrderModifier, Value)
   NODE(parser, OmpGrainsizeClause)
   NODE_ENUM(OmpGrainsizeClause, Prescriptiveness)
   NODE(parser, OmpNumTasksClause)
@@ -585,17 +586,17 @@ class ParseTreeDumper {
   NODE(OmpAllocateClause::AllocateModifier, ComplexModifier)
   NODE(OmpAllocateClause::AllocateModifier, Align)
   NODE(parser, OmpScheduleClause)
-  NODE_ENUM(OmpScheduleClause, ScheduleType)
+  NODE(OmpScheduleClause, Modifier)
+  NODE_ENUM(OmpScheduleClause, Kind)
   NODE(parser, OmpDeviceClause)
   NODE_ENUM(OmpDeviceClause, DeviceModifier)
   NODE(parser, OmpDeviceTypeClause)
   NODE_ENUM(OmpDeviceTypeClause, Type)
   NODE(parser, OmpUpdateClause)
-  NODE(parser, OmpScheduleModifier)
-  NODE(OmpScheduleModifier, Modifier1)
-  NODE(OmpScheduleModifier, Modifier2)
-  NODE(parser, OmpScheduleModifierType)
-  NODE_ENUM(OmpScheduleModifierType, ModType)
+  NODE(parser, OmpChunkModifier)
+  NODE_ENUM(OmpChunkModifier, Value)
+  NODE(parser, OmpOrderingModifier)
+  NODE_ENUM(OmpOrderingModifier, Value)
   NODE(parser, OmpSectionBlocks)
   NODE(parser, OmpSectionsDirective)
   NODE(parser, OmpSimpleStandaloneDirective)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 32ebaa7fbffcae..de179f47be8fca 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3457,6 +3457,17 @@ inline namespace modifier {
 //   ENUM_CLASS(Value, Keyword1, Keyword2);
 // };
 
+// Ref: [5.2:252-254]
+//
+// chunk-modifier ->
+//    SIMD                                          // since 5.2
+//
+// Prior to 5.2 "chunk-modifier" was a part of "modifier" on SCHEDULE clause.
+struct OmpChunkModifier {
+  ENUM_CLASS(Value, Simd)
+  WRAPPER_CLASS_BOILERPLATE(OmpChunkModifier, Value);
+};
+
 // Ref: [5.0:47-49], [5.1:49-51], [5.2:67-69]
 //
 // iterator-specifier ->
@@ -3508,6 +3519,30 @@ struct OmpLinearModifier {
   WRAPPER_CLASS_BOILERPLATE(OmpLinearModifier, Value);
 };
 
+// Ref: [4.5:56-63], [5.0:101-109], [5.1:126-133], [5.2:252-254]
+//
+// modifier ->
+//    MONOTONIC | NONMONOTONIC | SIMD               // since 4.5, until 5.1
+// ordering-modifier ->
+//    MONOTONIC | NONMONOTONIC                      // since 5.2
+//
+// Until 5.1, the SCHEDULE clause accepted up to two instances of "modifier".
+// Since 5.2 "modifier" was replaced with "ordering-modifier" and "chunk-
+// modifier".
+struct OmpOrderingModifier {
+  ENUM_CLASS(Value, Monotonic, Nonmonotonic, Simd)
+  WRAPPER_CLASS_BOILERPLATE(OmpOrderingModifier, Value);
+};
+
+// Ref: [5.1:125-126], [5.2:233-234]
+//
+// order-modifier ->
+//    REPRODUCIBLE | UNCONSTRAINED                  // since 5.1
+struct OmpOrderModifier {
+  ENUM_CLASS(Value, Reproducible, Unconstrained)
+  WRAPPER_CLASS_BOILERPLATE(OmpOrderModifier, Value);
+};
+
 // Ref: [4.5:201-207], [5.0:293-299], [5.1:325-331], [5.2:124]
 //
 // reduction-identifier ->
@@ -3786,16 +3821,16 @@ struct OmpMapClause {
       t;
 };
 
-// 2.9.5 order-clause -> ORDER ([order-modifier :]concurrent)
-struct OmpOrderModifier {
-  ENUM_CLASS(Kind, Reproducible, Unconstrained)
-  WRAPPER_CLASS_BOILERPLATE(OmpOrderModifier, Kind);
-};
-
+// Ref: [5.0:101-109], [5.1:126-134], [5.2:233-234]
+//
+// order-clause ->
+//    ORDER(CONCURRENT) |                           // since 5.0
+//    ORDER([order-modifier:] CONCURRENT)           // since 5.1
 struct OmpOrderClause {
   TUPLE_CLASS_BOILERPLATE(OmpOrderClause);
-  ENUM_CLASS(Type, Concurrent)
-  std::tuple<std::optional<OmpOrderModifier>, Type> t;
+  ENUM_CLASS(Ordering, Concurrent)
+  MODIFIER_BOILERPLATE(OmpOrderModifier);
+  std::tuple<MODIFIERS(), Ordering> t;
 };
 
 // 2.5 proc-bind-clause -> PROC_BIND (MASTER | CLOSE | SPREAD)
@@ -3816,27 +3851,19 @@ struct OmpReductionClause {
   std::tuple<MODIFIERS(), OmpObjectList> t;
 };
 
-// 2.7.1 sched-modifier -> MONOTONIC | NONMONOTONIC | SIMD
-struct OmpScheduleModifierType {
-  ENUM_CLASS(ModType, Monotonic, Nonmonotonic, Simd)
-  WRAPPER_CLASS_BOILERPLATE(OmpScheduleModifierType, ModType);
-};
-
-struct OmpScheduleModifier {
-  TUPLE_CLASS_BOILERPLATE(OmpScheduleModifier);
-  WRAPPER_CLASS(Modifier1, OmpScheduleModifierType);
-  WRAPPER_CLASS(Modifier2, OmpScheduleModifierType);
-  std::tuple<Modifier1, std::optional<Modifier2>> t;
-};
-
-// 2.7.1 schedule-clause -> SCHEDULE ([sched-modifier1] [, sched-modifier2]:]
-//                                    kind[, chunk_size])
+// Ref: [4.5:56-63], [5.0:101-109], [5.1:126-133], [5.2:252-254]
+//
+// schedule-clause ->
+//    SCHEDULE([modifier[, modifier]:]
+//        kind[, chunk-size])                       // since 4.5, until 5.1
+// schedule-clause ->
+//    SCHEDULE([ordering-modifier], chunk-modifier],
+//        kind[, chunk_size])                       // since 5.2
 struct OmpScheduleClause {
   TUPLE_CLASS_BOILERPLATE(OmpScheduleClause);
-  ENUM_CLASS(ScheduleType, Static, Dynamic, Guided, Auto, Runtime)
-  std::tuple<std::optional<OmpScheduleModifier>, ScheduleType,
-      std::optional<ScalarIntExpr>>
-      t;
+  ENUM_CLASS(Kind, Static, Dynamic, Guided, Auto, Runtime)
+  MODIFIER_BOILERPLATE(OmpOrderingModifier, OmpChunkModifier);
+  std::tuple<MODIFIERS(), Kind, std::optional<ScalarIntExpr>> t;
 };
 
 // Ref: [4.5:107-109], [5.0:176-180], [5.1:205-210], [5.2:167-168]
diff --git a/flang/include/flang/Semantics/openmp-modifiers.h b/flang/include/flang/Semantics/openmp-modifiers.h
index 28fec7314cd8b5..fd6bd86fb280d8 100644
--- a/flang/include/flang/Semantics/openmp-modifiers.h
+++ b/flang/include/flang/Semantics/openmp-modifiers.h
@@ -61,6 +61,8 @@ struct OmpModifierDescriptor {
 
 template <typename SpecificTy> const OmpModifierDescriptor &OmpGetDescriptor();
 
+template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpChunkModifier>();
 template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpDependenceType>();
 template <>
@@ -68,6 +70,10 @@ const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpIterator>();
 template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpLinearModifier>();
 template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpOrderModifier>();
+template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpOrderingModifier>();
+template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpReductionIdentifier>();
 template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpReductionModifier>();
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index 4f9e7a7c1d78c3..8639d08827f4ed 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -1109,7 +1109,7 @@ Order make(const parser::OmpClause::Order &inp,
   using wrapped = parser::OmpOrderClause;
 
   CLAUSET_ENUM_CONVERT( //
-      convert1, parser::OmpOrderModifier::Kind, Order::OrderModifier,
+      convert1, parser::OmpOrderModifier::Value, Order::OrderModifier,
       // clang-format off
       MS(Reproducible,   Reproducible)
       MS(Unconstrained,  Unconstrained)
@@ -1117,20 +1117,18 @@ Order make(const parser::OmpClause::Order &inp,
   );
 
   CLAUSET_ENUM_CONVERT( //
-      convert2, wrapped::Type, Order::Ordering,
+      convert2, wrapped::Ordering, Order::Ordering,
       // clang-format off
       MS(Concurrent, Concurrent)
       // clang-format on
   );
 
-  auto &t0 = std::get<std::optional<parser::OmpOrderModifier>>(inp.v.t);
-  auto &t1 = std::get<wrapped::Type>(inp.v.t);
+  auto &mods = semantics::OmpGetModifiers(inp.v);
+  auto *t0 = semantics::OmpGetUniqueModifier<parser::OmpOrderModifier>(mods);
+  auto &t1 = std::get<wrapped::Ordering>(inp.v.t);
 
-  auto convert3 = [&](const parser::OmpOrderModifier &s) {
-    return convert1(s.v);
-  };
-  return Order{
-      {/*OrderModifier=*/maybeApply(convert3, t0), /*Ordering=*/convert2(t1)}};
+  return Order{{/*OrderModifier=*/maybeApplyToV(convert1, t0),
+                /*Ordering=*/convert2(t1)}};
 }
 
 Ordered make(const parser::OmpClause::Ordered &inp,
@@ -1197,10 +1195,10 @@ Reduction make(const parser::OmpClause::Reduction &inp,
   auto *t1 =
       semantics::OmpGetUniqueModifier<parser::OmpReductionIdentifier>(mods);
   auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
+  assert(t1 && "OmpReductionIdentifier is required");
+
   return Reduction{
-      {/*ReductionModifier=*/t0
-           ? std::make_optional<Reduction::ReductionModifier>(convert(t0->v))
-           : std::nullopt,
+      {/*ReductionModifier=*/maybeApplyToV(convert, t0),
        /*ReductionIdentifiers=*/{makeReductionOperator(*t1, semaCtx)},
        /*List=*/makeObjects(t2, semaCtx)}};
 }
@@ -1221,7 +1219,7 @@ Schedule make(const parser::OmpClause::Schedule &inp,
   using wrapped = parser::OmpScheduleClause;
 
   CLAUSET_ENUM_CONVERT( //
-      convert1, wrapped::ScheduleType, Schedule::Kind,
+      convert1, wrapped::Kind, Schedule::Kind,
       // clang-format off
       MS(Static,   Static)
       MS(Dynamic,  Dynamic)
@@ -1232,8 +1230,7 @@ Schedule make(const parser::OmpClause::Schedule &inp,
   );
 
   CLAUSET_ENUM_CONVERT( //
-      convert2, parser::OmpScheduleModifierType::ModType,
-      Schedule::OrderingModifier,
+      convert2, parser::OmpOrderingModifier::Value, Schedule::OrderingModifier,
       // clang-format off
       MS(Monotonic,    Monotonic)
       MS(Nonmonotonic, Nonmonotonic)
@@ -1241,48 +1238,22 @@ Schedule make(const parser::OmpClause::Schedule &inp,
   );
 
   CLAUSET_ENUM_CONVERT( //
-      convert3, parser::OmpScheduleModifierType::ModType,
-      Schedule::ChunkModifier,
+      convert3, parser::OmpChunkModifier::Value, Schedule::ChunkModifier,
       // clang-format off
       MS(Simd, Simd)
       // clang-format on
   );
 
-  auto &t0 = std::get<std::optional<parser::OmpScheduleModifier>>(inp.v.t);
-  auto &t1 = std::get<wrapped::ScheduleType>(inp.v.t);
-  auto &t2 = std::get<std::optional<parser::ScalarIntExpr>>(inp.v.t);
-
-  if (!t0) {
-    return Schedule{{/*Kind=*/convert1(t1), /*OrderingModifier=*/std::nullopt,
-                     /*ChunkModifier=*/std::nullopt,
-                     /*ChunkSize=*/maybeApply(makeExprFn(semaCtx), t2)}};
-  }
-
-  // The members of parser::OmpScheduleModifier correspond to OrderingModifier,
-  // and ChunkModifier, but they can appear in any order.
-  auto &m1 = std::get<parser::OmpScheduleModifier::Modifier1>(t0->t);
-  auto &m2 =
-      std::get<std::optional<parser::OmpScheduleModifier::Modifier2>>(t0->t);
-
-  std::optional<Schedule::OrderingModifier> omod;
-  std::optional<Schedule::ChunkModifier> cmod;
-
-  if (m1.v.v == parser::OmpScheduleModifierType::ModType::Simd) {
-    // m1 is chunk-modifier
-    cmod = convert3(m1.v.v);
-    if (m2)
-      omod = convert2(m2->v.v);
-  } else {
-    // m1 is ordering-modifier
-    omod = convert2(m1.v.v);
-    if (m2)
-      cmod = convert3(m2->v.v);
-  }
+  auto &mods = semantics::OmpGetModifiers(inp.v);
+  auto *t0 = semantics::OmpGetUniqueModifier<parser::OmpOrderingModifier>(mods);
+  auto *t1 = semantics::OmpGetUniqueModifier<parser::OmpChunkModifier>(mods);
+  auto &t2 = std::get<wrapped::Kind>(inp.v.t);
+  auto &t3 = std::get<std::optional<parser::ScalarIntExpr>>(inp.v.t);
 
-  return Schedule{{/*Kind=*/convert1(t1),
-                   /*OrderingModifier=*/omod,
-                   /*ChunkModifier=*/cmod,
-                   /*ChunkSize=*/maybeApply(makeExprFn(semaCtx), t2)}};
+  return Schedule{{/*Kind=*/convert1(t2),
+                   /*OrderingModifier=*/maybeApplyToV(convert2, t0),
+                   /*ChunkModifier=*/maybeApplyToV(convert3, t1),
+                   /*ChunkSize=*/maybeApply(makeExprFn(semaCtx), t3)}};
 }
 
 // SeqCst: empty
@@ -1326,6 +1297,8 @@ TaskReduction make(const parser::OmpClause::TaskReduction &inp,
   auto *t0 =
       semantics::OmpGetUniqueModifier<parser::OmpReductionIdentifier>(mods);
   auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
+  assert(t0 && "OmpReductionIdentifier is required");
+
   return TaskReduction{
       {/*ReductionIdentifiers=*/{makeReductionOperator(*t0, semaCtx)},
        /*List=*/makeObjects(t1, semaCtx)}};
diff --git a/flang/lib/Lower/OpenMP/Clauses.h b/flang/lib/Lower/OpenMP/Clauses.h
index ada50e0488837b..9c053e2213c675 100644
--- a/flang/lib/Lower/OpenMP/Clauses.h
+++ b/flang/lib/Lower/OpenMP/Clauses.h
@@ -153,6 +153,16 @@ std::optional<ResultTy> maybeApply(FuncTy &&func,
   return std::move(func(*arg));
 }
 
+template <
+    typename FuncTy, //
+    typename ArgTy,  //
+    typename ResultTy = std::invoke_result_t<FuncTy, typename ArgTy::Value>>
+std::optional<ResultTy> maybeApplyToV(FuncTy &&func, const ArgTy *arg) {
+  if (!arg)
+    return std::nullopt;
+  return std::move(func(arg->v));
+}
+
 std::optional<Object> getBaseObject(const Object &object,
                                     semantics::SemanticsContext &semaCtx);
 
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index a1d368d73ab83a..ceae20270d13d0 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -228,6 +228,18 @@ TYPE_PARSER(construct<OmpLinearModifier>( //
 TYPE_PARSER(construct<OmpReductionIdentifier>(Parser<DefinedOperator>{}) ||
     construct<OmpReductionIdentifier>(Parser<ProcedureDesignator>{}))
 
+TYPE_PARSER(construct<OmpChunkModifier>( //
+    "SIMD" >> pure(OmpChunkModifier::Value::Simd)))
+
+TYPE_PARSER(construct<OmpOrderModifier>(
+    "REPRODUCIBLE" >> pure(OmpOrderModifier::Value::Reproducible) ||
+    "UNCONSTRAINED" >> pure(OmpOrderModifier::Value::Unconstrained)))
+
+TYPE_PARSER(construct<OmpOrderingModifier>(
+    "MONOTONIC" >> pure(OmpOrderingModifier::Value::Monotonic) ||
+    "NONMONOTONIC" >> pure(OmpOrderingModifier::Value::Nonmonotonic) ||
+    "SIMD" >> pure(OmpOrderingModifier::Value::Simd)))
+
 TYPE_PARSER(construct<OmpReductionModifier>(
     "INSCAN" >> pure(OmpReductionModifier::Value::Inscan) ||
     "TASK" >> pure(OmpReductionModifier::Value::Task) ||
@@ -241,12 +253,6 @@ TYPE_PARSER(construct<OmpTaskDependenceType>(
     "MUTEXINOUTSET" >> pure(OmpTaskDependenceType::Value::Mutexinoutset) ||
     "OUT" >> pure(OmpTaskDependenceType::Value::Out)))
 
-// This could be auto-generated.
-TYPE_PARSER(sourced(construct<OmpReductionClause::Modifier>(sourced(
-    construct<OmpReductionClause::Modifier>(Parser<OmpReductionModifier>{}) ||
-    construct<OmpReductionClause::Modifier>(
-        Parser<OmpReductionIdentifier>{})))))
-
 TYPE_PARSER(construct<OmpVariableCategory>(
     "AGGREGATE" >> pure(OmpVariableCategory::Value::Aggregate) ||
     "ALL"_id >> pure(OmpVariableCategory::Value::All) ||
@@ -254,6 +260,19 @@ TYPE_PARSER(construct<OmpVariableCategory>(
     "POINTER" >> pure(OmpVariableCategory::Value::Pointer) ||
     "SCALAR" >> pure(OmpVariableCategory::Value::Scalar)))
 
+// This could be auto-generated.
+TYPE_PARSER(
+    sourced(construct<OmpOrderClause::Modifier>(Parser<OmpOrderModifier>{})))
+
+TYPE_PARSER(sourced(construct<OmpReductionClause::Modifier>(sourced(
+    construct<OmpReductionClause::Modifier>(Parser<OmpReductionModifier>{}) ||
+    construct<OmpReductionClause::Modifier>(
+        Parser<OmpReductionIdentifier>{})))))
+
+TYPE_PARSER(sourced(construct<OmpScheduleClause::Modifier>(sourced(
+    construct<OmpScheduleClause::Modifier>(Parser<OmpChunkModifier>{}) ||
+    construct<OmpScheduleClause::Modifier>(Parser<OmpOrderingModifier>{})))))
+
 TYPE_PARSER(sourced(
     construct<OmpDefaultmapClause::Modifier>(Parser<OmpVariableCategory>{})))
 
@@ -336,25 +355,16 @@ TYPE_PARSER(construct<OmpDefaultmapClause>(
         "DEFAULT" >> pure(OmpDefaultmapClause::ImplicitBehavior::Default)),
     maybe(":" >> nonemptyList(Parser<OmpDefaultmapClause::Modifier>{}))))
 
-// 2.7.1 SCHEDULE ([modifier1 [, modifier2]:]kind[, chunk_size])
-//       Modifier ->  MONITONIC | NONMONOTONIC | SIMD
-//       kind -> STATIC | D...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Nov 21, 2024

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

Author: Krzysztof Parzyszek (kparzysz)

Changes

This actually simplifies the AST node for the schedule clause: the two allowed modifiers can be easily classified as the ordering-modifier and the chunk-modifier during parsing without the need to create additional classes.


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

14 Files Affected:

  • (modified) flang/examples/FeatureList/FeatureList.cpp (+6-7)
  • (modified) flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp (+7-3)
  • (modified) flang/examples/FlangOmpReport/FlangOmpReportVisitor.h (+2-1)
  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+9-8)
  • (modified) flang/include/flang/Parser/parse-tree.h (+54-27)
  • (modified) flang/include/flang/Semantics/openmp-modifiers.h (+6)
  • (modified) flang/lib/Lower/OpenMP/Clauses.cpp (+24-51)
  • (modified) flang/lib/Lower/OpenMP/Clauses.h (+10)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+38-33)
  • (modified) flang/lib/Parser/unparse.cpp (+11-12)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+23-35)
  • (modified) flang/lib/Semantics/check-omp-structure.h (-2)
  • (modified) flang/lib/Semantics/openmp-modifiers.cpp (+48)
  • (modified) flang/test/Parser/OpenMP/order-clause01.f90 (+25-25)
diff --git a/flang/examples/FeatureList/FeatureList.cpp b/flang/examples/FeatureList/FeatureList.cpp
index 753ecb918a9ccb..e1c42586c62c94 100644
--- a/flang/examples/FeatureList/FeatureList.cpp
+++ b/flang/examples/FeatureList/FeatureList.cpp
@@ -505,9 +505,9 @@ struct NodeVisitor {
   READ_FEATURE(OmpObject)
   READ_FEATURE(OmpObjectList)
   READ_FEATURE(OmpOrderClause)
-  READ_FEATURE(OmpOrderClause::Type)
+  READ_FEATURE(OmpOrderClause::Ordering)
   READ_FEATURE(OmpOrderModifier)
-  READ_FEATURE(OmpOrderModifier::Kind)
+  READ_FEATURE(OmpOrderModifier::Value)
   READ_FEATURE(OmpProcBindClause)
   READ_FEATURE(OmpProcBindClause::Type)
   READ_FEATURE(OmpReductionClause)
@@ -527,11 +527,10 @@ struct NodeVisitor {
   READ_FEATURE(OmpDeviceClause::DeviceModifier)
   READ_FEATURE(OmpDeviceTypeClause)
   READ_FEATURE(OmpDeviceTypeClause::Type)
-  READ_FEATURE(OmpScheduleModifier)
-  READ_FEATURE(OmpScheduleModifier::Modifier1)
-  READ_FEATURE(OmpScheduleModifier::Modifier2)
-  READ_FEATURE(OmpScheduleModifierType)
-  READ_FEATURE(OmpScheduleModifierType::ModType)
+  READ_FEATURE(OmpChunkModifier)
+  READ_FEATURE(OmpChunkModifier::Value)
+  READ_FEATURE(OmpOrderingModifier)
+  READ_FEATURE(OmpOrderingModifier::Value)
   READ_FEATURE(OmpSectionBlocks)
   READ_FEATURE(OmpSectionsDirective)
   READ_FEATURE(OmpSimpleStandaloneDirective)
diff --git a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp
index a9ff163f8243ce..a3d9b0cfdc79b8 100644
--- a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp
+++ b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp
@@ -213,14 +213,18 @@ void OpenMPCounterVisitor::Post(const OmpVariableCategory::Value &c) {
       "variable_category=" + std::string{OmpVariableCategory::EnumToString(c)} +
       ";";
 }
-void OpenMPCounterVisitor::Post(const OmpScheduleModifierType::ModType &c) {
+void OpenMPCounterVisitor::Post(const OmpChunkModifier::Value &c) {
   clauseDetails +=
-      "modifier=" + std::string{OmpScheduleModifierType::EnumToString(c)} + ";";
+      "modifier=" + std::string{OmpChunkModifier::EnumToString(c)} + ";";
 }
 void OpenMPCounterVisitor::Post(const OmpLinearModifier::Value &c) {
   clauseDetails +=
       "modifier=" + std::string{OmpLinearModifier::EnumToString(c)} + ";";
 }
+void OpenMPCounterVisitor::Post(const OmpOrderingModifier::Value &c) {
+  clauseDetails +=
+      "modifier=" + std::string{OmpOrderingModifier::EnumToString(c)} + ";";
+}
 void OpenMPCounterVisitor::Post(const OmpTaskDependenceType::Value &c) {
   clauseDetails +=
       "type=" + std::string{OmpTaskDependenceType::EnumToString(c)} + ";";
@@ -228,7 +232,7 @@ void OpenMPCounterVisitor::Post(const OmpTaskDependenceType::Value &c) {
 void OpenMPCounterVisitor::Post(const OmpMapClause::Type &c) {
   clauseDetails += "type=" + std::string{OmpMapClause::EnumToString(c)} + ";";
 }
-void OpenMPCounterVisitor::Post(const OmpScheduleClause::ScheduleType &c) {
+void OpenMPCounterVisitor::Post(const OmpScheduleClause::Kind &c) {
   clauseDetails +=
       "type=" + std::string{OmpScheduleClause::EnumToString(c)} + ";";
 }
diff --git a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h
index 83bd3644577e1c..608cb5a2241b83 100644
--- a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h
+++ b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h
@@ -71,8 +71,9 @@ struct OpenMPCounterVisitor {
   void Post(const OmpDefaultmapClause::ImplicitBehavior &c);
   void Post(const OmpVariableCategory::Value &c);
   void Post(const OmpDeviceTypeClause::Type &c);
-  void Post(const OmpScheduleModifierType::ModType &c);
+  void Post(const OmpChunkModifier::Value &c);
   void Post(const OmpLinearModifier::Value &c);
+  void Post(const OmpOrderingModifier::Value &c);
   void Post(const OmpTaskDependenceType::Value &c);
   void Post(const OmpMapClause::Type &c);
   void Post(const OmpScheduleClause::ScheduleType &c);
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index e9c149758c1493..6d1e7329d5cce8 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -559,9 +559,10 @@ class ParseTreeDumper {
   NODE(parser, OmpObject)
   NODE(parser, OmpObjectList)
   NODE(parser, OmpOrderClause)
-  NODE_ENUM(OmpOrderClause, Type)
+  NODE(OmpOrderClause, Modifier)
+  NODE_ENUM(OmpOrderClause, Ordering)
   NODE(parser, OmpOrderModifier)
-  NODE_ENUM(OmpOrderModifier, Kind)
+  NODE_ENUM(OmpOrderModifier, Value)
   NODE(parser, OmpGrainsizeClause)
   NODE_ENUM(OmpGrainsizeClause, Prescriptiveness)
   NODE(parser, OmpNumTasksClause)
@@ -585,17 +586,17 @@ class ParseTreeDumper {
   NODE(OmpAllocateClause::AllocateModifier, ComplexModifier)
   NODE(OmpAllocateClause::AllocateModifier, Align)
   NODE(parser, OmpScheduleClause)
-  NODE_ENUM(OmpScheduleClause, ScheduleType)
+  NODE(OmpScheduleClause, Modifier)
+  NODE_ENUM(OmpScheduleClause, Kind)
   NODE(parser, OmpDeviceClause)
   NODE_ENUM(OmpDeviceClause, DeviceModifier)
   NODE(parser, OmpDeviceTypeClause)
   NODE_ENUM(OmpDeviceTypeClause, Type)
   NODE(parser, OmpUpdateClause)
-  NODE(parser, OmpScheduleModifier)
-  NODE(OmpScheduleModifier, Modifier1)
-  NODE(OmpScheduleModifier, Modifier2)
-  NODE(parser, OmpScheduleModifierType)
-  NODE_ENUM(OmpScheduleModifierType, ModType)
+  NODE(parser, OmpChunkModifier)
+  NODE_ENUM(OmpChunkModifier, Value)
+  NODE(parser, OmpOrderingModifier)
+  NODE_ENUM(OmpOrderingModifier, Value)
   NODE(parser, OmpSectionBlocks)
   NODE(parser, OmpSectionsDirective)
   NODE(parser, OmpSimpleStandaloneDirective)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 32ebaa7fbffcae..de179f47be8fca 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3457,6 +3457,17 @@ inline namespace modifier {
 //   ENUM_CLASS(Value, Keyword1, Keyword2);
 // };
 
+// Ref: [5.2:252-254]
+//
+// chunk-modifier ->
+//    SIMD                                          // since 5.2
+//
+// Prior to 5.2 "chunk-modifier" was a part of "modifier" on SCHEDULE clause.
+struct OmpChunkModifier {
+  ENUM_CLASS(Value, Simd)
+  WRAPPER_CLASS_BOILERPLATE(OmpChunkModifier, Value);
+};
+
 // Ref: [5.0:47-49], [5.1:49-51], [5.2:67-69]
 //
 // iterator-specifier ->
@@ -3508,6 +3519,30 @@ struct OmpLinearModifier {
   WRAPPER_CLASS_BOILERPLATE(OmpLinearModifier, Value);
 };
 
+// Ref: [4.5:56-63], [5.0:101-109], [5.1:126-133], [5.2:252-254]
+//
+// modifier ->
+//    MONOTONIC | NONMONOTONIC | SIMD               // since 4.5, until 5.1
+// ordering-modifier ->
+//    MONOTONIC | NONMONOTONIC                      // since 5.2
+//
+// Until 5.1, the SCHEDULE clause accepted up to two instances of "modifier".
+// Since 5.2 "modifier" was replaced with "ordering-modifier" and "chunk-
+// modifier".
+struct OmpOrderingModifier {
+  ENUM_CLASS(Value, Monotonic, Nonmonotonic, Simd)
+  WRAPPER_CLASS_BOILERPLATE(OmpOrderingModifier, Value);
+};
+
+// Ref: [5.1:125-126], [5.2:233-234]
+//
+// order-modifier ->
+//    REPRODUCIBLE | UNCONSTRAINED                  // since 5.1
+struct OmpOrderModifier {
+  ENUM_CLASS(Value, Reproducible, Unconstrained)
+  WRAPPER_CLASS_BOILERPLATE(OmpOrderModifier, Value);
+};
+
 // Ref: [4.5:201-207], [5.0:293-299], [5.1:325-331], [5.2:124]
 //
 // reduction-identifier ->
@@ -3786,16 +3821,16 @@ struct OmpMapClause {
       t;
 };
 
-// 2.9.5 order-clause -> ORDER ([order-modifier :]concurrent)
-struct OmpOrderModifier {
-  ENUM_CLASS(Kind, Reproducible, Unconstrained)
-  WRAPPER_CLASS_BOILERPLATE(OmpOrderModifier, Kind);
-};
-
+// Ref: [5.0:101-109], [5.1:126-134], [5.2:233-234]
+//
+// order-clause ->
+//    ORDER(CONCURRENT) |                           // since 5.0
+//    ORDER([order-modifier:] CONCURRENT)           // since 5.1
 struct OmpOrderClause {
   TUPLE_CLASS_BOILERPLATE(OmpOrderClause);
-  ENUM_CLASS(Type, Concurrent)
-  std::tuple<std::optional<OmpOrderModifier>, Type> t;
+  ENUM_CLASS(Ordering, Concurrent)
+  MODIFIER_BOILERPLATE(OmpOrderModifier);
+  std::tuple<MODIFIERS(), Ordering> t;
 };
 
 // 2.5 proc-bind-clause -> PROC_BIND (MASTER | CLOSE | SPREAD)
@@ -3816,27 +3851,19 @@ struct OmpReductionClause {
   std::tuple<MODIFIERS(), OmpObjectList> t;
 };
 
-// 2.7.1 sched-modifier -> MONOTONIC | NONMONOTONIC | SIMD
-struct OmpScheduleModifierType {
-  ENUM_CLASS(ModType, Monotonic, Nonmonotonic, Simd)
-  WRAPPER_CLASS_BOILERPLATE(OmpScheduleModifierType, ModType);
-};
-
-struct OmpScheduleModifier {
-  TUPLE_CLASS_BOILERPLATE(OmpScheduleModifier);
-  WRAPPER_CLASS(Modifier1, OmpScheduleModifierType);
-  WRAPPER_CLASS(Modifier2, OmpScheduleModifierType);
-  std::tuple<Modifier1, std::optional<Modifier2>> t;
-};
-
-// 2.7.1 schedule-clause -> SCHEDULE ([sched-modifier1] [, sched-modifier2]:]
-//                                    kind[, chunk_size])
+// Ref: [4.5:56-63], [5.0:101-109], [5.1:126-133], [5.2:252-254]
+//
+// schedule-clause ->
+//    SCHEDULE([modifier[, modifier]:]
+//        kind[, chunk-size])                       // since 4.5, until 5.1
+// schedule-clause ->
+//    SCHEDULE([ordering-modifier], chunk-modifier],
+//        kind[, chunk_size])                       // since 5.2
 struct OmpScheduleClause {
   TUPLE_CLASS_BOILERPLATE(OmpScheduleClause);
-  ENUM_CLASS(ScheduleType, Static, Dynamic, Guided, Auto, Runtime)
-  std::tuple<std::optional<OmpScheduleModifier>, ScheduleType,
-      std::optional<ScalarIntExpr>>
-      t;
+  ENUM_CLASS(Kind, Static, Dynamic, Guided, Auto, Runtime)
+  MODIFIER_BOILERPLATE(OmpOrderingModifier, OmpChunkModifier);
+  std::tuple<MODIFIERS(), Kind, std::optional<ScalarIntExpr>> t;
 };
 
 // Ref: [4.5:107-109], [5.0:176-180], [5.1:205-210], [5.2:167-168]
diff --git a/flang/include/flang/Semantics/openmp-modifiers.h b/flang/include/flang/Semantics/openmp-modifiers.h
index 28fec7314cd8b5..fd6bd86fb280d8 100644
--- a/flang/include/flang/Semantics/openmp-modifiers.h
+++ b/flang/include/flang/Semantics/openmp-modifiers.h
@@ -61,6 +61,8 @@ struct OmpModifierDescriptor {
 
 template <typename SpecificTy> const OmpModifierDescriptor &OmpGetDescriptor();
 
+template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpChunkModifier>();
 template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpDependenceType>();
 template <>
@@ -68,6 +70,10 @@ const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpIterator>();
 template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpLinearModifier>();
 template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpOrderModifier>();
+template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpOrderingModifier>();
+template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpReductionIdentifier>();
 template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpReductionModifier>();
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index 4f9e7a7c1d78c3..8639d08827f4ed 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -1109,7 +1109,7 @@ Order make(const parser::OmpClause::Order &inp,
   using wrapped = parser::OmpOrderClause;
 
   CLAUSET_ENUM_CONVERT( //
-      convert1, parser::OmpOrderModifier::Kind, Order::OrderModifier,
+      convert1, parser::OmpOrderModifier::Value, Order::OrderModifier,
       // clang-format off
       MS(Reproducible,   Reproducible)
       MS(Unconstrained,  Unconstrained)
@@ -1117,20 +1117,18 @@ Order make(const parser::OmpClause::Order &inp,
   );
 
   CLAUSET_ENUM_CONVERT( //
-      convert2, wrapped::Type, Order::Ordering,
+      convert2, wrapped::Ordering, Order::Ordering,
       // clang-format off
       MS(Concurrent, Concurrent)
       // clang-format on
   );
 
-  auto &t0 = std::get<std::optional<parser::OmpOrderModifier>>(inp.v.t);
-  auto &t1 = std::get<wrapped::Type>(inp.v.t);
+  auto &mods = semantics::OmpGetModifiers(inp.v);
+  auto *t0 = semantics::OmpGetUniqueModifier<parser::OmpOrderModifier>(mods);
+  auto &t1 = std::get<wrapped::Ordering>(inp.v.t);
 
-  auto convert3 = [&](const parser::OmpOrderModifier &s) {
-    return convert1(s.v);
-  };
-  return Order{
-      {/*OrderModifier=*/maybeApply(convert3, t0), /*Ordering=*/convert2(t1)}};
+  return Order{{/*OrderModifier=*/maybeApplyToV(convert1, t0),
+                /*Ordering=*/convert2(t1)}};
 }
 
 Ordered make(const parser::OmpClause::Ordered &inp,
@@ -1197,10 +1195,10 @@ Reduction make(const parser::OmpClause::Reduction &inp,
   auto *t1 =
       semantics::OmpGetUniqueModifier<parser::OmpReductionIdentifier>(mods);
   auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
+  assert(t1 && "OmpReductionIdentifier is required");
+
   return Reduction{
-      {/*ReductionModifier=*/t0
-           ? std::make_optional<Reduction::ReductionModifier>(convert(t0->v))
-           : std::nullopt,
+      {/*ReductionModifier=*/maybeApplyToV(convert, t0),
        /*ReductionIdentifiers=*/{makeReductionOperator(*t1, semaCtx)},
        /*List=*/makeObjects(t2, semaCtx)}};
 }
@@ -1221,7 +1219,7 @@ Schedule make(const parser::OmpClause::Schedule &inp,
   using wrapped = parser::OmpScheduleClause;
 
   CLAUSET_ENUM_CONVERT( //
-      convert1, wrapped::ScheduleType, Schedule::Kind,
+      convert1, wrapped::Kind, Schedule::Kind,
       // clang-format off
       MS(Static,   Static)
       MS(Dynamic,  Dynamic)
@@ -1232,8 +1230,7 @@ Schedule make(const parser::OmpClause::Schedule &inp,
   );
 
   CLAUSET_ENUM_CONVERT( //
-      convert2, parser::OmpScheduleModifierType::ModType,
-      Schedule::OrderingModifier,
+      convert2, parser::OmpOrderingModifier::Value, Schedule::OrderingModifier,
       // clang-format off
       MS(Monotonic,    Monotonic)
       MS(Nonmonotonic, Nonmonotonic)
@@ -1241,48 +1238,22 @@ Schedule make(const parser::OmpClause::Schedule &inp,
   );
 
   CLAUSET_ENUM_CONVERT( //
-      convert3, parser::OmpScheduleModifierType::ModType,
-      Schedule::ChunkModifier,
+      convert3, parser::OmpChunkModifier::Value, Schedule::ChunkModifier,
       // clang-format off
       MS(Simd, Simd)
       // clang-format on
   );
 
-  auto &t0 = std::get<std::optional<parser::OmpScheduleModifier>>(inp.v.t);
-  auto &t1 = std::get<wrapped::ScheduleType>(inp.v.t);
-  auto &t2 = std::get<std::optional<parser::ScalarIntExpr>>(inp.v.t);
-
-  if (!t0) {
-    return Schedule{{/*Kind=*/convert1(t1), /*OrderingModifier=*/std::nullopt,
-                     /*ChunkModifier=*/std::nullopt,
-                     /*ChunkSize=*/maybeApply(makeExprFn(semaCtx), t2)}};
-  }
-
-  // The members of parser::OmpScheduleModifier correspond to OrderingModifier,
-  // and ChunkModifier, but they can appear in any order.
-  auto &m1 = std::get<parser::OmpScheduleModifier::Modifier1>(t0->t);
-  auto &m2 =
-      std::get<std::optional<parser::OmpScheduleModifier::Modifier2>>(t0->t);
-
-  std::optional<Schedule::OrderingModifier> omod;
-  std::optional<Schedule::ChunkModifier> cmod;
-
-  if (m1.v.v == parser::OmpScheduleModifierType::ModType::Simd) {
-    // m1 is chunk-modifier
-    cmod = convert3(m1.v.v);
-    if (m2)
-      omod = convert2(m2->v.v);
-  } else {
-    // m1 is ordering-modifier
-    omod = convert2(m1.v.v);
-    if (m2)
-      cmod = convert3(m2->v.v);
-  }
+  auto &mods = semantics::OmpGetModifiers(inp.v);
+  auto *t0 = semantics::OmpGetUniqueModifier<parser::OmpOrderingModifier>(mods);
+  auto *t1 = semantics::OmpGetUniqueModifier<parser::OmpChunkModifier>(mods);
+  auto &t2 = std::get<wrapped::Kind>(inp.v.t);
+  auto &t3 = std::get<std::optional<parser::ScalarIntExpr>>(inp.v.t);
 
-  return Schedule{{/*Kind=*/convert1(t1),
-                   /*OrderingModifier=*/omod,
-                   /*ChunkModifier=*/cmod,
-                   /*ChunkSize=*/maybeApply(makeExprFn(semaCtx), t2)}};
+  return Schedule{{/*Kind=*/convert1(t2),
+                   /*OrderingModifier=*/maybeApplyToV(convert2, t0),
+                   /*ChunkModifier=*/maybeApplyToV(convert3, t1),
+                   /*ChunkSize=*/maybeApply(makeExprFn(semaCtx), t3)}};
 }
 
 // SeqCst: empty
@@ -1326,6 +1297,8 @@ TaskReduction make(const parser::OmpClause::TaskReduction &inp,
   auto *t0 =
       semantics::OmpGetUniqueModifier<parser::OmpReductionIdentifier>(mods);
   auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
+  assert(t0 && "OmpReductionIdentifier is required");
+
   return TaskReduction{
       {/*ReductionIdentifiers=*/{makeReductionOperator(*t0, semaCtx)},
        /*List=*/makeObjects(t1, semaCtx)}};
diff --git a/flang/lib/Lower/OpenMP/Clauses.h b/flang/lib/Lower/OpenMP/Clauses.h
index ada50e0488837b..9c053e2213c675 100644
--- a/flang/lib/Lower/OpenMP/Clauses.h
+++ b/flang/lib/Lower/OpenMP/Clauses.h
@@ -153,6 +153,16 @@ std::optional<ResultTy> maybeApply(FuncTy &&func,
   return std::move(func(*arg));
 }
 
+template <
+    typename FuncTy, //
+    typename ArgTy,  //
+    typename ResultTy = std::invoke_result_t<FuncTy, typename ArgTy::Value>>
+std::optional<ResultTy> maybeApplyToV(FuncTy &&func, const ArgTy *arg) {
+  if (!arg)
+    return std::nullopt;
+  return std::move(func(arg->v));
+}
+
 std::optional<Object> getBaseObject(const Object &object,
                                     semantics::SemanticsContext &semaCtx);
 
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index a1d368d73ab83a..ceae20270d13d0 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -228,6 +228,18 @@ TYPE_PARSER(construct<OmpLinearModifier>( //
 TYPE_PARSER(construct<OmpReductionIdentifier>(Parser<DefinedOperator>{}) ||
     construct<OmpReductionIdentifier>(Parser<ProcedureDesignator>{}))
 
+TYPE_PARSER(construct<OmpChunkModifier>( //
+    "SIMD" >> pure(OmpChunkModifier::Value::Simd)))
+
+TYPE_PARSER(construct<OmpOrderModifier>(
+    "REPRODUCIBLE" >> pure(OmpOrderModifier::Value::Reproducible) ||
+    "UNCONSTRAINED" >> pure(OmpOrderModifier::Value::Unconstrained)))
+
+TYPE_PARSER(construct<OmpOrderingModifier>(
+    "MONOTONIC" >> pure(OmpOrderingModifier::Value::Monotonic) ||
+    "NONMONOTONIC" >> pure(OmpOrderingModifier::Value::Nonmonotonic) ||
+    "SIMD" >> pure(OmpOrderingModifier::Value::Simd)))
+
 TYPE_PARSER(construct<OmpReductionModifier>(
     "INSCAN" >> pure(OmpReductionModifier::Value::Inscan) ||
     "TASK" >> pure(OmpReductionModifier::Value::Task) ||
@@ -241,12 +253,6 @@ TYPE_PARSER(construct<OmpTaskDependenceType>(
     "MUTEXINOUTSET" >> pure(OmpTaskDependenceType::Value::Mutexinoutset) ||
     "OUT" >> pure(OmpTaskDependenceType::Value::Out)))
 
-// This could be auto-generated.
-TYPE_PARSER(sourced(construct<OmpReductionClause::Modifier>(sourced(
-    construct<OmpReductionClause::Modifier>(Parser<OmpReductionModifier>{}) ||
-    construct<OmpReductionClause::Modifier>(
-        Parser<OmpReductionIdentifier>{})))))
-
 TYPE_PARSER(construct<OmpVariableCategory>(
     "AGGREGATE" >> pure(OmpVariableCategory::Value::Aggregate) ||
     "ALL"_id >> pure(OmpVariableCategory::Value::All) ||
@@ -254,6 +260,19 @@ TYPE_PARSER(construct<OmpVariableCategory>(
     "POINTER" >> pure(OmpVariableCategory::Value::Pointer) ||
     "SCALAR" >> pure(OmpVariableCategory::Value::Scalar)))
 
+// This could be auto-generated.
+TYPE_PARSER(
+    sourced(construct<OmpOrderClause::Modifier>(Parser<OmpOrderModifier>{})))
+
+TYPE_PARSER(sourced(construct<OmpReductionClause::Modifier>(sourced(
+    construct<OmpReductionClause::Modifier>(Parser<OmpReductionModifier>{}) ||
+    construct<OmpReductionClause::Modifier>(
+        Parser<OmpReductionIdentifier>{})))))
+
+TYPE_PARSER(sourced(construct<OmpScheduleClause::Modifier>(sourced(
+    construct<OmpScheduleClause::Modifier>(Parser<OmpChunkModifier>{}) ||
+    construct<OmpScheduleClause::Modifier>(Parser<OmpOrderingModifier>{})))))
+
 TYPE_PARSER(sourced(
     construct<OmpDefaultmapClause::Modifier>(Parser<OmpVariableCategory>{})))
 
@@ -336,25 +355,16 @@ TYPE_PARSER(construct<OmpDefaultmapClause>(
         "DEFAULT" >> pure(OmpDefaultmapClause::ImplicitBehavior::Default)),
     maybe(":" >> nonemptyList(Parser<OmpDefaultmapClause::Modifier>{}))))
 
-// 2.7.1 SCHEDULE ([modifier1 [, modifier2]:]kind[, chunk_size])
-//       Modifier ->  MONITONIC | NONMONOTONIC | SIMD
-//       kind -> STATIC | D...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Nov 21, 2024

@llvm/pr-subscribers-flang-semantics

Author: Krzysztof Parzyszek (kparzysz)

Changes

This actually simplifies the AST node for the schedule clause: the two allowed modifiers can be easily classified as the ordering-modifier and the chunk-modifier during parsing without the need to create additional classes.


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

14 Files Affected:

  • (modified) flang/examples/FeatureList/FeatureList.cpp (+6-7)
  • (modified) flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp (+7-3)
  • (modified) flang/examples/FlangOmpReport/FlangOmpReportVisitor.h (+2-1)
  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+9-8)
  • (modified) flang/include/flang/Parser/parse-tree.h (+54-27)
  • (modified) flang/include/flang/Semantics/openmp-modifiers.h (+6)
  • (modified) flang/lib/Lower/OpenMP/Clauses.cpp (+24-51)
  • (modified) flang/lib/Lower/OpenMP/Clauses.h (+10)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+38-33)
  • (modified) flang/lib/Parser/unparse.cpp (+11-12)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+23-35)
  • (modified) flang/lib/Semantics/check-omp-structure.h (-2)
  • (modified) flang/lib/Semantics/openmp-modifiers.cpp (+48)
  • (modified) flang/test/Parser/OpenMP/order-clause01.f90 (+25-25)
diff --git a/flang/examples/FeatureList/FeatureList.cpp b/flang/examples/FeatureList/FeatureList.cpp
index 753ecb918a9ccb..e1c42586c62c94 100644
--- a/flang/examples/FeatureList/FeatureList.cpp
+++ b/flang/examples/FeatureList/FeatureList.cpp
@@ -505,9 +505,9 @@ struct NodeVisitor {
   READ_FEATURE(OmpObject)
   READ_FEATURE(OmpObjectList)
   READ_FEATURE(OmpOrderClause)
-  READ_FEATURE(OmpOrderClause::Type)
+  READ_FEATURE(OmpOrderClause::Ordering)
   READ_FEATURE(OmpOrderModifier)
-  READ_FEATURE(OmpOrderModifier::Kind)
+  READ_FEATURE(OmpOrderModifier::Value)
   READ_FEATURE(OmpProcBindClause)
   READ_FEATURE(OmpProcBindClause::Type)
   READ_FEATURE(OmpReductionClause)
@@ -527,11 +527,10 @@ struct NodeVisitor {
   READ_FEATURE(OmpDeviceClause::DeviceModifier)
   READ_FEATURE(OmpDeviceTypeClause)
   READ_FEATURE(OmpDeviceTypeClause::Type)
-  READ_FEATURE(OmpScheduleModifier)
-  READ_FEATURE(OmpScheduleModifier::Modifier1)
-  READ_FEATURE(OmpScheduleModifier::Modifier2)
-  READ_FEATURE(OmpScheduleModifierType)
-  READ_FEATURE(OmpScheduleModifierType::ModType)
+  READ_FEATURE(OmpChunkModifier)
+  READ_FEATURE(OmpChunkModifier::Value)
+  READ_FEATURE(OmpOrderingModifier)
+  READ_FEATURE(OmpOrderingModifier::Value)
   READ_FEATURE(OmpSectionBlocks)
   READ_FEATURE(OmpSectionsDirective)
   READ_FEATURE(OmpSimpleStandaloneDirective)
diff --git a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp
index a9ff163f8243ce..a3d9b0cfdc79b8 100644
--- a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp
+++ b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp
@@ -213,14 +213,18 @@ void OpenMPCounterVisitor::Post(const OmpVariableCategory::Value &c) {
       "variable_category=" + std::string{OmpVariableCategory::EnumToString(c)} +
       ";";
 }
-void OpenMPCounterVisitor::Post(const OmpScheduleModifierType::ModType &c) {
+void OpenMPCounterVisitor::Post(const OmpChunkModifier::Value &c) {
   clauseDetails +=
-      "modifier=" + std::string{OmpScheduleModifierType::EnumToString(c)} + ";";
+      "modifier=" + std::string{OmpChunkModifier::EnumToString(c)} + ";";
 }
 void OpenMPCounterVisitor::Post(const OmpLinearModifier::Value &c) {
   clauseDetails +=
       "modifier=" + std::string{OmpLinearModifier::EnumToString(c)} + ";";
 }
+void OpenMPCounterVisitor::Post(const OmpOrderingModifier::Value &c) {
+  clauseDetails +=
+      "modifier=" + std::string{OmpOrderingModifier::EnumToString(c)} + ";";
+}
 void OpenMPCounterVisitor::Post(const OmpTaskDependenceType::Value &c) {
   clauseDetails +=
       "type=" + std::string{OmpTaskDependenceType::EnumToString(c)} + ";";
@@ -228,7 +232,7 @@ void OpenMPCounterVisitor::Post(const OmpTaskDependenceType::Value &c) {
 void OpenMPCounterVisitor::Post(const OmpMapClause::Type &c) {
   clauseDetails += "type=" + std::string{OmpMapClause::EnumToString(c)} + ";";
 }
-void OpenMPCounterVisitor::Post(const OmpScheduleClause::ScheduleType &c) {
+void OpenMPCounterVisitor::Post(const OmpScheduleClause::Kind &c) {
   clauseDetails +=
       "type=" + std::string{OmpScheduleClause::EnumToString(c)} + ";";
 }
diff --git a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h
index 83bd3644577e1c..608cb5a2241b83 100644
--- a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h
+++ b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h
@@ -71,8 +71,9 @@ struct OpenMPCounterVisitor {
   void Post(const OmpDefaultmapClause::ImplicitBehavior &c);
   void Post(const OmpVariableCategory::Value &c);
   void Post(const OmpDeviceTypeClause::Type &c);
-  void Post(const OmpScheduleModifierType::ModType &c);
+  void Post(const OmpChunkModifier::Value &c);
   void Post(const OmpLinearModifier::Value &c);
+  void Post(const OmpOrderingModifier::Value &c);
   void Post(const OmpTaskDependenceType::Value &c);
   void Post(const OmpMapClause::Type &c);
   void Post(const OmpScheduleClause::ScheduleType &c);
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index e9c149758c1493..6d1e7329d5cce8 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -559,9 +559,10 @@ class ParseTreeDumper {
   NODE(parser, OmpObject)
   NODE(parser, OmpObjectList)
   NODE(parser, OmpOrderClause)
-  NODE_ENUM(OmpOrderClause, Type)
+  NODE(OmpOrderClause, Modifier)
+  NODE_ENUM(OmpOrderClause, Ordering)
   NODE(parser, OmpOrderModifier)
-  NODE_ENUM(OmpOrderModifier, Kind)
+  NODE_ENUM(OmpOrderModifier, Value)
   NODE(parser, OmpGrainsizeClause)
   NODE_ENUM(OmpGrainsizeClause, Prescriptiveness)
   NODE(parser, OmpNumTasksClause)
@@ -585,17 +586,17 @@ class ParseTreeDumper {
   NODE(OmpAllocateClause::AllocateModifier, ComplexModifier)
   NODE(OmpAllocateClause::AllocateModifier, Align)
   NODE(parser, OmpScheduleClause)
-  NODE_ENUM(OmpScheduleClause, ScheduleType)
+  NODE(OmpScheduleClause, Modifier)
+  NODE_ENUM(OmpScheduleClause, Kind)
   NODE(parser, OmpDeviceClause)
   NODE_ENUM(OmpDeviceClause, DeviceModifier)
   NODE(parser, OmpDeviceTypeClause)
   NODE_ENUM(OmpDeviceTypeClause, Type)
   NODE(parser, OmpUpdateClause)
-  NODE(parser, OmpScheduleModifier)
-  NODE(OmpScheduleModifier, Modifier1)
-  NODE(OmpScheduleModifier, Modifier2)
-  NODE(parser, OmpScheduleModifierType)
-  NODE_ENUM(OmpScheduleModifierType, ModType)
+  NODE(parser, OmpChunkModifier)
+  NODE_ENUM(OmpChunkModifier, Value)
+  NODE(parser, OmpOrderingModifier)
+  NODE_ENUM(OmpOrderingModifier, Value)
   NODE(parser, OmpSectionBlocks)
   NODE(parser, OmpSectionsDirective)
   NODE(parser, OmpSimpleStandaloneDirective)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 32ebaa7fbffcae..de179f47be8fca 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3457,6 +3457,17 @@ inline namespace modifier {
 //   ENUM_CLASS(Value, Keyword1, Keyword2);
 // };
 
+// Ref: [5.2:252-254]
+//
+// chunk-modifier ->
+//    SIMD                                          // since 5.2
+//
+// Prior to 5.2 "chunk-modifier" was a part of "modifier" on SCHEDULE clause.
+struct OmpChunkModifier {
+  ENUM_CLASS(Value, Simd)
+  WRAPPER_CLASS_BOILERPLATE(OmpChunkModifier, Value);
+};
+
 // Ref: [5.0:47-49], [5.1:49-51], [5.2:67-69]
 //
 // iterator-specifier ->
@@ -3508,6 +3519,30 @@ struct OmpLinearModifier {
   WRAPPER_CLASS_BOILERPLATE(OmpLinearModifier, Value);
 };
 
+// Ref: [4.5:56-63], [5.0:101-109], [5.1:126-133], [5.2:252-254]
+//
+// modifier ->
+//    MONOTONIC | NONMONOTONIC | SIMD               // since 4.5, until 5.1
+// ordering-modifier ->
+//    MONOTONIC | NONMONOTONIC                      // since 5.2
+//
+// Until 5.1, the SCHEDULE clause accepted up to two instances of "modifier".
+// Since 5.2 "modifier" was replaced with "ordering-modifier" and "chunk-
+// modifier".
+struct OmpOrderingModifier {
+  ENUM_CLASS(Value, Monotonic, Nonmonotonic, Simd)
+  WRAPPER_CLASS_BOILERPLATE(OmpOrderingModifier, Value);
+};
+
+// Ref: [5.1:125-126], [5.2:233-234]
+//
+// order-modifier ->
+//    REPRODUCIBLE | UNCONSTRAINED                  // since 5.1
+struct OmpOrderModifier {
+  ENUM_CLASS(Value, Reproducible, Unconstrained)
+  WRAPPER_CLASS_BOILERPLATE(OmpOrderModifier, Value);
+};
+
 // Ref: [4.5:201-207], [5.0:293-299], [5.1:325-331], [5.2:124]
 //
 // reduction-identifier ->
@@ -3786,16 +3821,16 @@ struct OmpMapClause {
       t;
 };
 
-// 2.9.5 order-clause -> ORDER ([order-modifier :]concurrent)
-struct OmpOrderModifier {
-  ENUM_CLASS(Kind, Reproducible, Unconstrained)
-  WRAPPER_CLASS_BOILERPLATE(OmpOrderModifier, Kind);
-};
-
+// Ref: [5.0:101-109], [5.1:126-134], [5.2:233-234]
+//
+// order-clause ->
+//    ORDER(CONCURRENT) |                           // since 5.0
+//    ORDER([order-modifier:] CONCURRENT)           // since 5.1
 struct OmpOrderClause {
   TUPLE_CLASS_BOILERPLATE(OmpOrderClause);
-  ENUM_CLASS(Type, Concurrent)
-  std::tuple<std::optional<OmpOrderModifier>, Type> t;
+  ENUM_CLASS(Ordering, Concurrent)
+  MODIFIER_BOILERPLATE(OmpOrderModifier);
+  std::tuple<MODIFIERS(), Ordering> t;
 };
 
 // 2.5 proc-bind-clause -> PROC_BIND (MASTER | CLOSE | SPREAD)
@@ -3816,27 +3851,19 @@ struct OmpReductionClause {
   std::tuple<MODIFIERS(), OmpObjectList> t;
 };
 
-// 2.7.1 sched-modifier -> MONOTONIC | NONMONOTONIC | SIMD
-struct OmpScheduleModifierType {
-  ENUM_CLASS(ModType, Monotonic, Nonmonotonic, Simd)
-  WRAPPER_CLASS_BOILERPLATE(OmpScheduleModifierType, ModType);
-};
-
-struct OmpScheduleModifier {
-  TUPLE_CLASS_BOILERPLATE(OmpScheduleModifier);
-  WRAPPER_CLASS(Modifier1, OmpScheduleModifierType);
-  WRAPPER_CLASS(Modifier2, OmpScheduleModifierType);
-  std::tuple<Modifier1, std::optional<Modifier2>> t;
-};
-
-// 2.7.1 schedule-clause -> SCHEDULE ([sched-modifier1] [, sched-modifier2]:]
-//                                    kind[, chunk_size])
+// Ref: [4.5:56-63], [5.0:101-109], [5.1:126-133], [5.2:252-254]
+//
+// schedule-clause ->
+//    SCHEDULE([modifier[, modifier]:]
+//        kind[, chunk-size])                       // since 4.5, until 5.1
+// schedule-clause ->
+//    SCHEDULE([ordering-modifier], chunk-modifier],
+//        kind[, chunk_size])                       // since 5.2
 struct OmpScheduleClause {
   TUPLE_CLASS_BOILERPLATE(OmpScheduleClause);
-  ENUM_CLASS(ScheduleType, Static, Dynamic, Guided, Auto, Runtime)
-  std::tuple<std::optional<OmpScheduleModifier>, ScheduleType,
-      std::optional<ScalarIntExpr>>
-      t;
+  ENUM_CLASS(Kind, Static, Dynamic, Guided, Auto, Runtime)
+  MODIFIER_BOILERPLATE(OmpOrderingModifier, OmpChunkModifier);
+  std::tuple<MODIFIERS(), Kind, std::optional<ScalarIntExpr>> t;
 };
 
 // Ref: [4.5:107-109], [5.0:176-180], [5.1:205-210], [5.2:167-168]
diff --git a/flang/include/flang/Semantics/openmp-modifiers.h b/flang/include/flang/Semantics/openmp-modifiers.h
index 28fec7314cd8b5..fd6bd86fb280d8 100644
--- a/flang/include/flang/Semantics/openmp-modifiers.h
+++ b/flang/include/flang/Semantics/openmp-modifiers.h
@@ -61,6 +61,8 @@ struct OmpModifierDescriptor {
 
 template <typename SpecificTy> const OmpModifierDescriptor &OmpGetDescriptor();
 
+template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpChunkModifier>();
 template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpDependenceType>();
 template <>
@@ -68,6 +70,10 @@ const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpIterator>();
 template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpLinearModifier>();
 template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpOrderModifier>();
+template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpOrderingModifier>();
+template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpReductionIdentifier>();
 template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpReductionModifier>();
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index 4f9e7a7c1d78c3..8639d08827f4ed 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -1109,7 +1109,7 @@ Order make(const parser::OmpClause::Order &inp,
   using wrapped = parser::OmpOrderClause;
 
   CLAUSET_ENUM_CONVERT( //
-      convert1, parser::OmpOrderModifier::Kind, Order::OrderModifier,
+      convert1, parser::OmpOrderModifier::Value, Order::OrderModifier,
       // clang-format off
       MS(Reproducible,   Reproducible)
       MS(Unconstrained,  Unconstrained)
@@ -1117,20 +1117,18 @@ Order make(const parser::OmpClause::Order &inp,
   );
 
   CLAUSET_ENUM_CONVERT( //
-      convert2, wrapped::Type, Order::Ordering,
+      convert2, wrapped::Ordering, Order::Ordering,
       // clang-format off
       MS(Concurrent, Concurrent)
       // clang-format on
   );
 
-  auto &t0 = std::get<std::optional<parser::OmpOrderModifier>>(inp.v.t);
-  auto &t1 = std::get<wrapped::Type>(inp.v.t);
+  auto &mods = semantics::OmpGetModifiers(inp.v);
+  auto *t0 = semantics::OmpGetUniqueModifier<parser::OmpOrderModifier>(mods);
+  auto &t1 = std::get<wrapped::Ordering>(inp.v.t);
 
-  auto convert3 = [&](const parser::OmpOrderModifier &s) {
-    return convert1(s.v);
-  };
-  return Order{
-      {/*OrderModifier=*/maybeApply(convert3, t0), /*Ordering=*/convert2(t1)}};
+  return Order{{/*OrderModifier=*/maybeApplyToV(convert1, t0),
+                /*Ordering=*/convert2(t1)}};
 }
 
 Ordered make(const parser::OmpClause::Ordered &inp,
@@ -1197,10 +1195,10 @@ Reduction make(const parser::OmpClause::Reduction &inp,
   auto *t1 =
       semantics::OmpGetUniqueModifier<parser::OmpReductionIdentifier>(mods);
   auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
+  assert(t1 && "OmpReductionIdentifier is required");
+
   return Reduction{
-      {/*ReductionModifier=*/t0
-           ? std::make_optional<Reduction::ReductionModifier>(convert(t0->v))
-           : std::nullopt,
+      {/*ReductionModifier=*/maybeApplyToV(convert, t0),
        /*ReductionIdentifiers=*/{makeReductionOperator(*t1, semaCtx)},
        /*List=*/makeObjects(t2, semaCtx)}};
 }
@@ -1221,7 +1219,7 @@ Schedule make(const parser::OmpClause::Schedule &inp,
   using wrapped = parser::OmpScheduleClause;
 
   CLAUSET_ENUM_CONVERT( //
-      convert1, wrapped::ScheduleType, Schedule::Kind,
+      convert1, wrapped::Kind, Schedule::Kind,
       // clang-format off
       MS(Static,   Static)
       MS(Dynamic,  Dynamic)
@@ -1232,8 +1230,7 @@ Schedule make(const parser::OmpClause::Schedule &inp,
   );
 
   CLAUSET_ENUM_CONVERT( //
-      convert2, parser::OmpScheduleModifierType::ModType,
-      Schedule::OrderingModifier,
+      convert2, parser::OmpOrderingModifier::Value, Schedule::OrderingModifier,
       // clang-format off
       MS(Monotonic,    Monotonic)
       MS(Nonmonotonic, Nonmonotonic)
@@ -1241,48 +1238,22 @@ Schedule make(const parser::OmpClause::Schedule &inp,
   );
 
   CLAUSET_ENUM_CONVERT( //
-      convert3, parser::OmpScheduleModifierType::ModType,
-      Schedule::ChunkModifier,
+      convert3, parser::OmpChunkModifier::Value, Schedule::ChunkModifier,
       // clang-format off
       MS(Simd, Simd)
       // clang-format on
   );
 
-  auto &t0 = std::get<std::optional<parser::OmpScheduleModifier>>(inp.v.t);
-  auto &t1 = std::get<wrapped::ScheduleType>(inp.v.t);
-  auto &t2 = std::get<std::optional<parser::ScalarIntExpr>>(inp.v.t);
-
-  if (!t0) {
-    return Schedule{{/*Kind=*/convert1(t1), /*OrderingModifier=*/std::nullopt,
-                     /*ChunkModifier=*/std::nullopt,
-                     /*ChunkSize=*/maybeApply(makeExprFn(semaCtx), t2)}};
-  }
-
-  // The members of parser::OmpScheduleModifier correspond to OrderingModifier,
-  // and ChunkModifier, but they can appear in any order.
-  auto &m1 = std::get<parser::OmpScheduleModifier::Modifier1>(t0->t);
-  auto &m2 =
-      std::get<std::optional<parser::OmpScheduleModifier::Modifier2>>(t0->t);
-
-  std::optional<Schedule::OrderingModifier> omod;
-  std::optional<Schedule::ChunkModifier> cmod;
-
-  if (m1.v.v == parser::OmpScheduleModifierType::ModType::Simd) {
-    // m1 is chunk-modifier
-    cmod = convert3(m1.v.v);
-    if (m2)
-      omod = convert2(m2->v.v);
-  } else {
-    // m1 is ordering-modifier
-    omod = convert2(m1.v.v);
-    if (m2)
-      cmod = convert3(m2->v.v);
-  }
+  auto &mods = semantics::OmpGetModifiers(inp.v);
+  auto *t0 = semantics::OmpGetUniqueModifier<parser::OmpOrderingModifier>(mods);
+  auto *t1 = semantics::OmpGetUniqueModifier<parser::OmpChunkModifier>(mods);
+  auto &t2 = std::get<wrapped::Kind>(inp.v.t);
+  auto &t3 = std::get<std::optional<parser::ScalarIntExpr>>(inp.v.t);
 
-  return Schedule{{/*Kind=*/convert1(t1),
-                   /*OrderingModifier=*/omod,
-                   /*ChunkModifier=*/cmod,
-                   /*ChunkSize=*/maybeApply(makeExprFn(semaCtx), t2)}};
+  return Schedule{{/*Kind=*/convert1(t2),
+                   /*OrderingModifier=*/maybeApplyToV(convert2, t0),
+                   /*ChunkModifier=*/maybeApplyToV(convert3, t1),
+                   /*ChunkSize=*/maybeApply(makeExprFn(semaCtx), t3)}};
 }
 
 // SeqCst: empty
@@ -1326,6 +1297,8 @@ TaskReduction make(const parser::OmpClause::TaskReduction &inp,
   auto *t0 =
       semantics::OmpGetUniqueModifier<parser::OmpReductionIdentifier>(mods);
   auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
+  assert(t0 && "OmpReductionIdentifier is required");
+
   return TaskReduction{
       {/*ReductionIdentifiers=*/{makeReductionOperator(*t0, semaCtx)},
        /*List=*/makeObjects(t1, semaCtx)}};
diff --git a/flang/lib/Lower/OpenMP/Clauses.h b/flang/lib/Lower/OpenMP/Clauses.h
index ada50e0488837b..9c053e2213c675 100644
--- a/flang/lib/Lower/OpenMP/Clauses.h
+++ b/flang/lib/Lower/OpenMP/Clauses.h
@@ -153,6 +153,16 @@ std::optional<ResultTy> maybeApply(FuncTy &&func,
   return std::move(func(*arg));
 }
 
+template <
+    typename FuncTy, //
+    typename ArgTy,  //
+    typename ResultTy = std::invoke_result_t<FuncTy, typename ArgTy::Value>>
+std::optional<ResultTy> maybeApplyToV(FuncTy &&func, const ArgTy *arg) {
+  if (!arg)
+    return std::nullopt;
+  return std::move(func(arg->v));
+}
+
 std::optional<Object> getBaseObject(const Object &object,
                                     semantics::SemanticsContext &semaCtx);
 
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index a1d368d73ab83a..ceae20270d13d0 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -228,6 +228,18 @@ TYPE_PARSER(construct<OmpLinearModifier>( //
 TYPE_PARSER(construct<OmpReductionIdentifier>(Parser<DefinedOperator>{}) ||
     construct<OmpReductionIdentifier>(Parser<ProcedureDesignator>{}))
 
+TYPE_PARSER(construct<OmpChunkModifier>( //
+    "SIMD" >> pure(OmpChunkModifier::Value::Simd)))
+
+TYPE_PARSER(construct<OmpOrderModifier>(
+    "REPRODUCIBLE" >> pure(OmpOrderModifier::Value::Reproducible) ||
+    "UNCONSTRAINED" >> pure(OmpOrderModifier::Value::Unconstrained)))
+
+TYPE_PARSER(construct<OmpOrderingModifier>(
+    "MONOTONIC" >> pure(OmpOrderingModifier::Value::Monotonic) ||
+    "NONMONOTONIC" >> pure(OmpOrderingModifier::Value::Nonmonotonic) ||
+    "SIMD" >> pure(OmpOrderingModifier::Value::Simd)))
+
 TYPE_PARSER(construct<OmpReductionModifier>(
     "INSCAN" >> pure(OmpReductionModifier::Value::Inscan) ||
     "TASK" >> pure(OmpReductionModifier::Value::Task) ||
@@ -241,12 +253,6 @@ TYPE_PARSER(construct<OmpTaskDependenceType>(
     "MUTEXINOUTSET" >> pure(OmpTaskDependenceType::Value::Mutexinoutset) ||
     "OUT" >> pure(OmpTaskDependenceType::Value::Out)))
 
-// This could be auto-generated.
-TYPE_PARSER(sourced(construct<OmpReductionClause::Modifier>(sourced(
-    construct<OmpReductionClause::Modifier>(Parser<OmpReductionModifier>{}) ||
-    construct<OmpReductionClause::Modifier>(
-        Parser<OmpReductionIdentifier>{})))))
-
 TYPE_PARSER(construct<OmpVariableCategory>(
     "AGGREGATE" >> pure(OmpVariableCategory::Value::Aggregate) ||
     "ALL"_id >> pure(OmpVariableCategory::Value::All) ||
@@ -254,6 +260,19 @@ TYPE_PARSER(construct<OmpVariableCategory>(
     "POINTER" >> pure(OmpVariableCategory::Value::Pointer) ||
     "SCALAR" >> pure(OmpVariableCategory::Value::Scalar)))
 
+// This could be auto-generated.
+TYPE_PARSER(
+    sourced(construct<OmpOrderClause::Modifier>(Parser<OmpOrderModifier>{})))
+
+TYPE_PARSER(sourced(construct<OmpReductionClause::Modifier>(sourced(
+    construct<OmpReductionClause::Modifier>(Parser<OmpReductionModifier>{}) ||
+    construct<OmpReductionClause::Modifier>(
+        Parser<OmpReductionIdentifier>{})))))
+
+TYPE_PARSER(sourced(construct<OmpScheduleClause::Modifier>(sourced(
+    construct<OmpScheduleClause::Modifier>(Parser<OmpChunkModifier>{}) ||
+    construct<OmpScheduleClause::Modifier>(Parser<OmpOrderingModifier>{})))))
+
 TYPE_PARSER(sourced(
     construct<OmpDefaultmapClause::Modifier>(Parser<OmpVariableCategory>{})))
 
@@ -336,25 +355,16 @@ TYPE_PARSER(construct<OmpDefaultmapClause>(
         "DEFAULT" >> pure(OmpDefaultmapClause::ImplicitBehavior::Default)),
     maybe(":" >> nonemptyList(Parser<OmpDefaultmapClause::Modifier>{}))))
 
-// 2.7.1 SCHEDULE ([modifier1 [, modifier2]:]kind[, chunk_size])
-//       Modifier ->  MONITONIC | NONMONOTONIC | SIMD
-//       kind -> STATIC | D...
[truncated]

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.

LGTM

std::optional<ResultTy> maybeApplyToV(FuncTy &&func, const ArgTy *arg) {
if (!arg)
return std::nullopt;
return std::move(func(arg->v));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't think this std::move is necessary. In the uses I can see here ResultTy is not a reference. Therefore, the function result is a prvalue and so will be moved automatically.

Copy link
Contributor Author

@kparzysz kparzysz Nov 21, 2024

Choose a reason for hiding this comment

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

Thanks. I'll update that at the next opportunity.

Edit: Actually, that's now. This isn't merged yet, and there were conflicts to resolve anyway...

Base automatically changed from users/kparzysz/spr/m03-semantic-checks to main November 21, 2024 19:18
@kparzysz kparzysz merged commit e79cd24 into main Nov 21, 2024
8 checks passed
@kparzysz kparzysz deleted the users/kparzysz/spr/m04-order-schedule branch November 21, 2024 23:50
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