Skip to content

[flang][OpenMP] Make all block constructs share the same structure #150956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 1, 2025

Conversation

kparzysz
Copy link
Contributor

The structure is

  • OmpBeginDirective (aka OmpDirectiveSpecification)
  • Block
  • optional (aka optional)

The OmpBeginDirective and OmpEndDirective are effectively different names for OmpDirectiveSpecification. They exist to allow the semantic analyses to distinguish between the beginning and the ending of a block construct without maintaining additional context.

The actual changes are in the parser: parse-tree.h and openmp-parser.cpp in particular. The rest is simply changing the way the directive/clause information is accessed (typically for the simpler).

All standalone and block constructs now use OmpDirectiveSpecification to store the directive/clause information.

kparzysz added 5 commits July 26, 2025 09:47
It was an alias for OmpDirectiveName, which could cause confusion in
parse-tree visitors: a visitor for OmpDirectiveNameModifier could be
executed for an OmpDirectiveName node, leading to unexpected results.
The OpenMPSectionConstruct corresponds to `!$omp section` directive, but
there is nothing in the AST node that stores the directive information.
Even though the only possibility (at the moment) is "section" without any
clauses, for improved generality it is helpful to have that information
anyway.
The structure is
- OmpBeginDirective (aka OmpDirectiveSpecification)
- Block
- optional<OmpEndDirective> (aka optional<OmpDirectiveSpecification>)

The OmpBeginDirective and OmpEndDirective are effectively different
names for OmpDirectiveSpecification. They exist to allow the semantic
analyses to distinguish between the beginning and the ending of a block
construct without maintaining additional context.

The actual changes are in the parser: parse-tree.h and openmp-parser.cpp
in particular. The rest is simply changing the way the directive/clause
information is accessed (typically for the simpler).

All standalone and block constructs now use OmpDirectiveSpecification
to store the directive/clause information.
@kparzysz kparzysz requested review from tblah and Stylie777 July 28, 2025 14:14
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp flang:semantics flang:parser labels Jul 28, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 28, 2025

@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-semantics

Author: Krzysztof Parzyszek (kparzysz)

Changes

The structure is

  • OmpBeginDirective (aka OmpDirectiveSpecification)
  • Block
  • optional<OmpEndDirective> (aka optional<OmpDirectiveSpecification>)

The OmpBeginDirective and OmpEndDirective are effectively different names for OmpDirectiveSpecification. They exist to allow the semantic analyses to distinguish between the beginning and the ending of a block construct without maintaining additional context.

The actual changes are in the parser: parse-tree.h and openmp-parser.cpp in particular. The rest is simply changing the way the directive/clause information is accessed (typically for the simpler).

All standalone and block constructs now use OmpDirectiveSpecification to store the directive/clause information.


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

38 Files Affected:

  • (modified) flang/examples/FeatureList/FeatureList.cpp (+2-3)
  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+2-3)
  • (modified) flang/include/flang/Parser/openmp-utils.h (+2-9)
  • (modified) flang/include/flang/Parser/parse-tree.h (+43-40)
  • (modified) flang/lib/Lower/OpenMP/Atomic.cpp (+1-1)
  • (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+14-35)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+100-92)
  • (modified) flang/lib/Parser/parse-tree.cpp (+3-3)
  • (modified) flang/lib/Parser/unparse.cpp (+20-80)
  • (modified) flang/lib/Semantics/check-omp-atomic.cpp (+3-5)
  • (modified) flang/lib/Semantics/check-omp-loop.cpp (+5-9)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+50-65)
  • (modified) flang/lib/Semantics/check-omp-structure.h (+3-3)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+21-13)
  • (modified) flang/lib/Semantics/resolve-names.cpp (+5-7)
  • (modified) flang/test/Parser/OpenMP/affinity-clause.f90 (+6-6)
  • (modified) flang/test/Parser/OpenMP/allocators-unparse.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/atomic-compare.f90 (+9-9)
  • (modified) flang/test/Parser/OpenMP/atomic-end.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/block-construct.f90 (+10-10)
  • (modified) flang/test/Parser/OpenMP/construct-prefix-conflict.f90 (+20-20)
  • (modified) flang/test/Parser/OpenMP/defaultmap-clause.f90 (+12-12)
  • (modified) flang/test/Parser/OpenMP/defaultmap-unparse.f90 (+18-18)
  • (modified) flang/test/Parser/OpenMP/dispatch.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/if-clause.f90 (+2-2)
  • (modified) flang/test/Parser/OpenMP/in-reduction-clause.f90 (+6-6)
  • (modified) flang/test/Parser/OpenMP/map-modifiers-v60.f90 (+10-10)
  • (modified) flang/test/Parser/OpenMP/map-modifiers.f90 (+23-23)
  • (modified) flang/test/Parser/OpenMP/masked-unparse.f90 (+6-6)
  • (modified) flang/test/Parser/OpenMP/master-unparse.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/openmp6-directive-spellings.f90 (+6-6)
  • (modified) flang/test/Parser/OpenMP/proc-bind.f90 (+2-2)
  • (modified) flang/test/Parser/OpenMP/scope.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/target_device_parse.f90 (+32-32)
  • (modified) flang/test/Parser/OpenMP/task-reduction-clause.f90 (+2-2)
  • (modified) flang/test/Parser/OpenMP/task.f90 (+1-1)
  • (modified) flang/test/Semantics/OpenMP/clause-validity01.f90 (+1-2)
  • (modified) flang/test/Semantics/OpenMP/symbol08.f90 (+1-2)
