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] Apply modifier representation to semantic checks #116658

Merged
merged 13 commits into from
Nov 21, 2024

Conversation

kparzysz
Copy link
Contributor

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

llvmbot commented Nov 18, 2024

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Krzysztof Parzyszek (kparzysz)

Changes

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


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

15 Files Affected:

  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+6-2)
  • (modified) flang/include/flang/Parser/parse-tree.h (+40-9)
  • (modified) flang/include/flang/Semantics/openmp-modifiers.h (+4)
  • (modified) flang/lib/Lower/OpenMP/Clauses.cpp (+19-14)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+24-16)
  • (modified) flang/lib/Parser/unparse.cpp (+6-9)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+49-34)
  • (modified) flang/lib/Semantics/check-omp-structure.h (+1-2)
  • (modified) flang/lib/Semantics/openmp-modifiers.cpp (+33)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+32-20)
  • (modified) flang/test/Parser/OpenMP/defaultmap-clause.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/defaultmap-unparse.f90 (+8-8)
  • (modified) flang/test/Parser/OpenMP/reduction-modifier.f90 (+3-3)
  • (modified) flang/test/Semantics/OpenMP/combined-constructs.f90 (+6-6)
  • (modified) flang/test/Semantics/OpenMP/defaultmap-clause-v45.f90 (+1-1)
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index df5bf1d8d3200e..9c59ce520a31aa 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -509,9 +509,11 @@ class ParseTreeDumper {
   NODE(parser, OmpDeclareMapperSpecifier)
   NODE(parser, OmpDefaultClause)
   NODE_ENUM(OmpDefaultClause, Type)
+  NODE(parser, OmpVariableCategory)
+  NODE_ENUM(OmpVariableCategory, Value)
   NODE(parser, OmpDefaultmapClause)
   NODE_ENUM(OmpDefaultmapClause, ImplicitBehavior)
-  NODE_ENUM(OmpDefaultmapClause, VariableCategory)
+  NODE(OmpDefaultmapClause, Modifier)
   NODE(parser, OmpDependenceType)
   NODE_ENUM(OmpDependenceType, Value)
   NODE(parser, OmpTaskDependenceType)
@@ -567,8 +569,10 @@ class ParseTreeDumper {
   NODE_ENUM(OmpBindClause, Type)
   NODE(parser, OmpProcBindClause)
   NODE_ENUM(OmpProcBindClause, Type)
-  NODE_ENUM(OmpReductionClause, ReductionModifier)
+  NODE(parser, OmpReductionModifier)
+  NODE_ENUM(OmpReductionModifier, Value)
   NODE(parser, OmpReductionClause)
+  NODE(OmpReductionClause, Modifier)
   NODE(parser, OmpInReductionClause)
   NODE(parser, OmpReductionCombiner)
   NODE(OmpReductionCombiner, FunctionCombiner)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index ef49a36578270e..5b28bcd4e21b80 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3440,6 +3440,16 @@ struct OmpObject {
 
 WRAPPER_CLASS(OmpObjectList, std::list<OmpObject>);
 
+#define MODIFIER_BOILERPLATE(...) \
+  struct Modifier { \
+    using Variant = std::variant<__VA_ARGS__>; \
+    UNION_CLASS_BOILERPLATE(Modifier); \
+    CharBlock source; \
+    Variant u; \
+  }
+
+#define MODIFIERS() std::optional<std::list<Modifier>>
+
 inline namespace modifier {
 // For uniformity, in all keyword modifiers the name of the type defined
 // by ENUM_CLASS is "Value", e.g.
@@ -3505,12 +3515,20 @@ struct OmpLinearModifier {
 //   - |                                            // 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: [5.0:300-302], [5.1:332-334], [5.2:134-137]
+//
+// reduction-modifier ->
+//   DEFAULT | INSCAN | TASK                        // since 5.0
+struct OmpReductionModifier {
+  ENUM_CLASS(Value, Default, Inscan, Task);
+  WRAPPER_CLASS_BOILERPLATE(OmpReductionModifier, Value);
+};
+
 // 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
@@ -3521,6 +3539,17 @@ struct OmpTaskDependenceType {
   ENUM_CLASS(Value, In, Out, Inout, Inoutset, Mutexinoutset, Depobj)
   WRAPPER_CLASS_BOILERPLATE(OmpTaskDependenceType, Value);
 };
+
+// Ref: [4.5:229-230], [5.0:324-325], [5.1:357-358], [5.2:161-162]
+//
+// variable-category ->
+//   SCALAR |                                       // since 4.5
+//   AGGREGATE | ALLOCATABLE | POINTER |            // since 5.0
+//   ALL                                            // since 5.2
+struct OmpVariableCategory {
+  ENUM_CLASS(Value, Aggregate, All, Allocatable, Pointer, Scalar)
+  WRAPPER_CLASS_BOILERPLATE(OmpVariableCategory, Value);
+};
 } // namespace modifier
 
 // --- Clauses
@@ -3578,8 +3607,8 @@ struct OmpDefaultmapClause {
   TUPLE_CLASS_BOILERPLATE(OmpDefaultmapClause);
   ENUM_CLASS(
       ImplicitBehavior, Alloc, To, From, Tofrom, Firstprivate, None, Default)
-  ENUM_CLASS(VariableCategory, All, Scalar, Aggregate, Allocatable, Pointer)
-  std::tuple<ImplicitBehavior, std::optional<VariableCategory>> t;
+  MODIFIER_BOILERPLATE(OmpVariableCategory);
+  std::tuple<ImplicitBehavior, MODIFIERS()> t;
 };
 
 // 2.13.9 iteration-offset -> +/- non-negative-constant
@@ -3771,14 +3800,16 @@ struct OmpProcBindClause {
   WRAPPER_CLASS_BOILERPLATE(OmpProcBindClause, Type);
 };
 
-// 2.15.3.6 reduction-clause -> REDUCTION (reduction-identifier:
-//                                         variable-name-list)
+// Ref: [4.5:201-207], [5.0:300-302], [5.1:332-334], [5.2:134-137]
+//
+// reduction-clause ->
+//    REDUCTION(reduction-identifier: list) |       // since 4.5
+//    REDUCTION([reduction-modifier,]
+//        reduction-identifier: list)               // since 5.0
 struct OmpReductionClause {
   TUPLE_CLASS_BOILERPLATE(OmpReductionClause);
-  ENUM_CLASS(ReductionModifier, Inscan, Task, Default)
-  std::tuple<std::optional<ReductionModifier>, OmpReductionIdentifier,
-      OmpObjectList>
-      t;
+  MODIFIER_BOILERPLATE(OmpReductionModifier, OmpReductionIdentifier);
+  std::tuple<MODIFIERS(), OmpObjectList> t;
 };
 
 // 2.7.1 sched-modifier -> MONOTONIC | NONMONOTONIC | SIMD
diff --git a/flang/include/flang/Semantics/openmp-modifiers.h b/flang/include/flang/Semantics/openmp-modifiers.h
index 6be582761ed687..28fec7314cd8b5 100644
--- a/flang/include/flang/Semantics/openmp-modifiers.h
+++ b/flang/include/flang/Semantics/openmp-modifiers.h
@@ -70,7 +70,11 @@ const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpLinearModifier>();
 template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpReductionIdentifier>();
 template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpReductionModifier>();
+template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpTaskDependenceType>();
+template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpVariableCategory>();
 
 // Explanation of terminology:
 //
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index 5e514b4ba9f0da..3e0a11bc785e33 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -12,6 +12,7 @@
 #include "flang/Evaluate/expression.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/expression.h"
+#include "flang/Semantics/openmp-modifiers.h"
 #include "flang/Semantics/symbol.h"
 
 #include "llvm/Frontend/OpenMP/OMPConstants.h"
@@ -571,7 +572,8 @@ Defaultmap make(const parser::OmpClause::Defaultmap &inp,
   );
 
   CLAUSET_ENUM_CONVERT( //
-      convert2, wrapped::VariableCategory, Defaultmap::VariableCategory,
+      convert2, parser::OmpVariableCategory::Value,
+      Defaultmap::VariableCategory,
       // clang-format off
       MS(Aggregate,    Aggregate)
       MS(All,          All)
@@ -581,10 +583,11 @@ Defaultmap make(const parser::OmpClause::Defaultmap &inp,
       // clang-format on
   );
 
+  auto &mods{semantics::OmpGetModifiers(inp.v)};
   auto &t0 = std::get<wrapped::ImplicitBehavior>(inp.v.t);
-  auto &t1 = std::get<std::optional<wrapped::VariableCategory>>(inp.v.t);
+  auto *t1 = semantics::OmpGetUniqueModifier<parser::OmpVariableCategory>(mods);
 
-  auto category = t1 ? convert2(*t1) : Defaultmap::VariableCategory::All;
+  auto category = t1 ? convert2(t1->v) : Defaultmap::VariableCategory::All;
   return Defaultmap{{/*ImplicitBehavior=*/convert1(t0),
                      /*VariableCategory=*/category}};
 }
@@ -1173,10 +1176,9 @@ ProcBind make(const parser::OmpClause::ProcBind &inp,
 Reduction make(const parser::OmpClause::Reduction &inp,
                semantics::SemanticsContext &semaCtx) {
   // inp.v -> parser::OmpReductionClause
-  using wrapped = parser::OmpReductionClause;
-
   CLAUSET_ENUM_CONVERT( //
-      convert, wrapped::ReductionModifier, Reduction::ReductionModifier,
+      convert, parser::OmpReductionModifier::Value,
+      Reduction::ReductionModifier,
       // clang-format off
       MS(Inscan,  Inscan)
       MS(Task,    Task)
@@ -1184,16 +1186,17 @@ Reduction make(const parser::OmpClause::Reduction &inp,
       // clang-format on
   );
 
-  auto &t0 =
-      std::get<std::optional<parser::OmpReductionClause::ReductionModifier>>(
-          inp.v.t);
-  auto &t1 = std::get<parser::OmpReductionIdentifier>(inp.v.t);
+  auto &mods = semantics::OmpGetModifiers(inp.v);
+  auto *t0 =
+      semantics::OmpGetUniqueModifier<parser::OmpReductionModifier>(mods);
+  auto *t1 =
+      semantics::OmpGetUniqueModifier<parser::OmpReductionIdentifier>(mods);
   auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
   return Reduction{
       {/*ReductionModifier=*/t0
-           ? std::make_optional<Reduction::ReductionModifier>(convert(*t0))
+           ? std::make_optional<Reduction::ReductionModifier>(convert(t0->v))
            : std::nullopt,
-       /*ReductionIdentifiers=*/{makeReductionOperator(t1, semaCtx)},
+       /*ReductionIdentifiers=*/{makeReductionOperator(*t1, semaCtx)},
        /*List=*/makeObjects(t2, semaCtx)}};
 }
 
@@ -1314,10 +1317,12 @@ 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::OmpReductionIdentifier>(inp.v.t);
+  auto &mods = semantics::OmpGetModifiers(inp.v);
+  auto *t0 =
+      semantics::OmpGetUniqueModifier<parser::OmpReductionIdentifier>(mods);
   auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
   return TaskReduction{
-      {/*ReductionIdentifiers=*/{makeReductionOperator(t0, semaCtx)},
+      {/*ReductionIdentifiers=*/{makeReductionOperator(*t0, semaCtx)},
        /*List=*/makeObjects(t1, semaCtx)}};
 }
 
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index b4d45873abb3ec..063201fc86ca41 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -229,6 +229,11 @@ TYPE_PARSER(construct<OmpLinearModifier>( //
 TYPE_PARSER(construct<OmpReductionIdentifier>(Parser<DefinedOperator>{}) ||
     construct<OmpReductionIdentifier>(Parser<ProcedureDesignator>{}))
 
+TYPE_PARSER(construct<OmpReductionModifier>(
+    "INSCAN" >> pure(OmpReductionModifier::Value::Inscan) ||
+    "TASK" >> pure(OmpReductionModifier::Value::Task) ||
+    "DEFAULT" >> pure(OmpReductionModifier::Value::Default)))
+
 TYPE_PARSER(construct<OmpTaskDependenceType>(
     "DEPOBJ" >> pure(OmpTaskDependenceType::Value::Depobj) ||
     "IN"_id >> pure(OmpTaskDependenceType::Value::In) ||
@@ -237,6 +242,22 @@ 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) ||
+    "ALLOCATABLE" >> pure(OmpVariableCategory::Value::Allocatable) ||
+    "POINTER" >> pure(OmpVariableCategory::Value::Pointer) ||
+    "SCALAR" >> pure(OmpVariableCategory::Value::Scalar)))
+
+TYPE_PARSER(sourced(construct<OmpDefaultmapClause::Modifier>(
+    Parser<OmpVariableCategory>{})))
+
 // --- Parsers for clauses --------------------------------------------
 
 // [5.0] 2.10.1 affinity([aff-modifier:] locator-list)
@@ -309,16 +330,7 @@ TYPE_PARSER(construct<OmpDefaultmapClause>(
             pure(OmpDefaultmapClause::ImplicitBehavior::Firstprivate) ||
         "NONE" >> pure(OmpDefaultmapClause::ImplicitBehavior::None) ||
         "DEFAULT" >> pure(OmpDefaultmapClause::ImplicitBehavior::Default)),
-    maybe(":" >>
-        construct<OmpDefaultmapClause::VariableCategory>(
-            "ALL"_id >> pure(OmpDefaultmapClause::VariableCategory::All) ||
-            "SCALAR" >> pure(OmpDefaultmapClause::VariableCategory::Scalar) ||
-            "AGGREGATE" >>
-                pure(OmpDefaultmapClause::VariableCategory::Aggregate) ||
-            "ALLOCATABLE" >>
-                pure(OmpDefaultmapClause::VariableCategory::Allocatable) ||
-            "POINTER" >>
-                pure(OmpDefaultmapClause::VariableCategory::Pointer)))))
+    maybe(":" >> nonemptyList(Parser<OmpDefaultmapClause::Modifier>{}))))
 
 // 2.7.1 SCHEDULE ([modifier1 [, modifier2]:]kind[, chunk_size])
 //       Modifier ->  MONITONIC | NONMONOTONIC | SIMD
@@ -375,12 +387,8 @@ TYPE_PARSER(construct<OmpIfClause>(
     scalarLogicalExpr))
 
 TYPE_PARSER(construct<OmpReductionClause>(
-    maybe(
-        ("INSCAN" >> pure(OmpReductionClause::ReductionModifier::Inscan) ||
-            "TASK" >> pure(OmpReductionClause::ReductionModifier::Task) ||
-            "DEFAULT" >> pure(OmpReductionClause::ReductionModifier::Default)) /
-        ","),
-    Parser<OmpReductionIdentifier>{} / ":", Parser<OmpObjectList>{}))
+    maybe(nonemptyList(Parser<OmpReductionClause::Modifier>{}) / ":"),
+    Parser<OmpObjectList>{}))
 
 // OMP 5.0 2.19.5.6 IN_REDUCTION (reduction-identifier: variable-name-list)
 TYPE_PARSER(construct<OmpInReductionClause>(
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index b2a0256cb58d0c..e06f4dea80f488 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2179,10 +2179,8 @@ class UnparseVisitor {
     Walk(":", x.step);
   }
   void Unparse(const OmpReductionClause &x) {
-    Walk(std::get<std::optional<OmpReductionClause::ReductionModifier>>(x.t),
-        ",");
-    Walk(std::get<OmpReductionIdentifier>(x.t));
-    Put(":");
+    using Modifier = OmpReductionClause::Modifier;
+    Walk(std::get<std::optional<std::list<Modifier>>>(x.t), ":");
     Walk(std::get<OmpObjectList>(x.t));
   }
   void Unparse(const OmpDetachClause &x) { Walk(x.v); }
@@ -2246,9 +2244,9 @@ class UnparseVisitor {
     Walk(std::get<OmpObjectList>(x.t));
   }
   void Unparse(const OmpDefaultmapClause &x) {
+    using Modifier = OmpDefaultmapClause::Modifier;
     Walk(std::get<OmpDefaultmapClause::ImplicitBehavior>(x.t));
-    Walk(":",
-        std::get<std::optional<OmpDefaultmapClause::VariableCategory>>(x.t));
+    Walk(":", std::get<std::optional<std::list<Modifier>>>(x.t));
   }
   void Unparse(const OmpToClause &x) {
     auto &expect{
@@ -2896,7 +2894,7 @@ class UnparseVisitor {
   WALK_NESTED_ENUM(OmpProcBindClause, Type) // OMP PROC_BIND
   WALK_NESTED_ENUM(OmpDefaultClause, Type) // OMP DEFAULT
   WALK_NESTED_ENUM(OmpDefaultmapClause, ImplicitBehavior) // OMP DEFAULTMAP
-  WALK_NESTED_ENUM(OmpDefaultmapClause, VariableCategory) // OMP DEFAULTMAP
+  WALK_NESTED_ENUM(OmpVariableCategory, Value) // OMP variable-category
   WALK_NESTED_ENUM(
       OmpLastprivateClause, LastprivateModifier) // OMP lastprivate-modifier
   WALK_NESTED_ENUM(OmpScheduleModifierType, ModType) // OMP schedule-modifier
@@ -2905,8 +2903,7 @@ class UnparseVisitor {
   WALK_NESTED_ENUM(OmpScheduleClause, ScheduleType) // OMP schedule-type
   WALK_NESTED_ENUM(OmpDeviceClause, DeviceModifier) // OMP device modifier
   WALK_NESTED_ENUM(OmpDeviceTypeClause, Type) // OMP DEVICE_TYPE
-  WALK_NESTED_ENUM(
-      OmpReductionClause, ReductionModifier) // OMP reduction-modifier
+  WALK_NESTED_ENUM(OmpReductionModifier, Value) // OMP reduction-modifier
   WALK_NESTED_ENUM(OmpFromClause, Expectation) // OMP motion-expectation
   WALK_NESTED_ENUM(OmpIfClause, DirectiveNameModifier) // OMP directive-modifier
   WALK_NESTED_ENUM(OmpCancelType, Type) // OMP cancel-type
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 9cac652216fcf2..079d0fd17bfac1 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -11,6 +11,7 @@
 #include "flang/Evaluate/check-expression.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/expression.h"
+#include "flang/Semantics/openmp-modifiers.h"
 #include "flang/Semantics/tools.h"
 #include <variant>
 
@@ -195,7 +196,7 @@ bool OmpStructureChecker::CheckAllowedClause(llvmOmpClause clause) {
 
   if (!llvm::omp::isAllowedClauseForDirective(dir, clause, version)) {
     unsigned allowedInVersion{[&] {
-      for (unsigned v : {45, 50, 51, 52, 60}) {
+      for (unsigned v : llvm::omp::getOpenMPVersions()) {
         if (v <= version) {
           continue;
         }
@@ -979,12 +980,14 @@ void OmpStructureChecker::Leave(const parser::OpenMPLoopConstruct &x) {
   // constructs inside LOOP may add the relevant information. Scan reduction is
   // supported only in loop constructs, so same checks are not applicable to
   // other directives.
+  using ReductionModifier = parser::OmpReductionModifier;
   for (const auto &clause : clauseList.v) {
     if (const auto *reductionClause{
             std::get_if<parser::OmpClause::Reduction>(&clause.u)}) {
-      const auto &maybeModifier{
-          std::get<std::optional<ReductionModifier>>(reductionClause->v.t)};
-      if (maybeModifier && *maybeModifier == ReductionModifier::Inscan) {
+      auto &modifiers{OmpGetModifiers(reductionClause->v)};
+      auto *maybeModifier{OmpGetUniqueModifier<ReductionModifier>(modifiers)};
+      if (maybeModifier &&
+          maybeModifier->v == ReductionModifier::Value::Inscan) {
         const auto &objectList{
             std::get<parser::OmpObjectList>(reductionClause->v.t)};
         auto checkReductionSymbolInScan = [&](const parser::Name *name) {
@@ -2850,20 +2853,27 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Destroy &x) {
 
 void OmpStructureChecker::Enter(const parser::OmpClause::Reduction &x) {
   CheckAllowedClause(llvm::omp::Clause::OMPC_reduction);
-  if (CheckReductionOperators(x)) {
-    CheckReductionTypeList(x);
-  }
-  if (const auto &maybeModifier{
-          std::get<std::optional<ReductionModifier>>(x.v.t)}) {
-    const ReductionModifier modifier{*maybeModifier};
-    CheckReductionModifier(modifier);
+  if (OmpVerifyModifiers(x.v, GetContext().clauseSource, context_)) {
+    if (CheckReductionOperators(x)) {
+      CheckReductionTypeList(x);
+    }
+    auto &modifiers{OmpGetModifiers(x.v)};
+    using ReductionModifier = parser::OmpReductionModifier;
+    if (auto *maybeModifier{
+            OmpGetUniqueModifier<ReductionModifier>(modifiers)}) {
+      CheckReductionModifier(*maybeModifier);
+    }
   }
 }
 
 bool OmpStructureChecker::CheckReductionOperators(
     const parser::OmpClause::Reduction &x) {
-
-  const auto &definedOp{std::get<parser::OmpReductionIdentifier>(x.v.t)};
+  auto &modifiers{OmpGetModifiers(x.v)};
+  const auto *definedOp{
+      OmpGetUniqueModifier<parser::OmpReductionIdentifier>(modifiers)};
+  if (!definedOp) {
+    return false;
+  }
   bool ok = false;
   common::visit(
       common::visitors{
@@ -2896,7 +2906,7 @@ bool OmpStructureChecker::CheckReductionOperators(
             }
           },
       },
-      definedOp.u);
+      definedOp->u);
 
   return ok;
 }
@@ -2928,7 +2938,12 @@ bool OmpStructureChecker::CheckIntrinsicOperator(
 
 static bool IsReductionAllowedForType(
     const parser::OmpClause::Reduction &x, const DeclTypeSpec &type) {
-  const auto &definedOp{std::get<parser::OmpReductionIdentifier>(x.v.t)};
+  auto &modifiers{OmpGetModifiers(x.v)};
+  const auto *definedOp{
+      OmpGetUniqueModifier<parser::OmpReductionIdentifier>(modifiers)};
+  if (!definedOp) {
+    return false;
+  }
   // TODO: user defined reduction operators. Just allow everything for now.
   bool ok{true};
 
@@ -3002,7 +3017,7 @@ static bool IsReductionAllowedForType(
             }
           },
       },
-      definedOp.u);
+      definedOp->u);
 
   return ok;
 }
@@ -3035,8 +3050,9 @@ void OmpStructureChecker::CheckReductionTypeList(
 }
 
 void OmpStructureChecker::CheckReductionModifier(
-    const ReductionModifier &modifier) {
-  if (modifier == ReductionModifier::Default) {
+    const parser::OmpReductionModifier &modifier) {
+  using ReductionModifier = parser::OmpReductionModifier;
+  if (modifier.v == ReductionModifier::Value::Default) {
     // The default one is always ok.
     return;
   }
@@ -3049,7 +3065,7 @@ void OmpStructureChecker::CheckReductionModifier(
     context_.Say(GetContext().clauseSource,
         "REDUCTION modifier on LOOP directive must be DEFAULT"_err_en_US);
   }
-  if (modifier == ReductionModifier::Task) {
+  if (modifier.v == ReductionModifier::Value::Task) {
     // "Task" is only allowed on...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Nov 18, 2024

@llvm/pr-subscribers-flang-parser

Author: Krzysztof Parzyszek (kparzysz)

Changes

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


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

15 Files Affected:

  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+6-2)
  • (modified) flang/include/flang/Parser/parse-tree.h (+40-9)
  • (modified) flang/include/flang/Semantics/openmp-modifiers.h (+4)
  • (modified) flang/lib/Lower/OpenMP/Clauses.cpp (+19-14)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+24-16)
  • (modified) flang/lib/Parser/unparse.cpp (+6-9)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+49-34)
  • (modified) flang/lib/Semantics/check-omp-structure.h (+1-2)
  • (modified) flang/lib/Semantics/openmp-modifiers.cpp (+33)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+32-20)
  • (modified) flang/test/Parser/OpenMP/defaultmap-clause.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/defaultmap-unparse.f90 (+8-8)
  • (modified) flang/test/Parser/OpenMP/reduction-modifier.f90 (+3-3)
  • (modified) flang/test/Semantics/OpenMP/combined-constructs.f90 (+6-6)
  • (modified) flang/test/Semantics/OpenMP/defaultmap-clause-v45.f90 (+1-1)
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index df5bf1d8d3200e..9c59ce520a31aa 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -509,9 +509,11 @@ class ParseTreeDumper {
   NODE(parser, OmpDeclareMapperSpecifier)
   NODE(parser, OmpDefaultClause)
   NODE_ENUM(OmpDefaultClause, Type)
+  NODE(parser, OmpVariableCategory)
+  NODE_ENUM(OmpVariableCategory, Value)
   NODE(parser, OmpDefaultmapClause)
   NODE_ENUM(OmpDefaultmapClause, ImplicitBehavior)
-  NODE_ENUM(OmpDefaultmapClause, VariableCategory)
+  NODE(OmpDefaultmapClause, Modifier)
   NODE(parser, OmpDependenceType)
   NODE_ENUM(OmpDependenceType, Value)
   NODE(parser, OmpTaskDependenceType)
@@ -567,8 +569,10 @@ class ParseTreeDumper {
   NODE_ENUM(OmpBindClause, Type)
   NODE(parser, OmpProcBindClause)
   NODE_ENUM(OmpProcBindClause, Type)
-  NODE_ENUM(OmpReductionClause, ReductionModifier)
+  NODE(parser, OmpReductionModifier)
+  NODE_ENUM(OmpReductionModifier, Value)
   NODE(parser, OmpReductionClause)
+  NODE(OmpReductionClause, Modifier)
   NODE(parser, OmpInReductionClause)
   NODE(parser, OmpReductionCombiner)
   NODE(OmpReductionCombiner, FunctionCombiner)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index ef49a36578270e..5b28bcd4e21b80 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3440,6 +3440,16 @@ struct OmpObject {
 
 WRAPPER_CLASS(OmpObjectList, std::list<OmpObject>);
 
+#define MODIFIER_BOILERPLATE(...) \
+  struct Modifier { \
+    using Variant = std::variant<__VA_ARGS__>; \
+    UNION_CLASS_BOILERPLATE(Modifier); \
+    CharBlock source; \
+    Variant u; \
+  }
+
+#define MODIFIERS() std::optional<std::list<Modifier>>
+
 inline namespace modifier {
 // For uniformity, in all keyword modifiers the name of the type defined
 // by ENUM_CLASS is "Value", e.g.
@@ -3505,12 +3515,20 @@ struct OmpLinearModifier {
 //   - |                                            // 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: [5.0:300-302], [5.1:332-334], [5.2:134-137]
+//
+// reduction-modifier ->
+//   DEFAULT | INSCAN | TASK                        // since 5.0
+struct OmpReductionModifier {
+  ENUM_CLASS(Value, Default, Inscan, Task);
+  WRAPPER_CLASS_BOILERPLATE(OmpReductionModifier, Value);
+};
+
 // 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
@@ -3521,6 +3539,17 @@ struct OmpTaskDependenceType {
   ENUM_CLASS(Value, In, Out, Inout, Inoutset, Mutexinoutset, Depobj)
   WRAPPER_CLASS_BOILERPLATE(OmpTaskDependenceType, Value);
 };
+
+// Ref: [4.5:229-230], [5.0:324-325], [5.1:357-358], [5.2:161-162]
+//
+// variable-category ->
+//   SCALAR |                                       // since 4.5
+//   AGGREGATE | ALLOCATABLE | POINTER |            // since 5.0
+//   ALL                                            // since 5.2
+struct OmpVariableCategory {
+  ENUM_CLASS(Value, Aggregate, All, Allocatable, Pointer, Scalar)
+  WRAPPER_CLASS_BOILERPLATE(OmpVariableCategory, Value);
+};
 } // namespace modifier
 
 // --- Clauses
@@ -3578,8 +3607,8 @@ struct OmpDefaultmapClause {
   TUPLE_CLASS_BOILERPLATE(OmpDefaultmapClause);
   ENUM_CLASS(
       ImplicitBehavior, Alloc, To, From, Tofrom, Firstprivate, None, Default)
-  ENUM_CLASS(VariableCategory, All, Scalar, Aggregate, Allocatable, Pointer)
-  std::tuple<ImplicitBehavior, std::optional<VariableCategory>> t;
+  MODIFIER_BOILERPLATE(OmpVariableCategory);
+  std::tuple<ImplicitBehavior, MODIFIERS()> t;
 };
 
 // 2.13.9 iteration-offset -> +/- non-negative-constant
@@ -3771,14 +3800,16 @@ struct OmpProcBindClause {
   WRAPPER_CLASS_BOILERPLATE(OmpProcBindClause, Type);
 };
 
-// 2.15.3.6 reduction-clause -> REDUCTION (reduction-identifier:
-//                                         variable-name-list)
+// Ref: [4.5:201-207], [5.0:300-302], [5.1:332-334], [5.2:134-137]
+//
+// reduction-clause ->
+//    REDUCTION(reduction-identifier: list) |       // since 4.5
+//    REDUCTION([reduction-modifier,]
+//        reduction-identifier: list)               // since 5.0
 struct OmpReductionClause {
   TUPLE_CLASS_BOILERPLATE(OmpReductionClause);
-  ENUM_CLASS(ReductionModifier, Inscan, Task, Default)
-  std::tuple<std::optional<ReductionModifier>, OmpReductionIdentifier,
-      OmpObjectList>
-      t;
+  MODIFIER_BOILERPLATE(OmpReductionModifier, OmpReductionIdentifier);
+  std::tuple<MODIFIERS(), OmpObjectList> t;
 };
 
 // 2.7.1 sched-modifier -> MONOTONIC | NONMONOTONIC | SIMD
diff --git a/flang/include/flang/Semantics/openmp-modifiers.h b/flang/include/flang/Semantics/openmp-modifiers.h
index 6be582761ed687..28fec7314cd8b5 100644
--- a/flang/include/flang/Semantics/openmp-modifiers.h
+++ b/flang/include/flang/Semantics/openmp-modifiers.h
@@ -70,7 +70,11 @@ const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpLinearModifier>();
 template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpReductionIdentifier>();
 template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpReductionModifier>();
+template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpTaskDependenceType>();
+template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpVariableCategory>();
 
 // Explanation of terminology:
 //
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index 5e514b4ba9f0da..3e0a11bc785e33 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -12,6 +12,7 @@
 #include "flang/Evaluate/expression.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/expression.h"
+#include "flang/Semantics/openmp-modifiers.h"
 #include "flang/Semantics/symbol.h"
 
 #include "llvm/Frontend/OpenMP/OMPConstants.h"
@@ -571,7 +572,8 @@ Defaultmap make(const parser::OmpClause::Defaultmap &inp,
   );
 
   CLAUSET_ENUM_CONVERT( //
-      convert2, wrapped::VariableCategory, Defaultmap::VariableCategory,
+      convert2, parser::OmpVariableCategory::Value,
+      Defaultmap::VariableCategory,
       // clang-format off
       MS(Aggregate,    Aggregate)
       MS(All,          All)
@@ -581,10 +583,11 @@ Defaultmap make(const parser::OmpClause::Defaultmap &inp,
       // clang-format on
   );
 
+  auto &mods{semantics::OmpGetModifiers(inp.v)};
   auto &t0 = std::get<wrapped::ImplicitBehavior>(inp.v.t);
-  auto &t1 = std::get<std::optional<wrapped::VariableCategory>>(inp.v.t);
+  auto *t1 = semantics::OmpGetUniqueModifier<parser::OmpVariableCategory>(mods);
 
-  auto category = t1 ? convert2(*t1) : Defaultmap::VariableCategory::All;
+  auto category = t1 ? convert2(t1->v) : Defaultmap::VariableCategory::All;
   return Defaultmap{{/*ImplicitBehavior=*/convert1(t0),
                      /*VariableCategory=*/category}};
 }
@@ -1173,10 +1176,9 @@ ProcBind make(const parser::OmpClause::ProcBind &inp,
 Reduction make(const parser::OmpClause::Reduction &inp,
                semantics::SemanticsContext &semaCtx) {
   // inp.v -> parser::OmpReductionClause
-  using wrapped = parser::OmpReductionClause;
-
   CLAUSET_ENUM_CONVERT( //
-      convert, wrapped::ReductionModifier, Reduction::ReductionModifier,
+      convert, parser::OmpReductionModifier::Value,
+      Reduction::ReductionModifier,
       // clang-format off
       MS(Inscan,  Inscan)
       MS(Task,    Task)
@@ -1184,16 +1186,17 @@ Reduction make(const parser::OmpClause::Reduction &inp,
       // clang-format on
   );
 
-  auto &t0 =
-      std::get<std::optional<parser::OmpReductionClause::ReductionModifier>>(
-          inp.v.t);
-  auto &t1 = std::get<parser::OmpReductionIdentifier>(inp.v.t);
+  auto &mods = semantics::OmpGetModifiers(inp.v);
+  auto *t0 =
+      semantics::OmpGetUniqueModifier<parser::OmpReductionModifier>(mods);
+  auto *t1 =
+      semantics::OmpGetUniqueModifier<parser::OmpReductionIdentifier>(mods);
   auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
   return Reduction{
       {/*ReductionModifier=*/t0
-           ? std::make_optional<Reduction::ReductionModifier>(convert(*t0))
+           ? std::make_optional<Reduction::ReductionModifier>(convert(t0->v))
            : std::nullopt,
-       /*ReductionIdentifiers=*/{makeReductionOperator(t1, semaCtx)},
+       /*ReductionIdentifiers=*/{makeReductionOperator(*t1, semaCtx)},
        /*List=*/makeObjects(t2, semaCtx)}};
 }
 
@@ -1314,10 +1317,12 @@ 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::OmpReductionIdentifier>(inp.v.t);
+  auto &mods = semantics::OmpGetModifiers(inp.v);
+  auto *t0 =
+      semantics::OmpGetUniqueModifier<parser::OmpReductionIdentifier>(mods);
   auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
   return TaskReduction{
-      {/*ReductionIdentifiers=*/{makeReductionOperator(t0, semaCtx)},
+      {/*ReductionIdentifiers=*/{makeReductionOperator(*t0, semaCtx)},
        /*List=*/makeObjects(t1, semaCtx)}};
 }
 
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index b4d45873abb3ec..063201fc86ca41 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -229,6 +229,11 @@ TYPE_PARSER(construct<OmpLinearModifier>( //
 TYPE_PARSER(construct<OmpReductionIdentifier>(Parser<DefinedOperator>{}) ||
     construct<OmpReductionIdentifier>(Parser<ProcedureDesignator>{}))
 
+TYPE_PARSER(construct<OmpReductionModifier>(
+    "INSCAN" >> pure(OmpReductionModifier::Value::Inscan) ||
+    "TASK" >> pure(OmpReductionModifier::Value::Task) ||
+    "DEFAULT" >> pure(OmpReductionModifier::Value::Default)))
+
 TYPE_PARSER(construct<OmpTaskDependenceType>(
     "DEPOBJ" >> pure(OmpTaskDependenceType::Value::Depobj) ||
     "IN"_id >> pure(OmpTaskDependenceType::Value::In) ||
@@ -237,6 +242,22 @@ 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) ||
+    "ALLOCATABLE" >> pure(OmpVariableCategory::Value::Allocatable) ||
+    "POINTER" >> pure(OmpVariableCategory::Value::Pointer) ||
+    "SCALAR" >> pure(OmpVariableCategory::Value::Scalar)))
+
+TYPE_PARSER(sourced(construct<OmpDefaultmapClause::Modifier>(
+    Parser<OmpVariableCategory>{})))
+
 // --- Parsers for clauses --------------------------------------------
 
 // [5.0] 2.10.1 affinity([aff-modifier:] locator-list)
@@ -309,16 +330,7 @@ TYPE_PARSER(construct<OmpDefaultmapClause>(
             pure(OmpDefaultmapClause::ImplicitBehavior::Firstprivate) ||
         "NONE" >> pure(OmpDefaultmapClause::ImplicitBehavior::None) ||
         "DEFAULT" >> pure(OmpDefaultmapClause::ImplicitBehavior::Default)),
-    maybe(":" >>
-        construct<OmpDefaultmapClause::VariableCategory>(
-            "ALL"_id >> pure(OmpDefaultmapClause::VariableCategory::All) ||
-            "SCALAR" >> pure(OmpDefaultmapClause::VariableCategory::Scalar) ||
-            "AGGREGATE" >>
-                pure(OmpDefaultmapClause::VariableCategory::Aggregate) ||
-            "ALLOCATABLE" >>
-                pure(OmpDefaultmapClause::VariableCategory::Allocatable) ||
-            "POINTER" >>
-                pure(OmpDefaultmapClause::VariableCategory::Pointer)))))
+    maybe(":" >> nonemptyList(Parser<OmpDefaultmapClause::Modifier>{}))))
 
 // 2.7.1 SCHEDULE ([modifier1 [, modifier2]:]kind[, chunk_size])
 //       Modifier ->  MONITONIC | NONMONOTONIC | SIMD
@@ -375,12 +387,8 @@ TYPE_PARSER(construct<OmpIfClause>(
     scalarLogicalExpr))
 
 TYPE_PARSER(construct<OmpReductionClause>(
-    maybe(
-        ("INSCAN" >> pure(OmpReductionClause::ReductionModifier::Inscan) ||
-            "TASK" >> pure(OmpReductionClause::ReductionModifier::Task) ||
-            "DEFAULT" >> pure(OmpReductionClause::ReductionModifier::Default)) /
-        ","),
-    Parser<OmpReductionIdentifier>{} / ":", Parser<OmpObjectList>{}))
+    maybe(nonemptyList(Parser<OmpReductionClause::Modifier>{}) / ":"),
+    Parser<OmpObjectList>{}))
 
 // OMP 5.0 2.19.5.6 IN_REDUCTION (reduction-identifier: variable-name-list)
 TYPE_PARSER(construct<OmpInReductionClause>(
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index b2a0256cb58d0c..e06f4dea80f488 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2179,10 +2179,8 @@ class UnparseVisitor {
     Walk(":", x.step);
   }
   void Unparse(const OmpReductionClause &x) {
-    Walk(std::get<std::optional<OmpReductionClause::ReductionModifier>>(x.t),
-        ",");
-    Walk(std::get<OmpReductionIdentifier>(x.t));
-    Put(":");
+    using Modifier = OmpReductionClause::Modifier;
+    Walk(std::get<std::optional<std::list<Modifier>>>(x.t), ":");
     Walk(std::get<OmpObjectList>(x.t));
   }
   void Unparse(const OmpDetachClause &x) { Walk(x.v); }
@@ -2246,9 +2244,9 @@ class UnparseVisitor {
     Walk(std::get<OmpObjectList>(x.t));
   }
   void Unparse(const OmpDefaultmapClause &x) {
+    using Modifier = OmpDefaultmapClause::Modifier;
     Walk(std::get<OmpDefaultmapClause::ImplicitBehavior>(x.t));
-    Walk(":",
-        std::get<std::optional<OmpDefaultmapClause::VariableCategory>>(x.t));
+    Walk(":", std::get<std::optional<std::list<Modifier>>>(x.t));
   }
   void Unparse(const OmpToClause &x) {
     auto &expect{
@@ -2896,7 +2894,7 @@ class UnparseVisitor {
   WALK_NESTED_ENUM(OmpProcBindClause, Type) // OMP PROC_BIND
   WALK_NESTED_ENUM(OmpDefaultClause, Type) // OMP DEFAULT
   WALK_NESTED_ENUM(OmpDefaultmapClause, ImplicitBehavior) // OMP DEFAULTMAP
-  WALK_NESTED_ENUM(OmpDefaultmapClause, VariableCategory) // OMP DEFAULTMAP
+  WALK_NESTED_ENUM(OmpVariableCategory, Value) // OMP variable-category
   WALK_NESTED_ENUM(
       OmpLastprivateClause, LastprivateModifier) // OMP lastprivate-modifier
   WALK_NESTED_ENUM(OmpScheduleModifierType, ModType) // OMP schedule-modifier
@@ -2905,8 +2903,7 @@ class UnparseVisitor {
   WALK_NESTED_ENUM(OmpScheduleClause, ScheduleType) // OMP schedule-type
   WALK_NESTED_ENUM(OmpDeviceClause, DeviceModifier) // OMP device modifier
   WALK_NESTED_ENUM(OmpDeviceTypeClause, Type) // OMP DEVICE_TYPE
-  WALK_NESTED_ENUM(
-      OmpReductionClause, ReductionModifier) // OMP reduction-modifier
+  WALK_NESTED_ENUM(OmpReductionModifier, Value) // OMP reduction-modifier
   WALK_NESTED_ENUM(OmpFromClause, Expectation) // OMP motion-expectation
   WALK_NESTED_ENUM(OmpIfClause, DirectiveNameModifier) // OMP directive-modifier
   WALK_NESTED_ENUM(OmpCancelType, Type) // OMP cancel-type
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 9cac652216fcf2..079d0fd17bfac1 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -11,6 +11,7 @@
 #include "flang/Evaluate/check-expression.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/expression.h"
+#include "flang/Semantics/openmp-modifiers.h"
 #include "flang/Semantics/tools.h"
 #include <variant>
 
@@ -195,7 +196,7 @@ bool OmpStructureChecker::CheckAllowedClause(llvmOmpClause clause) {
 
   if (!llvm::omp::isAllowedClauseForDirective(dir, clause, version)) {
     unsigned allowedInVersion{[&] {
-      for (unsigned v : {45, 50, 51, 52, 60}) {
+      for (unsigned v : llvm::omp::getOpenMPVersions()) {
         if (v <= version) {
           continue;
         }
@@ -979,12 +980,14 @@ void OmpStructureChecker::Leave(const parser::OpenMPLoopConstruct &x) {
   // constructs inside LOOP may add the relevant information. Scan reduction is
   // supported only in loop constructs, so same checks are not applicable to
   // other directives.
+  using ReductionModifier = parser::OmpReductionModifier;
   for (const auto &clause : clauseList.v) {
     if (const auto *reductionClause{
             std::get_if<parser::OmpClause::Reduction>(&clause.u)}) {
-      const auto &maybeModifier{
-          std::get<std::optional<ReductionModifier>>(reductionClause->v.t)};
-      if (maybeModifier && *maybeModifier == ReductionModifier::Inscan) {
+      auto &modifiers{OmpGetModifiers(reductionClause->v)};
+      auto *maybeModifier{OmpGetUniqueModifier<ReductionModifier>(modifiers)};
+      if (maybeModifier &&
+          maybeModifier->v == ReductionModifier::Value::Inscan) {
         const auto &objectList{
             std::get<parser::OmpObjectList>(reductionClause->v.t)};
         auto checkReductionSymbolInScan = [&](const parser::Name *name) {
@@ -2850,20 +2853,27 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Destroy &x) {
 
 void OmpStructureChecker::Enter(const parser::OmpClause::Reduction &x) {
   CheckAllowedClause(llvm::omp::Clause::OMPC_reduction);
-  if (CheckReductionOperators(x)) {
-    CheckReductionTypeList(x);
-  }
-  if (const auto &maybeModifier{
-          std::get<std::optional<ReductionModifier>>(x.v.t)}) {
-    const ReductionModifier modifier{*maybeModifier};
-    CheckReductionModifier(modifier);
+  if (OmpVerifyModifiers(x.v, GetContext().clauseSource, context_)) {
+    if (CheckReductionOperators(x)) {
+      CheckReductionTypeList(x);
+    }
+    auto &modifiers{OmpGetModifiers(x.v)};
+    using ReductionModifier = parser::OmpReductionModifier;
+    if (auto *maybeModifier{
+            OmpGetUniqueModifier<ReductionModifier>(modifiers)}) {
+      CheckReductionModifier(*maybeModifier);
+    }
   }
 }
 
 bool OmpStructureChecker::CheckReductionOperators(
     const parser::OmpClause::Reduction &x) {
-
-  const auto &definedOp{std::get<parser::OmpReductionIdentifier>(x.v.t)};
+  auto &modifiers{OmpGetModifiers(x.v)};
+  const auto *definedOp{
+      OmpGetUniqueModifier<parser::OmpReductionIdentifier>(modifiers)};
+  if (!definedOp) {
+    return false;
+  }
   bool ok = false;
   common::visit(
       common::visitors{
@@ -2896,7 +2906,7 @@ bool OmpStructureChecker::CheckReductionOperators(
             }
           },
       },
-      definedOp.u);
+      definedOp->u);
 
   return ok;
 }
@@ -2928,7 +2938,12 @@ bool OmpStructureChecker::CheckIntrinsicOperator(
 
 static bool IsReductionAllowedForType(
     const parser::OmpClause::Reduction &x, const DeclTypeSpec &type) {
-  const auto &definedOp{std::get<parser::OmpReductionIdentifier>(x.v.t)};
+  auto &modifiers{OmpGetModifiers(x.v)};
+  const auto *definedOp{
+      OmpGetUniqueModifier<parser::OmpReductionIdentifier>(modifiers)};
+  if (!definedOp) {
+    return false;
+  }
   // TODO: user defined reduction operators. Just allow everything for now.
   bool ok{true};
 
@@ -3002,7 +3017,7 @@ static bool IsReductionAllowedForType(
             }
           },
       },
-      definedOp.u);
+      definedOp->u);
 
   return ok;
 }
@@ -3035,8 +3050,9 @@ void OmpStructureChecker::CheckReductionTypeList(
 }
 
 void OmpStructureChecker::CheckReductionModifier(
-    const ReductionModifier &modifier) {
-  if (modifier == ReductionModifier::Default) {
+    const parser::OmpReductionModifier &modifier) {
+  using ReductionModifier = parser::OmpReductionModifier;
+  if (modifier.v == ReductionModifier::Value::Default) {
     // The default one is always ok.
     return;
   }
@@ -3049,7 +3065,7 @@ void OmpStructureChecker::CheckReductionModifier(
     context_.Say(GetContext().clauseSource,
         "REDUCTION modifier on LOOP directive must be DEFAULT"_err_en_US);
   }
-  if (modifier == ReductionModifier::Task) {
+  if (modifier.v == ReductionModifier::Value::Task) {
     // "Task" is only allowed on...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Nov 18, 2024

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

Author: Krzysztof Parzyszek (kparzysz)

Changes

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


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

15 Files Affected:

  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+6-2)
  • (modified) flang/include/flang/Parser/parse-tree.h (+40-9)
  • (modified) flang/include/flang/Semantics/openmp-modifiers.h (+4)
  • (modified) flang/lib/Lower/OpenMP/Clauses.cpp (+19-14)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+24-16)
  • (modified) flang/lib/Parser/unparse.cpp (+6-9)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+49-34)
  • (modified) flang/lib/Semantics/check-omp-structure.h (+1-2)
  • (modified) flang/lib/Semantics/openmp-modifiers.cpp (+33)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+32-20)
  • (modified) flang/test/Parser/OpenMP/defaultmap-clause.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/defaultmap-unparse.f90 (+8-8)
  • (modified) flang/test/Parser/OpenMP/reduction-modifier.f90 (+3-3)
  • (modified) flang/test/Semantics/OpenMP/combined-constructs.f90 (+6-6)
  • (modified) flang/test/Semantics/OpenMP/defaultmap-clause-v45.f90 (+1-1)
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index df5bf1d8d3200e..9c59ce520a31aa 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -509,9 +509,11 @@ class ParseTreeDumper {
   NODE(parser, OmpDeclareMapperSpecifier)
   NODE(parser, OmpDefaultClause)
   NODE_ENUM(OmpDefaultClause, Type)
+  NODE(parser, OmpVariableCategory)
+  NODE_ENUM(OmpVariableCategory, Value)
   NODE(parser, OmpDefaultmapClause)
   NODE_ENUM(OmpDefaultmapClause, ImplicitBehavior)
-  NODE_ENUM(OmpDefaultmapClause, VariableCategory)
+  NODE(OmpDefaultmapClause, Modifier)
   NODE(parser, OmpDependenceType)
   NODE_ENUM(OmpDependenceType, Value)
   NODE(parser, OmpTaskDependenceType)
@@ -567,8 +569,10 @@ class ParseTreeDumper {
   NODE_ENUM(OmpBindClause, Type)
   NODE(parser, OmpProcBindClause)
   NODE_ENUM(OmpProcBindClause, Type)
-  NODE_ENUM(OmpReductionClause, ReductionModifier)
+  NODE(parser, OmpReductionModifier)
+  NODE_ENUM(OmpReductionModifier, Value)
   NODE(parser, OmpReductionClause)
+  NODE(OmpReductionClause, Modifier)
   NODE(parser, OmpInReductionClause)
   NODE(parser, OmpReductionCombiner)
   NODE(OmpReductionCombiner, FunctionCombiner)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index ef49a36578270e..5b28bcd4e21b80 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3440,6 +3440,16 @@ struct OmpObject {
 
 WRAPPER_CLASS(OmpObjectList, std::list<OmpObject>);
 
+#define MODIFIER_BOILERPLATE(...) \
+  struct Modifier { \
+    using Variant = std::variant<__VA_ARGS__>; \
+    UNION_CLASS_BOILERPLATE(Modifier); \
+    CharBlock source; \
+    Variant u; \
+  }
+
+#define MODIFIERS() std::optional<std::list<Modifier>>
+
 inline namespace modifier {
 // For uniformity, in all keyword modifiers the name of the type defined
 // by ENUM_CLASS is "Value", e.g.
@@ -3505,12 +3515,20 @@ struct OmpLinearModifier {
 //   - |                                            // 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: [5.0:300-302], [5.1:332-334], [5.2:134-137]
+//
+// reduction-modifier ->
+//   DEFAULT | INSCAN | TASK                        // since 5.0
+struct OmpReductionModifier {
+  ENUM_CLASS(Value, Default, Inscan, Task);
+  WRAPPER_CLASS_BOILERPLATE(OmpReductionModifier, Value);
+};
+
 // 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
@@ -3521,6 +3539,17 @@ struct OmpTaskDependenceType {
   ENUM_CLASS(Value, In, Out, Inout, Inoutset, Mutexinoutset, Depobj)
   WRAPPER_CLASS_BOILERPLATE(OmpTaskDependenceType, Value);
 };
+
+// Ref: [4.5:229-230], [5.0:324-325], [5.1:357-358], [5.2:161-162]
+//
+// variable-category ->
+//   SCALAR |                                       // since 4.5
+//   AGGREGATE | ALLOCATABLE | POINTER |            // since 5.0
+//   ALL                                            // since 5.2
+struct OmpVariableCategory {
+  ENUM_CLASS(Value, Aggregate, All, Allocatable, Pointer, Scalar)
+  WRAPPER_CLASS_BOILERPLATE(OmpVariableCategory, Value);
+};
 } // namespace modifier
 
 // --- Clauses
@@ -3578,8 +3607,8 @@ struct OmpDefaultmapClause {
   TUPLE_CLASS_BOILERPLATE(OmpDefaultmapClause);
   ENUM_CLASS(
       ImplicitBehavior, Alloc, To, From, Tofrom, Firstprivate, None, Default)
-  ENUM_CLASS(VariableCategory, All, Scalar, Aggregate, Allocatable, Pointer)
-  std::tuple<ImplicitBehavior, std::optional<VariableCategory>> t;
+  MODIFIER_BOILERPLATE(OmpVariableCategory);
+  std::tuple<ImplicitBehavior, MODIFIERS()> t;
 };
 
 // 2.13.9 iteration-offset -> +/- non-negative-constant
@@ -3771,14 +3800,16 @@ struct OmpProcBindClause {
   WRAPPER_CLASS_BOILERPLATE(OmpProcBindClause, Type);
 };
 
-// 2.15.3.6 reduction-clause -> REDUCTION (reduction-identifier:
-//                                         variable-name-list)
+// Ref: [4.5:201-207], [5.0:300-302], [5.1:332-334], [5.2:134-137]
+//
+// reduction-clause ->
+//    REDUCTION(reduction-identifier: list) |       // since 4.5
+//    REDUCTION([reduction-modifier,]
+//        reduction-identifier: list)               // since 5.0
 struct OmpReductionClause {
   TUPLE_CLASS_BOILERPLATE(OmpReductionClause);
-  ENUM_CLASS(ReductionModifier, Inscan, Task, Default)
-  std::tuple<std::optional<ReductionModifier>, OmpReductionIdentifier,
-      OmpObjectList>
-      t;
+  MODIFIER_BOILERPLATE(OmpReductionModifier, OmpReductionIdentifier);
+  std::tuple<MODIFIERS(), OmpObjectList> t;
 };
 
 // 2.7.1 sched-modifier -> MONOTONIC | NONMONOTONIC | SIMD
diff --git a/flang/include/flang/Semantics/openmp-modifiers.h b/flang/include/flang/Semantics/openmp-modifiers.h
index 6be582761ed687..28fec7314cd8b5 100644
--- a/flang/include/flang/Semantics/openmp-modifiers.h
+++ b/flang/include/flang/Semantics/openmp-modifiers.h
@@ -70,7 +70,11 @@ const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpLinearModifier>();
 template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpReductionIdentifier>();
 template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpReductionModifier>();
+template <>
 const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpTaskDependenceType>();
+template <>
+const OmpModifierDescriptor &OmpGetDescriptor<parser::OmpVariableCategory>();
 
 // Explanation of terminology:
 //
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index 5e514b4ba9f0da..3e0a11bc785e33 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -12,6 +12,7 @@
 #include "flang/Evaluate/expression.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/expression.h"
+#include "flang/Semantics/openmp-modifiers.h"
 #include "flang/Semantics/symbol.h"
 
 #include "llvm/Frontend/OpenMP/OMPConstants.h"
@@ -571,7 +572,8 @@ Defaultmap make(const parser::OmpClause::Defaultmap &inp,
   );
 
   CLAUSET_ENUM_CONVERT( //
-      convert2, wrapped::VariableCategory, Defaultmap::VariableCategory,
+      convert2, parser::OmpVariableCategory::Value,
+      Defaultmap::VariableCategory,
       // clang-format off
       MS(Aggregate,    Aggregate)
       MS(All,          All)
@@ -581,10 +583,11 @@ Defaultmap make(const parser::OmpClause::Defaultmap &inp,
       // clang-format on
   );
 
+  auto &mods{semantics::OmpGetModifiers(inp.v)};
   auto &t0 = std::get<wrapped::ImplicitBehavior>(inp.v.t);
-  auto &t1 = std::get<std::optional<wrapped::VariableCategory>>(inp.v.t);
+  auto *t1 = semantics::OmpGetUniqueModifier<parser::OmpVariableCategory>(mods);
 
-  auto category = t1 ? convert2(*t1) : Defaultmap::VariableCategory::All;
+  auto category = t1 ? convert2(t1->v) : Defaultmap::VariableCategory::All;
   return Defaultmap{{/*ImplicitBehavior=*/convert1(t0),
                      /*VariableCategory=*/category}};
 }
@@ -1173,10 +1176,9 @@ ProcBind make(const parser::OmpClause::ProcBind &inp,
 Reduction make(const parser::OmpClause::Reduction &inp,
                semantics::SemanticsContext &semaCtx) {
   // inp.v -> parser::OmpReductionClause
-  using wrapped = parser::OmpReductionClause;
-
   CLAUSET_ENUM_CONVERT( //
-      convert, wrapped::ReductionModifier, Reduction::ReductionModifier,
+      convert, parser::OmpReductionModifier::Value,
+      Reduction::ReductionModifier,
       // clang-format off
       MS(Inscan,  Inscan)
       MS(Task,    Task)
@@ -1184,16 +1186,17 @@ Reduction make(const parser::OmpClause::Reduction &inp,
       // clang-format on
   );
 
-  auto &t0 =
-      std::get<std::optional<parser::OmpReductionClause::ReductionModifier>>(
-          inp.v.t);
-  auto &t1 = std::get<parser::OmpReductionIdentifier>(inp.v.t);
+  auto &mods = semantics::OmpGetModifiers(inp.v);
+  auto *t0 =
+      semantics::OmpGetUniqueModifier<parser::OmpReductionModifier>(mods);
+  auto *t1 =
+      semantics::OmpGetUniqueModifier<parser::OmpReductionIdentifier>(mods);
   auto &t2 = std::get<parser::OmpObjectList>(inp.v.t);
   return Reduction{
       {/*ReductionModifier=*/t0
-           ? std::make_optional<Reduction::ReductionModifier>(convert(*t0))
+           ? std::make_optional<Reduction::ReductionModifier>(convert(t0->v))
            : std::nullopt,
-       /*ReductionIdentifiers=*/{makeReductionOperator(t1, semaCtx)},
+       /*ReductionIdentifiers=*/{makeReductionOperator(*t1, semaCtx)},
        /*List=*/makeObjects(t2, semaCtx)}};
 }
 
@@ -1314,10 +1317,12 @@ 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::OmpReductionIdentifier>(inp.v.t);
+  auto &mods = semantics::OmpGetModifiers(inp.v);
+  auto *t0 =
+      semantics::OmpGetUniqueModifier<parser::OmpReductionIdentifier>(mods);
   auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
   return TaskReduction{
-      {/*ReductionIdentifiers=*/{makeReductionOperator(t0, semaCtx)},
+      {/*ReductionIdentifiers=*/{makeReductionOperator(*t0, semaCtx)},
        /*List=*/makeObjects(t1, semaCtx)}};
 }
 
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index b4d45873abb3ec..063201fc86ca41 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -229,6 +229,11 @@ TYPE_PARSER(construct<OmpLinearModifier>( //
 TYPE_PARSER(construct<OmpReductionIdentifier>(Parser<DefinedOperator>{}) ||
     construct<OmpReductionIdentifier>(Parser<ProcedureDesignator>{}))
 
+TYPE_PARSER(construct<OmpReductionModifier>(
+    "INSCAN" >> pure(OmpReductionModifier::Value::Inscan) ||
+    "TASK" >> pure(OmpReductionModifier::Value::Task) ||
+    "DEFAULT" >> pure(OmpReductionModifier::Value::Default)))
+
 TYPE_PARSER(construct<OmpTaskDependenceType>(
     "DEPOBJ" >> pure(OmpTaskDependenceType::Value::Depobj) ||
     "IN"_id >> pure(OmpTaskDependenceType::Value::In) ||
@@ -237,6 +242,22 @@ 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) ||
+    "ALLOCATABLE" >> pure(OmpVariableCategory::Value::Allocatable) ||
+    "POINTER" >> pure(OmpVariableCategory::Value::Pointer) ||
+    "SCALAR" >> pure(OmpVariableCategory::Value::Scalar)))
+
+TYPE_PARSER(sourced(construct<OmpDefaultmapClause::Modifier>(
+    Parser<OmpVariableCategory>{})))
+
 // --- Parsers for clauses --------------------------------------------
 
 // [5.0] 2.10.1 affinity([aff-modifier:] locator-list)
@@ -309,16 +330,7 @@ TYPE_PARSER(construct<OmpDefaultmapClause>(
             pure(OmpDefaultmapClause::ImplicitBehavior::Firstprivate) ||
         "NONE" >> pure(OmpDefaultmapClause::ImplicitBehavior::None) ||
         "DEFAULT" >> pure(OmpDefaultmapClause::ImplicitBehavior::Default)),
-    maybe(":" >>
-        construct<OmpDefaultmapClause::VariableCategory>(
-            "ALL"_id >> pure(OmpDefaultmapClause::VariableCategory::All) ||
-            "SCALAR" >> pure(OmpDefaultmapClause::VariableCategory::Scalar) ||
-            "AGGREGATE" >>
-                pure(OmpDefaultmapClause::VariableCategory::Aggregate) ||
-            "ALLOCATABLE" >>
-                pure(OmpDefaultmapClause::VariableCategory::Allocatable) ||
-            "POINTER" >>
-                pure(OmpDefaultmapClause::VariableCategory::Pointer)))))
+    maybe(":" >> nonemptyList(Parser<OmpDefaultmapClause::Modifier>{}))))
 
 // 2.7.1 SCHEDULE ([modifier1 [, modifier2]:]kind[, chunk_size])
 //       Modifier ->  MONITONIC | NONMONOTONIC | SIMD
@@ -375,12 +387,8 @@ TYPE_PARSER(construct<OmpIfClause>(
     scalarLogicalExpr))
 
 TYPE_PARSER(construct<OmpReductionClause>(
-    maybe(
-        ("INSCAN" >> pure(OmpReductionClause::ReductionModifier::Inscan) ||
-            "TASK" >> pure(OmpReductionClause::ReductionModifier::Task) ||
-            "DEFAULT" >> pure(OmpReductionClause::ReductionModifier::Default)) /
-        ","),
-    Parser<OmpReductionIdentifier>{} / ":", Parser<OmpObjectList>{}))
+    maybe(nonemptyList(Parser<OmpReductionClause::Modifier>{}) / ":"),
+    Parser<OmpObjectList>{}))
 
 // OMP 5.0 2.19.5.6 IN_REDUCTION (reduction-identifier: variable-name-list)
 TYPE_PARSER(construct<OmpInReductionClause>(
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index b2a0256cb58d0c..e06f4dea80f488 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2179,10 +2179,8 @@ class UnparseVisitor {
     Walk(":", x.step);
   }
   void Unparse(const OmpReductionClause &x) {
-    Walk(std::get<std::optional<OmpReductionClause::ReductionModifier>>(x.t),
-        ",");
-    Walk(std::get<OmpReductionIdentifier>(x.t));
-    Put(":");
+    using Modifier = OmpReductionClause::Modifier;
+    Walk(std::get<std::optional<std::list<Modifier>>>(x.t), ":");
     Walk(std::get<OmpObjectList>(x.t));
   }
   void Unparse(const OmpDetachClause &x) { Walk(x.v); }
@@ -2246,9 +2244,9 @@ class UnparseVisitor {
     Walk(std::get<OmpObjectList>(x.t));
   }
   void Unparse(const OmpDefaultmapClause &x) {
+    using Modifier = OmpDefaultmapClause::Modifier;
     Walk(std::get<OmpDefaultmapClause::ImplicitBehavior>(x.t));
-    Walk(":",
-        std::get<std::optional<OmpDefaultmapClause::VariableCategory>>(x.t));
+    Walk(":", std::get<std::optional<std::list<Modifier>>>(x.t));
   }
   void Unparse(const OmpToClause &x) {
     auto &expect{
@@ -2896,7 +2894,7 @@ class UnparseVisitor {
   WALK_NESTED_ENUM(OmpProcBindClause, Type) // OMP PROC_BIND
   WALK_NESTED_ENUM(OmpDefaultClause, Type) // OMP DEFAULT
   WALK_NESTED_ENUM(OmpDefaultmapClause, ImplicitBehavior) // OMP DEFAULTMAP
-  WALK_NESTED_ENUM(OmpDefaultmapClause, VariableCategory) // OMP DEFAULTMAP
+  WALK_NESTED_ENUM(OmpVariableCategory, Value) // OMP variable-category
   WALK_NESTED_ENUM(
       OmpLastprivateClause, LastprivateModifier) // OMP lastprivate-modifier
   WALK_NESTED_ENUM(OmpScheduleModifierType, ModType) // OMP schedule-modifier
@@ -2905,8 +2903,7 @@ class UnparseVisitor {
   WALK_NESTED_ENUM(OmpScheduleClause, ScheduleType) // OMP schedule-type
   WALK_NESTED_ENUM(OmpDeviceClause, DeviceModifier) // OMP device modifier
   WALK_NESTED_ENUM(OmpDeviceTypeClause, Type) // OMP DEVICE_TYPE
-  WALK_NESTED_ENUM(
-      OmpReductionClause, ReductionModifier) // OMP reduction-modifier
+  WALK_NESTED_ENUM(OmpReductionModifier, Value) // OMP reduction-modifier
   WALK_NESTED_ENUM(OmpFromClause, Expectation) // OMP motion-expectation
   WALK_NESTED_ENUM(OmpIfClause, DirectiveNameModifier) // OMP directive-modifier
   WALK_NESTED_ENUM(OmpCancelType, Type) // OMP cancel-type
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 9cac652216fcf2..079d0fd17bfac1 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -11,6 +11,7 @@
 #include "flang/Evaluate/check-expression.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/expression.h"
+#include "flang/Semantics/openmp-modifiers.h"
 #include "flang/Semantics/tools.h"
 #include <variant>
 
@@ -195,7 +196,7 @@ bool OmpStructureChecker::CheckAllowedClause(llvmOmpClause clause) {
 
   if (!llvm::omp::isAllowedClauseForDirective(dir, clause, version)) {
     unsigned allowedInVersion{[&] {
-      for (unsigned v : {45, 50, 51, 52, 60}) {
+      for (unsigned v : llvm::omp::getOpenMPVersions()) {
         if (v <= version) {
           continue;
         }
@@ -979,12 +980,14 @@ void OmpStructureChecker::Leave(const parser::OpenMPLoopConstruct &x) {
   // constructs inside LOOP may add the relevant information. Scan reduction is
   // supported only in loop constructs, so same checks are not applicable to
   // other directives.
+  using ReductionModifier = parser::OmpReductionModifier;
   for (const auto &clause : clauseList.v) {
     if (const auto *reductionClause{
             std::get_if<parser::OmpClause::Reduction>(&clause.u)}) {
-      const auto &maybeModifier{
-          std::get<std::optional<ReductionModifier>>(reductionClause->v.t)};
-      if (maybeModifier && *maybeModifier == ReductionModifier::Inscan) {
+      auto &modifiers{OmpGetModifiers(reductionClause->v)};
+      auto *maybeModifier{OmpGetUniqueModifier<ReductionModifier>(modifiers)};
+      if (maybeModifier &&
+          maybeModifier->v == ReductionModifier::Value::Inscan) {
         const auto &objectList{
             std::get<parser::OmpObjectList>(reductionClause->v.t)};
         auto checkReductionSymbolInScan = [&](const parser::Name *name) {
@@ -2850,20 +2853,27 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Destroy &x) {
 
 void OmpStructureChecker::Enter(const parser::OmpClause::Reduction &x) {
   CheckAllowedClause(llvm::omp::Clause::OMPC_reduction);
-  if (CheckReductionOperators(x)) {
-    CheckReductionTypeList(x);
-  }
-  if (const auto &maybeModifier{
-          std::get<std::optional<ReductionModifier>>(x.v.t)}) {
-    const ReductionModifier modifier{*maybeModifier};
-    CheckReductionModifier(modifier);
+  if (OmpVerifyModifiers(x.v, GetContext().clauseSource, context_)) {
+    if (CheckReductionOperators(x)) {
+      CheckReductionTypeList(x);
+    }
+    auto &modifiers{OmpGetModifiers(x.v)};
+    using ReductionModifier = parser::OmpReductionModifier;
+    if (auto *maybeModifier{
+            OmpGetUniqueModifier<ReductionModifier>(modifiers)}) {
+      CheckReductionModifier(*maybeModifier);
+    }
   }
 }
 
 bool OmpStructureChecker::CheckReductionOperators(
     const parser::OmpClause::Reduction &x) {
-
-  const auto &definedOp{std::get<parser::OmpReductionIdentifier>(x.v.t)};
+  auto &modifiers{OmpGetModifiers(x.v)};
+  const auto *definedOp{
+      OmpGetUniqueModifier<parser::OmpReductionIdentifier>(modifiers)};
+  if (!definedOp) {
+    return false;
+  }
   bool ok = false;
   common::visit(
       common::visitors{
@@ -2896,7 +2906,7 @@ bool OmpStructureChecker::CheckReductionOperators(
             }
           },
       },
-      definedOp.u);
+      definedOp->u);
 
   return ok;
 }
@@ -2928,7 +2938,12 @@ bool OmpStructureChecker::CheckIntrinsicOperator(
 
 static bool IsReductionAllowedForType(
     const parser::OmpClause::Reduction &x, const DeclTypeSpec &type) {
-  const auto &definedOp{std::get<parser::OmpReductionIdentifier>(x.v.t)};
+  auto &modifiers{OmpGetModifiers(x.v)};
+  const auto *definedOp{
+      OmpGetUniqueModifier<parser::OmpReductionIdentifier>(modifiers)};
+  if (!definedOp) {
+    return false;
+  }
   // TODO: user defined reduction operators. Just allow everything for now.
   bool ok{true};
 
@@ -3002,7 +3017,7 @@ static bool IsReductionAllowedForType(
             }
           },
       },
-      definedOp.u);
+      definedOp->u);
 
   return ok;
 }
@@ -3035,8 +3050,9 @@ void OmpStructureChecker::CheckReductionTypeList(
 }
 
 void OmpStructureChecker::CheckReductionModifier(
-    const ReductionModifier &modifier) {
-  if (modifier == ReductionModifier::Default) {
+    const parser::OmpReductionModifier &modifier) {
+  using ReductionModifier = parser::OmpReductionModifier;
+  if (modifier.v == ReductionModifier::Value::Default) {
     // The default one is always ok.
     return;
   }
@@ -3049,7 +3065,7 @@ void OmpStructureChecker::CheckReductionModifier(
     context_.Say(GetContext().clauseSource,
         "REDUCTION modifier on LOOP directive must be DEFAULT"_err_en_US);
   }
-  if (modifier == ReductionModifier::Task) {
+  if (modifier.v == ReductionModifier::Value::Task) {
     // "Task" is only allowed on...
[truncated]

Copy link

github-actions bot commented Nov 18, 2024

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

@@ -33,7 +33,7 @@ program main
enddo
!$omp end target parallel

!ERROR: The DEFAULTMAP clause requires a variable-category SCALAR in OpenMP v1.1, try -fopenmp-version=50
!ERROR: A variable-category modifier is required
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried that the new error messages are less informative than the old ones

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can be improved. Right now OmpVerifyModifiers checks all properties, and returns a single value: "everything passed" or "something is wrong". Based on the descriptor information, it could generate the "try different version" if applicable, plus it could return a more detailed information about what failed to the caller.

On the positive side, with the new functions, the actual modifier is highlighted (except if it's required but missing).

If you're ok with that, I can improve this in a subsequent PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Improving in a subsequent PR is okay with me.

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.

Tentatively +1, but if you can please wait for Kiran to take a look as well.

flang/include/flang/Parser/parse-tree.h Show resolved Hide resolved
Base automatically changed from users/kparzysz/spr/m02-openmp-descriptors to main November 20, 2024 16:38
@kparzysz
Copy link
Contributor Author

Tentatively +1, but if you can please wait for Kiran to take a look as well.

@kiranchandramohan: I'll wait for your feedback.

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.

Comment on lines +525 to +528
auto &modifiers{OmpGetModifiers(x.v)};
if (!modifiers) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Generally frontend code is written in a nested fashion

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'll update that in a separate PR.

Comment on lines +2872 to +2876
const auto *definedOp{
OmpGetUniqueModifier<parser::OmpReductionIdentifier>(modifiers)};
if (!definedOp) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Generally frontend code is nested. Below also.

const auto &objList{std::get<parser::OmpObjectList>(x.v.t)};
ResolveOmpObjectList(objList, Symbol::Flag::OmpReduction);

auto &modifiers{OmpGetModifiers(x.v)};
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a bit to process that reduction identifiers are modifiers also. Was that established in OpenMP 5.2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In explicit terms, yes. Before 5.2 it was a de facto modifier by the virtue of being on the left side of a :, e.g. reduction(+: x, y, z). In 5.2 all such things are modifiers (there are also a couple of post-modifiers, i.e. following a :).

@kparzysz kparzysz merged commit 4fc1141 into main Nov 21, 2024
8 checks passed
@kparzysz kparzysz deleted the users/kparzysz/spr/m03-semantic-checks branch November 21, 2024 19:18
kraj pushed a commit to kraj/llvm-project that referenced this pull request Nov 21, 2024
Frontend code is generally nested.
Follow-up to llvm#116658.
kparzysz added a commit that referenced this pull request Nov 21, 2024
Frontend code is generally nested.
Follow-up to #116658.
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