diff --git a/flang/examples/FeatureList/FeatureList.cpp b/flang/examples/FeatureList/FeatureList.cpp
index 64b57b633feaf..0b8066e36312b 100644
--- a/flang/examples/FeatureList/FeatureList.cpp
+++ b/flang/examples/FeatureList/FeatureList.cpp
@@ -445,10 +445,9 @@ struct NodeVisitor {
   READ_FEATURE(ObjectDecl)
   READ_FEATURE(OldParameterStmt)
   READ_FEATURE(OmpAlignedClause)
-  READ_FEATURE(OmpBeginBlockDirective)
+  READ_FEATURE(OmpBeginDirective)
   READ_FEATURE(OmpBeginLoopDirective)
   READ_FEATURE(OmpBeginSectionsDirective)
-  READ_FEATURE(OmpBlockDirective)
   READ_FEATURE(OmpClause)
   READ_FEATURE(OmpClauseList)
   READ_FEATURE(OmpCriticalDirective)
@@ -472,7 +471,7 @@ struct NodeVisitor {
   READ_FEATURE(OmpIteration)
   READ_FEATURE(OmpIterationOffset)
   READ_FEATURE(OmpIterationVector)
-  READ_FEATURE(OmpEndBlockDirective)
+  READ_FEATURE(OmpEndDirective)
   READ_FEATURE(OmpEndCriticalDirective)
   READ_FEATURE(OmpEndLoopDirective)
   READ_FEATURE(OmpEndSectionsDirective)
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index ebac54f6e29ba..b09dd32a90b98 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -534,10 +534,8 @@ class ParseTreeDumper {
   NODE(parser, OmpAtClause)
   NODE_ENUM(OmpAtClause, ActionTime)
   NODE_ENUM(OmpSeverityClause, Severity)
-  NODE(parser, OmpBeginBlockDirective)
   NODE(parser, OmpBeginLoopDirective)
   NODE(parser, OmpBeginSectionsDirective)
-  NODE(parser, OmpBlockDirective)
   static std::string GetNodeName(const llvm::omp::Directive &x) {
     return llvm::Twine("llvm::omp::Directive = ",
         llvm::omp::getOpenMPDirectiveName(x, llvm::omp::FallbackVersion))
@@ -584,7 +582,6 @@ class ParseTreeDumper {
   NODE(parser, OmpDetachClause)
   NODE(parser, OmpDoacrossClause)
   NODE(parser, OmpDestroyClause)
-  NODE(parser, OmpEndBlockDirective)
   NODE(parser, OmpEndCriticalDirective)
   NODE(parser, OmpEndLoopDirective)
   NODE(parser, OmpEndSectionsDirective)
@@ -704,6 +701,8 @@ class ParseTreeDumper {
   NODE(parser, OpenMPDeclarativeAssumes)
   NODE(parser, OmpAssumeDirective)
   NODE(parser, OmpEndAssumeDirective)
+  NODE(parser, OmpBeginDirective)
+  NODE(parser, OmpEndDirective)
   NODE(parser, OpenMPAtomicConstruct)
   NODE(parser, OpenMPBlockConstruct)
   NODE(parser, OpenMPCancelConstruct)
diff --git a/flang/include/flang/Parser/openmp-utils.h b/flang/include/flang/Parser/openmp-utils.h
index 41c04424e91c6..fa0f7656cd5d8 100644
--- a/flang/include/flang/Parser/openmp-utils.h
+++ b/flang/include/flang/Parser/openmp-utils.h
@@ -68,11 +68,6 @@ struct DirectiveNameScope {
     return MakeName(x.source, llvm::omp::Directive::OMPD_nothing);
   }
 
-  static OmpDirectiveName GetOmpDirectiveName(const OmpBeginBlockDirective &x) {
-    auto &dir{std::get<OmpBlockDirective>(x.t)};
-    return MakeName(dir.source, dir.v);
-  }
-
   static OmpDirectiveName GetOmpDirectiveName(const OmpBeginLoopDirective &x) {
     auto &dir{std::get<OmpLoopDirective>(x.t)};
     return MakeName(dir.source, dir.v);
@@ -106,10 +101,8 @@ struct DirectiveNameScope {
         return GetOmpDirectiveName(x.v);
       }
     } else if constexpr (TupleTrait<T>) {
-      if constexpr (std::is_same_v<T, OpenMPAllocatorsConstruct> ||
-          std::is_same_v<T, OpenMPAtomicConstruct> ||
-          std::is_same_v<T, OpenMPDispatchConstruct>) {
-        return std::get<OmpDirectiveSpecification>(x.t).DirName();
+      if constexpr (std::is_base_of_v<OmpBlockConstruct, T>) {
+        return std::get<OmpBeginDirective>(x.t).DirName();
       } else if constexpr (std::is_same_v<T, OmpAssumeDirective> ||
           std::is_same_v<T, OmpCriticalDirective> ||
           std::is_same_v<T, OmpDeclareVariantDirective> ||
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index da0a3632b8a63..7ba5fe964f7a1 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3469,6 +3469,13 @@ WRAPPER_CLASS(PauseStmt, std::optional<StopCode>);
 
 // --- Common definitions
 
+#define INHERITED_TUPLE_CLASS_BOILERPLATE(classname, basename) \
+  template <typename... Ts, typename = common::NoLvalue<Ts...>> \
+  classname(Ts &&...args) : basename(std::move(args)...) {} \
+  classname(basename &&b) : basename(std::move(b)) {} \
+  using TupleTrait = std::true_type; \
+  BOILERPLATE(classname)
+
 #define INHERITED_WRAPPER_CLASS_BOILERPLATE(classname, basename) \
   BOILERPLATE(classname); \
   classname(decltype(basename::v) &&x) : basename(std::move(x)) {} \
@@ -4729,6 +4736,33 @@ struct OmpDirectiveSpecification {
       t;
 };
 
+// OmpBeginDirective and OmpEndDirective are needed for semantic analysis,
+// where some checks are done specifically for either the begin or the end
+// directive. The structure of both is identical, but the diffent types
+// allow to distinguish them in the type-based parse-tree visitor.
+struct OmpBeginDirective : public OmpDirectiveSpecification {
+  INHERITED_TUPLE_CLASS_BOILERPLATE(
+      OmpBeginDirective, OmpDirectiveSpecification);
+};
+
+struct OmpEndDirective : public OmpDirectiveSpecification {
+  INHERITED_TUPLE_CLASS_BOILERPLATE(OmpEndDirective, OmpDirectiveSpecification);
+};
+
+// Common base class for block-associated constructs.
+struct OmpBlockConstruct {
+  TUPLE_CLASS_BOILERPLATE(OmpBlockConstruct);
+  const OmpBeginDirective &BeginDir() const {
+    return std::get<OmpBeginDirective>(t);
+  }
+  const std::optional<OmpEndDirective> &EndDir() const {
+    return std::get<std::optional<OmpEndDirective>>(t);
+  }
+
+  CharBlock source;
+  std::tuple<OmpBeginDirective, Block, std::optional<OmpEndDirective>> t;
+};
+
 struct OmpMetadirectiveDirective {
   TUPLE_CLASS_BOILERPLATE(OmpMetadirectiveDirective);
   std::tuple<Verbatim, OmpClauseList> t;
@@ -4833,12 +4867,6 @@ struct OpenMPSectionsConstruct {
       t;
 };
 
-// OpenMP directive beginning or ending a block
-struct OmpBlockDirective {
-  WRAPPER_CLASS_BOILERPLATE(OmpBlockDirective, llvm::omp::Directive);
-  CharBlock source;
-};
-
 struct OmpDeclareVariantDirective {
   TUPLE_CLASS_BOILERPLATE(OmpDeclareVariantDirective);
   CharBlock source;
@@ -4963,12 +4991,9 @@ struct OpenMPExecutableAllocate {
 //    ALLOCATORS [allocate-clause...]
 //    block
 //    [END ALLOCATORS]
-struct OpenMPAllocatorsConstruct {
-  TUPLE_CLASS_BOILERPLATE(OpenMPAllocatorsConstruct);
-  CharBlock source;
-  std::tuple<OmpDirectiveSpecification, Block,
-      std::optional<OmpDirectiveSpecification>>
-      t;
+struct OpenMPAllocatorsConstruct : public OmpBlockConstruct {
+  INHERITED_TUPLE_CLASS_BOILERPLATE(
+      OpenMPAllocatorsConstruct, OmpBlockConstruct);
 };
 
 // 2.17.7 Atomic construct/2.17.8 Flush construct [OpenMP 5.0]
@@ -4982,15 +5007,11 @@ struct OmpMemoryOrderClause {
   CharBlock source;
 };
 
-struct OpenMPAtomicConstruct {
+struct OpenMPAtomicConstruct : public OmpBlockConstruct {
   llvm::omp::Clause GetKind() const;
   bool IsCapture() const;
   bool IsCompare() const;
-  TUPLE_CLASS_BOILERPLATE(OpenMPAtomicConstruct);
-  CharBlock source;
-  std::tuple<OmpDirectiveSpecification, Block,
-      std::optional<OmpDirectiveSpecification>>
-      t;
+  INHERITED_TUPLE_CLASS_BOILERPLATE(OpenMPAtomicConstruct, OmpBlockConstruct);
 
   // Information filled out during semantic checks to avoid duplication
   // of analyses.
@@ -5054,12 +5075,8 @@ struct OpenMPDepobjConstruct {
 //                    nocontext-clause |
 //                    novariants-clause |
 //                    nowait-clause
-struct OpenMPDispatchConstruct {
-  TUPLE_CLASS_BOILERPLATE(OpenMPDispatchConstruct);
-  CharBlock source;
-  std::tuple<OmpDirectiveSpecification, Block,
-      std::optional<OmpDirectiveSpecification>>
-      t;
+struct OpenMPDispatchConstruct : public OmpBlockConstruct {
+  INHERITED_TUPLE_CLASS_BOILERPLATE(OpenMPDispatchConstruct, OmpBlockConstruct);
 };
 
 // [4.5:162-165], [5.0:242-246], [5.1:275-279], [5.2:315-316], [6.0:498-500]
@@ -5114,22 +5131,8 @@ struct OmpEndLoopDirective {
   CharBlock source;
 };
 
-struct OmpBeginBlockDirective {
-  TUPLE_CLASS_BOILERPLATE(OmpBeginBlockDirective);
-  std::tuple<OmpBlockDirective, OmpClauseList> t;
-  CharBlock source;
-};
-
-struct OmpEndBlockDirective {
-  TUPLE_CLASS_BOILERPLATE(OmpEndBlockDirective);
-  std::tuple<OmpBlockDirective, OmpClauseList> t;
-  CharBlock source;
-};
-
-struct OpenMPBlockConstruct {
-  TUPLE_CLASS_BOILERPLATE(OpenMPBlockConstruct);
-  std::tuple<OmpBeginBlockDirective, Block, std::optional<OmpEndBlockDirective>>
-      t;
+struct OpenMPBlockConstruct : public OmpBlockConstruct {
+  INHERITED_TUPLE_CLASS_BOILERPLATE(OpenMPBlockConstruct, OmpBlockConstruct);
 };
 
 // OpenMP directives enclosing do loop
diff --git a/flang/lib/Lower/OpenMP/Atomic.cpp b/flang/lib/Lower/OpenMP/Atomic.cpp
index 9a233d2d8cb08..623a9370aa00d 100644
--- a/flang/lib/Lower/OpenMP/Atomic.cpp
+++ b/flang/lib/Lower/OpenMP/Atomic.cpp
@@ -699,7 +699,7 @@ void Fortran::lower::omp::lowerAtomic(
   };
 
   fir::FirOpBuilder &builder = converter.getFirOpBuilder();
-  auto &dirSpec = std::get<parser::OmpDirectiveSpecification>(construct.t);
+  const parser::OmpDirectiveSpecification &dirSpec = construct.BeginDir();
   omp::List<omp::Clause> clauses = makeClauses(dirSpec.Clauses(), semaCtx);
   lower::StatementContext stmtCtx;
 
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 6dd4b16a1fb55..68ebb66309f99 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -407,16 +407,9 @@ static void processHostEvalClauses(lower::AbstractConverter &converter,
     common::visit(
         common::visitors{
             [&](const parser::OpenMPBlockConstruct &ompConstruct) {
-              const auto &beginDirective =
-                  std::get<parser::OmpBeginBlockDirective>(ompConstruct.t);
-              beginClauseList =
-                  &std::get<parser::OmpClauseList>(beginDirective.t);
-              if (auto &endDirective =
-                      std::get<std::optional<parser::OmpEndBlockDirective>>(
-                          ompConstruct.t)) {
-                endClauseList =
-                    &std::get<parser::OmpClauseList>(endDirective->t);
-              }
+              beginClauseList = &ompConstruct.BeginDir().Clauses();
+              if (auto &endSpec = ompConstruct.EndDir())
+                endClauseList = &endSpec->Clauses();
             },
             [&](const parser::OpenMPLoopConstruct &ompConstruct) {
               const auto &beginDirective =
@@ -3716,25 +3709,16 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
                    semantics::SemanticsContext &semaCtx,
                    lower::pft::Evaluation &eval,
                    const parser::OpenMPBlockConstruct &blockConstruct) {
-  const auto &beginBlockDirective =
-      std::get<parser::OmpBeginBlockDirective>(blockConstruct.t);
-  mlir::Location currentLocation =
-      converter.genLocation(beginBlockDirective.source);
-  const auto origDirective =
-      std::get<parser::OmpBlockDirective>(beginBlockDirective.t).v;
-  List<Clause> clauses = makeClauses(
-      std::get<parser::OmpClauseList>(beginBlockDirective.t), semaCtx);
-
-  if (const auto &endBlockDirective =
-          std::get<std::optional<parser::OmpEndBlockDirective>>(
-              blockConstruct.t)) {
-    clauses.append(makeClauses(
-        std::get<parser::OmpClauseList>(endBlockDirective->t), semaCtx));
-  }
-
-  assert(llvm::omp::blockConstructSet.test(origDirective) &&
+  const parser::OmpDirectiveSpecification &beginSpec =
+      blockConstruct.BeginDir();
+  List<Clause> clauses = makeClauses(beginSpec.Clauses(), semaCtx);
+  if (auto &endSpec = blockConstruct.EndDir())
+    clauses.append(makeClauses(endSpec->Clauses(), semaCtx));
+
+  llvm::omp::Directive directive = beginSpec.DirId();
+  assert(llvm::omp::blockConstructSet.test(directive) &&
          "Expected block construct");
-  (void)origDirective;
+  mlir::Location currentLocation = converter.genLocation(beginSpec.source);
 
   for (const Clause &clause : clauses) {
     mlir::Location clauseLocation = converter.genLocation(clause.source);
@@ -3777,13 +3761,9 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
     }
   }
 
-  llvm::omp::Directive directive =
-      std::get<parser::OmpBlockDirective>(beginBlockDirective.t).v;
-  const parser::CharBlock &source =
-      std::get<parser::OmpBlockDirective>(beginBlockDirective.t).source;
   ConstructQueue queue{
       buildConstructQueue(converter.getFirOpBuilder().getModule(), semaCtx,
-                          eval, source, directive, clauses)};
+                          eval, beginSpec.source, directive, clauses)};
   genOMPDispatch(converter, symTable, semaCtx, eval, currentLocation, queue,
                  queue.begin());
 }
@@ -4071,8 +4051,7 @@ bool Fortran::lower::isOpenMPTargetConstruct(
     const parser::OpenMPConstruct &omp) {
   llvm::omp::Directive dir = llvm::omp::Directive::OMPD_unknown;
   if (const auto *block = std::get_if<parser::OpenMPBlockConstruct>(&omp.u)) {
-    const auto &begin = std::get<parser::OmpBeginBlockDirective>(block->t);
-    dir = std::get<parser::OmpBlockDirective>(begin.t).v;
+    dir = block->BeginDir().DirId();
   } else if (const auto *loop =
                  std::get_if<parser::OpenMPLoopConstruct>(&omp.u)) {
     const auto &begin = std::get<parser::OmpBeginLoopDirective>(loop->t);
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 8c780388b9cb1..c2c1921f446c7 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -1371,16 +1371,41 @@ TYPE_PARSER(sourced(construct<OmpLoopDirective>(first(
 TYPE_PARSER(sourced(construct<OmpBeginLoopDirective>(
     sourced(Parser<OmpLoopDirective>{}), Parser<OmpClauseList>{})))
 
+static inline constexpr auto IsDirective(llvm::omp::Directive dir) {
+  return [dir](const OmpDirectiveName &name) -> bool { return dir == name.v; };
+}
+
+struct OmpBeginDirectiveParser {
+  using resultType = OmpDirectiveSpecification;
+
+  constexpr OmpBeginDirectiveParser(llvm::omp::Directive dir) : dir_(dir) {}
+
+  std::optional<resultType> Parse(ParseState &state) const {
+    auto &&p{predicated(Parser<OmpDirectiveName>{}, IsDirective(dir_)) >=
+        Parser<OmpDirectiveSpecification>{}};
+    return p.Parse(state);
+  }
+
+private:
+  llvm::omp::Directive dir_;
+};
+
 struct OmpEndDirectiveParser {
   using resultType = OmpDirectiveSpecification;
 
   constexpr OmpEndDirectiveParser(llvm::omp::Directive dir) : dir_(dir) {}
 
   std::optional<resultType> Parse(ParseState &state) const {
-    if ((startOmpLine >> "END"_sptok).Parse(state)) {
-      auto &&dirSpec{Parser<OmpDirectiveSpecification>{}.Parse(state)};
-      if (dirSpec && dirSpec->DirId() == dir_) {
-        return std::move(dirSpec);
+    if (startOmpLine.Parse(state)) {
+      if (auto endToken{verbatim("END"_sptok).Parse(state)}) {
+        if (auto &&dirSpec{OmpBeginDirectiveParser(dir_).Parse(state)}) {
+          // Extend the "source" on both the OmpDirectiveName and the
+          // OmpDirectiveNameSpecification.
+          CharBlock &nameSource{std::get<OmpDirectiveName>(dirSpec->t).source};
+          nameSource.ExtendToCover(endToken->source);
+          dirSpec->source.ExtendToCover(endToken->source);
+          return std::move(*dirSpec);
+        }
       }
     }
     return std::nullopt;
@@ -1390,57 +1415,67 @@ struct OmpEndDirectiveParser {
   llvm::omp::Directive dir_;
 };
 
-struct OmpAllocatorsConstructParser {
-  using resultType = OpenMPAllocatorsConstruct;
+struct OmpStatementConstructParser {
+  using resultType = OmpBlockConstruct;
+
+  constexpr OmpStatementConstructParser(llvm::omp::Directive dir) : dir_(dir) {}
 
   std::optional<resultType> Parse(ParseState &state) const {
-    auto dirSpec{Parser<OmpDirectiveSpecification>{}.Parse(state)};
-    if (!dirSpec || dirSpec->DirId() != llvm::omp::Directive::OMPD_allocators) {
-      return std::nullopt;
-    }
+    if (auto begin{OmpBeginDirectiveParser(dir_).Parse(state)}) {
+      Block body;
+      if (auto stmt{attempt(Parser<ExecutionPartConstruct>{}).Parse(state)}) {
+        body.emplace_back(std::move(*stmt));
+      }
+      // Allow empty block. Check for this in semantics.
 
-    // This should be an allocate-stmt. That will be checked in semantics.
-    Block block;
-    if (auto stmt{attempt(Parser<ExecutionPartConstruct>{}).Parse(state)}) {
-      block.emplace_back(std::move(*stmt));
+      auto end{maybe(OmpEndDirectiveParser{dir_}).Parse(state)};
+      return OmpBlockConstruct{OmpBeginDirective(std::move(*begin)),
+          std::move(body),
+          llvm::transformOptional(std::move(*end),
+              [](auto &&s) { return OmpEndDirective(std::move(s)); })};
     }
-    // Allow empty block. Check for this in semantics.
-
-    auto end{OmpEndDirectiveParser{llvm::omp::Directive::OMPD_allocators}};
-    return OpenMPAllocatorsConstruct{
-        std::move(*dirSpec), std::move(block), *maybe(end).Parse(state)};
+    return std::nullopt;
   }
+
+private:
+  llvm::omp::Directive dir_;
 };
 
-TYPE_PARSER(sourced( //
-    construct<OpenMPAllocatorsConstruct>(
-        "ALLOCATORS"_tok >= OmpAllocatorsConstructParser{})))
+struct OmpBlockConstructParser {
+  using resultType = OmpBlockConstruct;
 
-struct OmpDispatchConstructParser {
-  using resultType = OpenMPDispatchConstruct;
+  constexpr OmpBlockConstructParser(llvm::omp::Directive dir) : dir_(dir) {}
 
   std::optional<resultType> Parse(ParseState &state) const {
-    auto dirSpec{Parser<OmpDirectiveSpecification>{}.Parse(state)};
-    if (!dirSpec || dirSpec->DirId() != llvm::omp::Directive::OMPD_dispatch) {
-      return std::nullopt;
-    }
-
-    // This should be a function call. That will be checked in semantics.
-    Block block;
-    if (auto stmt{attempt(Parser<ExecutionPartConstruct>{}).Parse(state)}) {
-      block.emplace_back(std::move(*stmt));
+    if (auto &&begin{OmpBeginDirectiveParser(dir_).Parse(state)}) {
+      if (auto &&body{attempt(StrictlyStructuredBlockParser{}).Parse(state)}) {
+        // Try strictly-structured block with an optional end-directive
+        auto end{maybe(OmpEndDirectiveParser{dir_}).Parse(state)};
+        return OmpBlockConstruct{OmpBeginDirective(std::move(*begin)),
+            std::move(*body),
+            llvm::transformOptional(std::move(*end),
+                [](auto &&s) { return OmpEndDirective(std::move(s)); })};
+      } else if (auto &&body{
+                     attempt(LooselyStructuredBlockParser{}).Parse(state)}) {
+        // Try loosely-structured block with a mandatory end-directive
+        if (auto end{OmpEndDirectiveParser{dir_}.Parse(state)}) {
+          return OmpBlockConstruct{OmpBeginDirective(std::move(*begin)),
+              std::move(*body), OmpEndDirective{std::move(*end)}};
+        }
+      }
     }
-    // Allow empty block. Check for this in semantics.
-
-    auto end{OmpEndDirectiveParser{llvm::omp::Directive::OMPD_dispatch}};
-    return OpenMPDispatchConstruct{
-        std::move(*dirSpec), std::move(block), *maybe(end).Parse(state)};
+    return std::nullopt;
   }
+
+private:
+  llvm::omp::Directive dir_;
 };
 
-TYPE_PARSER(sourced( //
-    construct<OpenMPDispatchConstruct>(
-        "DISPATCH"_tok >= OmpDispatchConstructParser{})))
+TYPE_PARSER(sourced(construct<OpenMPAllocatorsConstruct>(
+    OmpStatementConstructParser{llvm::omp::Directive::OMPD_allocators})))
+
+TYPE_PARSER(sourced(construct<OpenMPDispatchConstruct>(
+    OmpStatementConstructParser{llvm::omp::Directive::OMPD_dispatch})))
 
 // Parser for an arbitrary OpenMP ATOMIC construct.
 //
@@ -1505,8 +1540,10 @@ struct OmpAtomicConstructParser {
         }
       }
       recursing_ = false;
-      return OpenMPAtomicConstruct{
-          std::move(*dirSpec), std::move(tail.first), std::move(tail.second)};
+      return OpenMPAtomicConstruct{OmpBeginDirective(std::move(*dirSpec)),
+          std::move(tail.first),
+          llvm::transformOptional(std::move(tail.second),
+              [](auto &&s) { return OmpEndDirective(std::move(s)); })};
     }
 
     recursing_ = false;
@@ -1607,10 +1644,6 @@ TYPE_PARSER(sourced( //
         predicated(OmpDirectiveName...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Jul 28, 2025

@llvm/pr-subscribers-flang-parser

Author: Krzysztof Parzyszek (kparzysz)

Changes

The structure is

  • OmpBeginDirective (aka OmpDirectiveSpecification)
  • Block
  • optional<OmpEndDirective> (aka optional<OmpDirectiveSpecification>)

The OmpBeginDirective and OmpEndDirective are effectively different names for OmpDirectiveSpecification. They exist to allow the semantic analyses to distinguish between the beginning and the ending of a block construct without maintaining additional context.

The actual changes are in the parser: parse-tree.h and openmp-parser.cpp in particular. The rest is simply changing the way the directive/clause information is accessed (typically for the simpler).

All standalone and block constructs now use OmpDirectiveSpecification to store the directive/clause information.


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

38 Files Affected:

  • (modified) flang/examples/FeatureList/FeatureList.cpp (+2-3)
  • (modified) flang/include/flang/Parser/dump-parse-tree.h (+2-3)
  • (modified) flang/include/flang/Parser/openmp-utils.h (+2-9)
  • (modified) flang/include/flang/Parser/parse-tree.h (+43-40)
  • (modified) flang/lib/Lower/OpenMP/Atomic.cpp (+1-1)
  • (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+14-35)
  • (modified) flang/lib/Parser/openmp-parsers.cpp (+100-92)
  • (modified) flang/lib/Parser/parse-tree.cpp (+3-3)
  • (modified) flang/lib/Parser/unparse.cpp (+20-80)
  • (modified) flang/lib/Semantics/check-omp-atomic.cpp (+3-5)
  • (modified) flang/lib/Semantics/check-omp-loop.cpp (+5-9)
  • (modified) flang/lib/Semantics/check-omp-structure.cpp (+50-65)
  • (modified) flang/lib/Semantics/check-omp-structure.h (+3-3)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+21-13)
  • (modified) flang/lib/Semantics/resolve-names.cpp (+5-7)
  • (modified) flang/test/Parser/OpenMP/affinity-clause.f90 (+6-6)
  • (modified) flang/test/Parser/OpenMP/allocators-unparse.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/atomic-compare.f90 (+9-9)
  • (modified) flang/test/Parser/OpenMP/atomic-end.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/block-construct.f90 (+10-10)
  • (modified) flang/test/Parser/OpenMP/construct-prefix-conflict.f90 (+20-20)
  • (modified) flang/test/Parser/OpenMP/defaultmap-clause.f90 (+12-12)
  • (modified) flang/test/Parser/OpenMP/defaultmap-unparse.f90 (+18-18)
  • (modified) flang/test/Parser/OpenMP/dispatch.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/if-clause.f90 (+2-2)
  • (modified) flang/test/Parser/OpenMP/in-reduction-clause.f90 (+6-6)
  • (modified) flang/test/Parser/OpenMP/map-modifiers-v60.f90 (+10-10)
  • (modified) flang/test/Parser/OpenMP/map-modifiers.f90 (+23-23)
  • (modified) flang/test/Parser/OpenMP/masked-unparse.f90 (+6-6)
  • (modified) flang/test/Parser/OpenMP/master-unparse.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/openmp6-directive-spellings.f90 (+6-6)
  • (modified) flang/test/Parser/OpenMP/proc-bind.f90 (+2-2)
  • (modified) flang/test/Parser/OpenMP/scope.f90 (+4-4)
  • (modified) flang/test/Parser/OpenMP/target_device_parse.f90 (+32-32)
  • (modified) flang/test/Parser/OpenMP/task-reduction-clause.f90 (+2-2)
  • (modified) flang/test/Parser/OpenMP/task.f90 (+1-1)
  • (modified) flang/test/Semantics/OpenMP/clause-validity01.f90 (+1-2)
  • (modified) flang/test/Semantics/OpenMP/symbol08.f90 (+1-2)
diff --git a/flang/examples/FeatureList/FeatureList.cpp b/flang/examples/FeatureList/FeatureList.cpp
index 64b57b633feaf..0b8066e36312b 100644
--- a/flang/examples/FeatureList/FeatureList.cpp
+++ b/flang/examples/FeatureList/FeatureList.cpp
@@ -445,10 +445,9 @@ struct NodeVisitor {
   READ_FEATURE(ObjectDecl)
   READ_FEATURE(OldParameterStmt)
   READ_FEATURE(OmpAlignedClause)
-  READ_FEATURE(OmpBeginBlockDirective)
+  READ_FEATURE(OmpBeginDirective)
   READ_FEATURE(OmpBeginLoopDirective)
   READ_FEATURE(OmpBeginSectionsDirective)
-  READ_FEATURE(OmpBlockDirective)
   READ_FEATURE(OmpClause)
   READ_FEATURE(OmpClauseList)
   READ_FEATURE(OmpCriticalDirective)
@@ -472,7 +471,7 @@ struct NodeVisitor {
   READ_FEATURE(OmpIteration)
   READ_FEATURE(OmpIterationOffset)
   READ_FEATURE(OmpIterationVector)
-  READ_FEATURE(OmpEndBlockDirective)
+  READ_FEATURE(OmpEndDirective)
   READ_FEATURE(OmpEndCriticalDirective)
   READ_FEATURE(OmpEndLoopDirective)
   READ_FEATURE(OmpEndSectionsDirective)
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index ebac54f6e29ba..b09dd32a90b98 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -534,10 +534,8 @@ class ParseTreeDumper {
   NODE(parser, OmpAtClause)
   NODE_ENUM(OmpAtClause, ActionTime)
   NODE_ENUM(OmpSeverityClause, Severity)
-  NODE(parser, OmpBeginBlockDirective)
   NODE(parser, OmpBeginLoopDirective)
   NODE(parser, OmpBeginSectionsDirective)
-  NODE(parser, OmpBlockDirective)
   static std::string GetNodeName(const llvm::omp::Directive &x) {
     return llvm::Twine("llvm::omp::Directive = ",
         llvm::omp::getOpenMPDirectiveName(x, llvm::omp::FallbackVersion))
@@ -584,7 +582,6 @@ class ParseTreeDumper {
   NODE(parser, OmpDetachClause)
   NODE(parser, OmpDoacrossClause)
   NODE(parser, OmpDestroyClause)
-  NODE(parser, OmpEndBlockDirective)
   NODE(parser, OmpEndCriticalDirective)
   NODE(parser, OmpEndLoopDirective)
   NODE(parser, OmpEndSectionsDirective)
@@ -704,6 +701,8 @@ class ParseTreeDumper {
   NODE(parser, OpenMPDeclarativeAssumes)
   NODE(parser, OmpAssumeDirective)
   NODE(parser, OmpEndAssumeDirective)
+  NODE(parser, OmpBeginDirective)
+  NODE(parser, OmpEndDirective)
   NODE(parser, OpenMPAtomicConstruct)
   NODE(parser, OpenMPBlockConstruct)
   NODE(parser, OpenMPCancelConstruct)
diff --git a/flang/include/flang/Parser/openmp-utils.h b/flang/include/flang/Parser/openmp-utils.h
index 41c04424e91c6..fa0f7656cd5d8 100644
--- a/flang/include/flang/Parser/openmp-utils.h
+++ b/flang/include/flang/Parser/openmp-utils.h
@@ -68,11 +68,6 @@ struct DirectiveNameScope {
     return MakeName(x.source, llvm::omp::Directive::OMPD_nothing);
   }
 
-  static OmpDirectiveName GetOmpDirectiveName(const OmpBeginBlockDirective &x) {
-    auto &dir{std::get<OmpBlockDirective>(x.t)};
-    return MakeName(dir.source, dir.v);
-  }
-
   static OmpDirectiveName GetOmpDirectiveName(const OmpBeginLoopDirective &x) {
     auto &dir{std::get<OmpLoopDirective>(x.t)};
     return MakeName(dir.source, dir.v);
@@ -106,10 +101,8 @@ struct DirectiveNameScope {
         return GetOmpDirectiveName(x.v);
       }
     } else if constexpr (TupleTrait<T>) {
-      if constexpr (std::is_same_v<T, OpenMPAllocatorsConstruct> ||
-          std::is_same_v<T, OpenMPAtomicConstruct> ||
-          std::is_same_v<T, OpenMPDispatchConstruct>) {
-        return std::get<OmpDirectiveSpecification>(x.t).DirName();
+      if constexpr (std::is_base_of_v<OmpBlockConstruct, T>) {
+        return std::get<OmpBeginDirective>(x.t).DirName();
       } else if constexpr (std::is_same_v<T, OmpAssumeDirective> ||
           std::is_same_v<T, OmpCriticalDirective> ||
           std::is_same_v<T, OmpDeclareVariantDirective> ||
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index da0a3632b8a63..7ba5fe964f7a1 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3469,6 +3469,13 @@ WRAPPER_CLASS(PauseStmt, std::optional<StopCode>);
 
 // --- Common definitions
 
+#define INHERITED_TUPLE_CLASS_BOILERPLATE(classname, basename) \
+  template <typename... Ts, typename = common::NoLvalue<Ts...>> \
+  classname(Ts &&...args) : basename(std::move(args)...) {} \
+  classname(basename &&b) : basename(std::move(b)) {} \
+  using TupleTrait = std::true_type; \
+  BOILERPLATE(classname)
+
 #define INHERITED_WRAPPER_CLASS_BOILERPLATE(classname, basename) \
   BOILERPLATE(classname); \
   classname(decltype(basename::v) &&x) : basename(std::move(x)) {} \
@@ -4729,6 +4736,33 @@ struct OmpDirectiveSpecification {
       t;
 };
 
+// OmpBeginDirective and OmpEndDirective are needed for semantic analysis,
+// where some checks are done specifically for either the begin or the end
+// directive. The structure of both is identical, but the diffent types
+// allow to distinguish them in the type-based parse-tree visitor.
+struct OmpBeginDirective : public OmpDirectiveSpecification {
+  INHERITED_TUPLE_CLASS_BOILERPLATE(
+      OmpBeginDirective, OmpDirectiveSpecification);
+};
+
+struct OmpEndDirective : public OmpDirectiveSpecification {
+  INHERITED_TUPLE_CLASS_BOILERPLATE(OmpEndDirective, OmpDirectiveSpecification);
+};
+
+// Common base class for block-associated constructs.
+struct OmpBlockConstruct {
+  TUPLE_CLASS_BOILERPLATE(OmpBlockConstruct);
+  const OmpBeginDirective &BeginDir() const {
+    return std::get<OmpBeginDirective>(t);
+  }
+  const std::optional<OmpEndDirective> &EndDir() const {
+    return std::get<std::optional<OmpEndDirective>>(t);
+  }
+
+  CharBlock source;
+  std::tuple<OmpBeginDirective, Block, std::optional<OmpEndDirective>> t;
+};
+
 struct OmpMetadirectiveDirective {
   TUPLE_CLASS_BOILERPLATE(OmpMetadirectiveDirective);
   std::tuple<Verbatim, OmpClauseList> t;
@@ -4833,12 +4867,6 @@ struct OpenMPSectionsConstruct {
       t;
 };
 
-// OpenMP directive beginning or ending a block
-struct OmpBlockDirective {
-  WRAPPER_CLASS_BOILERPLATE(OmpBlockDirective, llvm::omp::Directive);
-  CharBlock source;
-};
-
 struct OmpDeclareVariantDirective {
   TUPLE_CLASS_BOILERPLATE(OmpDeclareVariantDirective);
   CharBlock source;
@@ -4963,12 +4991,9 @@ struct OpenMPExecutableAllocate {
 //    ALLOCATORS [allocate-clause...]
 //    block
 //    [END ALLOCATORS]
-struct OpenMPAllocatorsConstruct {
-  TUPLE_CLASS_BOILERPLATE(OpenMPAllocatorsConstruct);
-  CharBlock source;
-  std::tuple<OmpDirectiveSpecification, Block,
-      std::optional<OmpDirectiveSpecification>>
-      t;
+struct OpenMPAllocatorsConstruct : public OmpBlockConstruct {
+  INHERITED_TUPLE_CLASS_BOILERPLATE(
+      OpenMPAllocatorsConstruct, OmpBlockConstruct);
 };
 
 // 2.17.7 Atomic construct/2.17.8 Flush construct [OpenMP 5.0]
@@ -4982,15 +5007,11 @@ struct OmpMemoryOrderClause {
   CharBlock source;
 };
 
-struct OpenMPAtomicConstruct {
+struct OpenMPAtomicConstruct : public OmpBlockConstruct {
   llvm::omp::Clause GetKind() const;
   bool IsCapture() const;
   bool IsCompare() const;
-  TUPLE_CLASS_BOILERPLATE(OpenMPAtomicConstruct);
-  CharBlock source;
-  std::tuple<OmpDirectiveSpecification, Block,
-      std::optional<OmpDirectiveSpecification>>
-      t;
+  INHERITED_TUPLE_CLASS_BOILERPLATE(OpenMPAtomicConstruct, OmpBlockConstruct);
 
   // Information filled out during semantic checks to avoid duplication
   // of analyses.
@@ -5054,12 +5075,8 @@ struct OpenMPDepobjConstruct {
 //                    nocontext-clause |
 //                    novariants-clause |
 //                    nowait-clause
-struct OpenMPDispatchConstruct {
-  TUPLE_CLASS_BOILERPLATE(OpenMPDispatchConstruct);
-  CharBlock source;
-  std::tuple<OmpDirectiveSpecification, Block,
-      std::optional<OmpDirectiveSpecification>>
-      t;
+struct OpenMPDispatchConstruct : public OmpBlockConstruct {
+  INHERITED_TUPLE_CLASS_BOILERPLATE(OpenMPDispatchConstruct, OmpBlockConstruct);
 };
 
 // [4.5:162-165], [5.0:242-246], [5.1:275-279], [5.2:315-316], [6.0:498-500]
@@ -5114,22 +5131,8 @@ struct OmpEndLoopDirective {
   CharBlock source;
 };
 
-struct OmpBeginBlockDirective {
-  TUPLE_CLASS_BOILERPLATE(OmpBeginBlockDirective);
-  std::tuple<OmpBlockDirective, OmpClauseList> t;
-  CharBlock source;
-};
-
-struct OmpEndBlockDirective {
-  TUPLE_CLASS_BOILERPLATE(OmpEndBlockDirective);
-  std::tuple<OmpBlockDirective, OmpClauseList> t;
-  CharBlock source;
-};
-
-struct OpenMPBlockConstruct {
-  TUPLE_CLASS_BOILERPLATE(OpenMPBlockConstruct);
-  std::tuple<OmpBeginBlockDirective, Block, std::optional<OmpEndBlockDirective>>
-      t;
+struct OpenMPBlockConstruct : public OmpBlockConstruct {
+  INHERITED_TUPLE_CLASS_BOILERPLATE(OpenMPBlockConstruct, OmpBlockConstruct);
 };
 
 // OpenMP directives enclosing do loop
diff --git a/flang/lib/Lower/OpenMP/Atomic.cpp b/flang/lib/Lower/OpenMP/Atomic.cpp
index 9a233d2d8cb08..623a9370aa00d 100644
--- a/flang/lib/Lower/OpenMP/Atomic.cpp
+++ b/flang/lib/Lower/OpenMP/Atomic.cpp
@@ -699,7 +699,7 @@ void Fortran::lower::omp::lowerAtomic(
   };
 
   fir::FirOpBuilder &builder = converter.getFirOpBuilder();
-  auto &dirSpec = std::get<parser::OmpDirectiveSpecification>(construct.t);
+  const parser::OmpDirectiveSpecification &dirSpec = construct.BeginDir();
   omp::List<omp::Clause> clauses = makeClauses(dirSpec.Clauses(), semaCtx);
   lower::StatementContext stmtCtx;
 
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 6dd4b16a1fb55..68ebb66309f99 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -407,16 +407,9 @@ static void processHostEvalClauses(lower::AbstractConverter &converter,
     common::visit(
         common::visitors{
             [&](const parser::OpenMPBlockConstruct &ompConstruct) {
-              const auto &beginDirective =
-                  std::get<parser::OmpBeginBlockDirective>(ompConstruct.t);
-              beginClauseList =
-                  &std::get<parser::OmpClauseList>(beginDirective.t);
-              if (auto &endDirective =
-                      std::get<std::optional<parser::OmpEndBlockDirective>>(
-                          ompConstruct.t)) {
-                endClauseList =
-                    &std::get<parser::OmpClauseList>(endDirective->t);
-              }
+              beginClauseList = &ompConstruct.BeginDir().Clauses();
+              if (auto &endSpec = ompConstruct.EndDir())
+                endClauseList = &endSpec->Clauses();
             },
             [&](const parser::OpenMPLoopConstruct &ompConstruct) {
               const auto &beginDirective =
@@ -3716,25 +3709,16 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
                    semantics::SemanticsContext &semaCtx,
                    lower::pft::Evaluation &eval,
                    const parser::OpenMPBlockConstruct &blockConstruct) {
-  const auto &beginBlockDirective =
-      std::get<parser::OmpBeginBlockDirective>(blockConstruct.t);
-  mlir::Location currentLocation =
-      converter.genLocation(beginBlockDirective.source);
-  const auto origDirective =
-      std::get<parser::OmpBlockDirective>(beginBlockDirective.t).v;
-  List<Clause> clauses = makeClauses(
-      std::get<parser::OmpClauseList>(beginBlockDirective.t), semaCtx);
-
-  if (const auto &endBlockDirective =
-          std::get<std::optional<parser::OmpEndBlockDirective>>(
-              blockConstruct.t)) {
-    clauses.append(makeClauses(
-        std::get<parser::OmpClauseList>(endBlockDirective->t), semaCtx));
-  }
-
-  assert(llvm::omp::blockConstructSet.test(origDirective) &&
+  const parser::OmpDirectiveSpecification &beginSpec =
+      blockConstruct.BeginDir();
+  List<Clause> clauses = makeClauses(beginSpec.Clauses(), semaCtx);
+  if (auto &endSpec = blockConstruct.EndDir())
+    clauses.append(makeClauses(endSpec->Clauses(), semaCtx));
+
+  llvm::omp::Directive directive = beginSpec.DirId();
+  assert(llvm::omp::blockConstructSet.test(directive) &&
          "Expected block construct");
-  (void)origDirective;
+  mlir::Location currentLocation = converter.genLocation(beginSpec.source);
 
   for (const Clause &clause : clauses) {
     mlir::Location clauseLocation = converter.genLocation(clause.source);
@@ -3777,13 +3761,9 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
     }
   }
 
-  llvm::omp::Directive directive =
-      std::get<parser::OmpBlockDirective>(beginBlockDirective.t).v;
-  const parser::CharBlock &source =
-      std::get<parser::OmpBlockDirective>(beginBlockDirective.t).source;
   ConstructQueue queue{
       buildConstructQueue(converter.getFirOpBuilder().getModule(), semaCtx,
-                          eval, source, directive, clauses)};
+                          eval, beginSpec.source, directive, clauses)};
   genOMPDispatch(converter, symTable, semaCtx, eval, currentLocation, queue,
                  queue.begin());
 }
@@ -4071,8 +4051,7 @@ bool Fortran::lower::isOpenMPTargetConstruct(
     const parser::OpenMPConstruct &omp) {
   llvm::omp::Directive dir = llvm::omp::Directive::OMPD_unknown;
   if (const auto *block = std::get_if<parser::OpenMPBlockConstruct>(&omp.u)) {
-    const auto &begin = std::get<parser::OmpBeginBlockDirective>(block->t);
-    dir = std::get<parser::OmpBlockDirective>(begin.t).v;
+    dir = block->BeginDir().DirId();
   } else if (const auto *loop =
                  std::get_if<parser::OpenMPLoopConstruct>(&omp.u)) {
     const auto &begin = std::get<parser::OmpBeginLoopDirective>(loop->t);
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 8c780388b9cb1..c2c1921f446c7 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -1371,16 +1371,41 @@ TYPE_PARSER(sourced(construct<OmpLoopDirective>(first(
 TYPE_PARSER(sourced(construct<OmpBeginLoopDirective>(
     sourced(Parser<OmpLoopDirective>{}), Parser<OmpClauseList>{})))
 
+static inline constexpr auto IsDirective(llvm::omp::Directive dir) {
+  return [dir](const OmpDirectiveName &name) -> bool { return dir == name.v; };
+}
+
+struct OmpBeginDirectiveParser {
+  using resultType = OmpDirectiveSpecification;
+
+  constexpr OmpBeginDirectiveParser(llvm::omp::Directive dir) : dir_(dir) {}
+
+  std::optional<resultType> Parse(ParseState &state) const {
+    auto &&p{predicated(Parser<OmpDirectiveName>{}, IsDirective(dir_)) >=
+        Parser<OmpDirectiveSpecification>{}};
+    return p.Parse(state);
+  }
+
+private:
+  llvm::omp::Directive dir_;
+};
+
 struct OmpEndDirectiveParser {
   using resultType = OmpDirectiveSpecification;
 
   constexpr OmpEndDirectiveParser(llvm::omp::Directive dir) : dir_(dir) {}
 
   std::optional<resultType> Parse(ParseState &state) const {
-    if ((startOmpLine >> "END"_sptok).Parse(state)) {
-      auto &&dirSpec{Parser<OmpDirectiveSpecification>{}.Parse(state)};
-      if (dirSpec && dirSpec->DirId() == dir_) {
-        return std::move(dirSpec);
+    if (startOmpLine.Parse(state)) {
+      if (auto endToken{verbatim("END"_sptok).Parse(state)}) {
+        if (auto &&dirSpec{OmpBeginDirectiveParser(dir_).Parse(state)}) {
+          // Extend the "source" on both the OmpDirectiveName and the
+          // OmpDirectiveNameSpecification.
+          CharBlock &nameSource{std::get<OmpDirectiveName>(dirSpec->t).source};
+          nameSource.ExtendToCover(endToken->source);
+          dirSpec->source.ExtendToCover(endToken->source);
+          return std::move(*dirSpec);
+        }
       }
     }
     return std::nullopt;
@@ -1390,57 +1415,67 @@ struct OmpEndDirectiveParser {
   llvm::omp::Directive dir_;
 };
 
-struct OmpAllocatorsConstructParser {
-  using resultType = OpenMPAllocatorsConstruct;
+struct OmpStatementConstructParser {
+  using resultType = OmpBlockConstruct;
+
+  constexpr OmpStatementConstructParser(llvm::omp::Directive dir) : dir_(dir) {}
 
   std::optional<resultType> Parse(ParseState &state) const {
-    auto dirSpec{Parser<OmpDirectiveSpecification>{}.Parse(state)};
-    if (!dirSpec || dirSpec->DirId() != llvm::omp::Directive::OMPD_allocators) {
-      return std::nullopt;
-    }
+    if (auto begin{OmpBeginDirectiveParser(dir_).Parse(state)}) {
+      Block body;
+      if (auto stmt{attempt(Parser<ExecutionPartConstruct>{}).Parse(state)}) {
+        body.emplace_back(std::move(*stmt));
+      }
+      // Allow empty block. Check for this in semantics.
 
-    // This should be an allocate-stmt. That will be checked in semantics.
-    Block block;
-    if (auto stmt{attempt(Parser<ExecutionPartConstruct>{}).Parse(state)}) {
-      block.emplace_back(std::move(*stmt));
+      auto end{maybe(OmpEndDirectiveParser{dir_}).Parse(state)};
+      return OmpBlockConstruct{OmpBeginDirective(std::move(*begin)),
+          std::move(body),
+          llvm::transformOptional(std::move(*end),
+              [](auto &&s) { return OmpEndDirective(std::move(s)); })};
     }
-    // Allow empty block. Check for this in semantics.
-
-    auto end{OmpEndDirectiveParser{llvm::omp::Directive::OMPD_allocators}};
-    return OpenMPAllocatorsConstruct{
-        std::move(*dirSpec), std::move(block), *maybe(end).Parse(state)};
+    return std::nullopt;
   }
+
+private:
+  llvm::omp::Directive dir_;
 };
 
-TYPE_PARSER(sourced( //
-    construct<OpenMPAllocatorsConstruct>(
-        "ALLOCATORS"_tok >= OmpAllocatorsConstructParser{})))
+struct OmpBlockConstructParser {
+  using resultType = OmpBlockConstruct;
 
-struct OmpDispatchConstructParser {
-  using resultType = OpenMPDispatchConstruct;
+  constexpr OmpBlockConstructParser(llvm::omp::Directive dir) : dir_(dir) {}
 
   std::optional<resultType> Parse(ParseState &state) const {
-    auto dirSpec{Parser<OmpDirectiveSpecification>{}.Parse(state)};
-    if (!dirSpec || dirSpec->DirId() != llvm::omp::Directive::OMPD_dispatch) {
-      return std::nullopt;
-    }
-
-    // This should be a function call. That will be checked in semantics.
-    Block block;
-    if (auto stmt{attempt(Parser<ExecutionPartConstruct>{}).Parse(state)}) {
-      block.emplace_back(std::move(*stmt));
+    if (auto &&begin{OmpBeginDirectiveParser(dir_).Parse(state)}) {
+      if (auto &&body{attempt(StrictlyStructuredBlockParser{}).Parse(state)}) {
+        // Try strictly-structured block with an optional end-directive
+        auto end{maybe(OmpEndDirectiveParser{dir_}).Parse(state)};
+        return OmpBlockConstruct{OmpBeginDirective(std::move(*begin)),
+            std::move(*body),
+            llvm::transformOptional(std::move(*end),
+                [](auto &&s) { return OmpEndDirective(std::move(s)); })};
+      } else if (auto &&body{
+                     attempt(LooselyStructuredBlockParser{}).Parse(state)}) {
+        // Try loosely-structured block with a mandatory end-directive
+        if (auto end{OmpEndDirectiveParser{dir_}.Parse(state)}) {
+          return OmpBlockConstruct{OmpBeginDirective(std::move(*begin)),
+              std::move(*body), OmpEndDirective{std::move(*end)}};
+        }
+      }
     }
-    // Allow empty block. Check for this in semantics.
-
-    auto end{OmpEndDirectiveParser{llvm::omp::Directive::OMPD_dispatch}};
-    return OpenMPDispatchConstruct{
-        std::move(*dirSpec), std::move(block), *maybe(end).Parse(state)};
+    return std::nullopt;
   }
+
+private:
+  llvm::omp::Directive dir_;
 };
 
-TYPE_PARSER(sourced( //
-    construct<OpenMPDispatchConstruct>(
-        "DISPATCH"_tok >= OmpDispatchConstructParser{})))
+TYPE_PARSER(sourced(construct<OpenMPAllocatorsConstruct>(
+    OmpStatementConstructParser{llvm::omp::Directive::OMPD_allocators})))
+
+TYPE_PARSER(sourced(construct<OpenMPDispatchConstruct>(
+    OmpStatementConstructParser{llvm::omp::Directive::OMPD_dispatch})))
 
 // Parser for an arbitrary OpenMP ATOMIC construct.
 //
@@ -1505,8 +1540,10 @@ struct OmpAtomicConstructParser {
         }
       }
       recursing_ = false;
-      return OpenMPAtomicConstruct{
-          std::move(*dirSpec), std::move(tail.first), std::move(tail.second)};
+      return OpenMPAtomicConstruct{OmpBeginDirective(std::move(*dirSpec)),
+          std::move(tail.first),
+          llvm::transformOptional(std::move(tail.second),
+              [](auto &&s) { return OmpEndDirective(std::move(s)); })};
     }
 
     recursing_ = false;
@@ -1607,10 +1644,6 @@ TYPE_PARSER(sourced( //
         predicated(OmpDirectiveName...
[truncated]

@kparzysz
Copy link
Contributor Author

Part of the reason to do this is that it will make dealing with the hundreds of new compound directives easier. The next step would be to make the parser parse constructs based on properties (such as association), not names or directive id's.

Base automatically changed from users/kparzysz/a03-getsource-dsa to main July 31, 2025 13:20
Comment on lines -166 to -167
!ERROR: Unmatched END TARGET directive
!$omp end target
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? What happens instead of this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We get a syntax error. Our current end-directive parser expects "END" followed by the specific directive. I've been thinking about making it accept any end-directive. That way we could retain the above error, but I haven't tried it, so I don't know what else would happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This behavior is less user friendly than before, so it needs to be addressed eventually. I was planning to do that in a future PR.

Comment on lines -133 to -134
!$omp teams num_teams(num_teams) thread_limit(block_threads) reduction(+: sum&
!$OMP&)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current output doesn't have the two spaces between "teams" and "num_teams". The only character in the continuation line was ), and now it all fits in a single line.

As to why this happens, I think the unparser for OmpDirectiveSpecification doesn't print the extra space, whereas the previous unparsers did.

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

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

Thanks for answering my questions.

@kparzysz kparzysz merged commit 6533ad0 into main Aug 1, 2025
9 checks passed
@kparzysz kparzysz deleted the users/kparzysz/a04-block-construct-structure branch August 1, 2025 12:53
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.

3 participants