[flang][OpenMP] Simplify handling of UserReductionDetails a bit

[kparzysz]

Instead of having a variant with specific AST nodes that can contain a reduction specifier, simply store the OpenMPDeclarativeConstruct. It is used to emit the source code directive when generating a module file, and unparsing the top-level AST node will work just fine.

[llvmbot]

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-parser

Author: Krzysztof Parzyszek (kparzysz)

Changes

Instead of having a variant with specific AST nodes that can contain a reduction specifier, simply store the OpenMPDeclarativeConstruct. It is used to emit the source code directive when generating a module file, and unparsing the top-level AST node will work just fine.

---

Full diff: https://github.com/llvm/llvm-project/pull/159944.diff

4 Files Affected:

• (modified) flang/include/flang/Semantics/symbol.h (+5-6)
• (modified) flang/lib/Parser/unparse.cpp (+2-5)
• (modified) flang/lib/Semantics/mod-file.cpp (+2-13)
• (modified) flang/lib/Semantics/resolve-names.cpp (+9-11)

diff --git a/flang/include/flang/Semantics/symbol.h b/flang/include/flang/Semantics/symbol.h
index 774fc9873f7bc..e90e9c617805d 100644
--- a/flang/include/flang/Semantics/symbol.h
+++ b/flang/include/flang/Semantics/symbol.h
@@ -30,8 +30,7 @@ class raw_ostream;
 }
 namespace Fortran::parser {
 struct Expr;
-struct OpenMPDeclareReductionConstruct;
-struct OmpMetadirectiveDirective;
+struct OpenMPDeclarativeConstruct;
 }
 
 namespace Fortran::semantics {
@@ -736,9 +735,7 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &, const GenericDetails &);
 class UserReductionDetails {
 public:
   using TypeVector = std::vector<const DeclTypeSpec *>;
-  using DeclInfo = std::variant<const parser::OpenMPDeclareReductionConstruct *,
-      const parser::OmpMetadirectiveDirective *>;
-  using DeclVector = std::vector<DeclInfo>;
+  using DeclVector = std::vector<const parser::OpenMPDeclarativeConstruct *>;
 
   UserReductionDetails() = default;
 
@@ -756,7 +753,9 @@ class UserReductionDetails {
     return false;
   }
 
-  void AddDecl(const DeclInfo &decl) { declList_.emplace_back(decl); }
+  void AddDecl(const parser::OpenMPDeclarativeConstruct *decl) {
+    declList_.emplace_back(decl);
+  }
   const DeclVector &GetDeclList() const { return declList_; }
 
 private:
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 9d73bcafa0e15..2db60c9a78b0f 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -3082,11 +3082,8 @@ template void Unparse<Expr>(llvm::raw_ostream &, const Expr &,
     const common::LangOptions &, Encoding, bool, bool, preStatementType *,
     AnalyzedObjectsAsFortran *);
 
-template void Unparse<parser::OpenMPDeclareReductionConstruct>(
-    llvm::raw_ostream &, const parser::OpenMPDeclareReductionConstruct &,
+template void Unparse<parser::OpenMPDeclarativeConstruct>(
+    llvm::raw_ostream &, const parser::OpenMPDeclarativeConstruct &,
     const common::LangOptions &, Encoding, bool, bool, preStatementType *,
     AnalyzedObjectsAsFortran *);
-template void Unparse<parser::OmpMetadirectiveDirective>(llvm::raw_ostream &,
-    const parser::OmpMetadirectiveDirective &, const common::LangOptions &,
-    Encoding, bool, bool, preStatementType *, AnalyzedObjectsAsFortran *);
 } // namespace Fortran::parser
diff --git a/flang/lib/Semantics/mod-file.cpp b/flang/lib/Semantics/mod-file.cpp
index 82c8536902eb2..8074c94b41e1a 100644
--- a/flang/lib/Semantics/mod-file.cpp
+++ b/flang/lib/Semantics/mod-file.cpp
@@ -1056,19 +1056,8 @@ void ModFileWriter::PutUserReduction(
   // The module content for a OpenMP Declare Reduction is the OpenMP
   // declaration. There may be multiple declarations.
   // Decls are pointers, so do not use a reference.
-  for (const auto decl : details.GetDeclList()) {
-    common::visit( //
-        common::visitors{//
-            [&](const parser::OpenMPDeclareReductionConstruct *d) {
-              Unparse(os, *d, context_.langOptions());
-            },
-            [&](const parser::OmpMetadirectiveDirective *m) {
-              Unparse(os, *m, context_.langOptions());
-            },
-            [&](const auto &) {
-              DIE("Unknown OpenMP DECLARE REDUCTION content");
-            }},
-        decl);
+  for (const auto *decl : details.GetDeclList()) {
+    Unparse(os, *decl, context_.langOptions());
   }
 }
 
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index cdd8d6ff2f60e..69b8a45e6ceaa 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -1480,12 +1480,10 @@ class OmpVisitor : public virtual DeclarationVisitor {
   static bool NeedsScope(const parser::OmpClause &);
 
   bool Pre(const parser::OmpMetadirectiveDirective &x) { //
-    metaDirective_ = &x;
     ++metaLevel_;
     return true;
   }
   void Post(const parser::OmpMetadirectiveDirective &) { //
-    metaDirective_ = nullptr;
     --metaLevel_;
   }
 
@@ -1583,7 +1581,8 @@ class OmpVisitor : public virtual DeclarationVisitor {
     AddOmpSourceRange(x.source);
     ProcessReductionSpecifier(
         std::get<Indirection<parser::OmpReductionSpecifier>>(x.t).value(),
-        std::get<std::optional<parser::OmpClauseList>>(x.t), x);
+        std::get<std::optional<parser::OmpClauseList>>(x.t),
+        declaratives_.back());
     return false;
   }
   bool Pre(const parser::OmpMapClause &);
@@ -1684,9 +1683,11 @@ class OmpVisitor : public virtual DeclarationVisitor {
     // can implicitly declare variables instead of only using the
     // ones already declared in the Fortran sources.
     SkipImplicitTyping(true);
+    declaratives_.push_back(&x);
     return true;
   }
   void Post(const parser::OpenMPDeclarativeConstruct &) {
+    declaratives_.pop_back();
     SkipImplicitTyping(false);
     messageHandler().set_currStmtSource(std::nullopt);
   }
@@ -1728,15 +1729,14 @@ class OmpVisitor : public virtual DeclarationVisitor {
 private:
   void ProcessMapperSpecifier(const parser::OmpMapperSpecifier &spec,
       const parser::OmpClauseList &clauses);
-  template <typename T>
   void ProcessReductionSpecifier(const parser::OmpReductionSpecifier &spec,
       const std::optional<parser::OmpClauseList> &clauses,
-      const T &wholeConstruct);
+      const parser::OpenMPDeclarativeConstruct *wholeConstruct);
 
   void ResolveCriticalName(const parser::OmpArgument &arg);
 
   int metaLevel_{0};
-  const parser::OmpMetadirectiveDirective *metaDirective_{nullptr};
+  std::vector<const parser::OpenMPDeclarativeConstruct *> declaratives_;
 };
 
 bool OmpVisitor::NeedsScope(const parser::OmpBlockConstruct &x) {
@@ -1861,11 +1861,10 @@ std::string MangleDefinedOperator(const parser::CharBlock &name) {
   return "op" + name.ToString();
 }
 
-template <typename T>
 void OmpVisitor::ProcessReductionSpecifier(
     const parser::OmpReductionSpecifier &spec,
     const std::optional<parser::OmpClauseList> &clauses,
-    const T &wholeOmpConstruct) {
+    const parser::OpenMPDeclarativeConstruct *construct) {
   const parser::Name *name{nullptr};
   parser::CharBlock mangledName;
   UserReductionDetails reductionDetailsTemp;
@@ -1952,7 +1951,7 @@ void OmpVisitor::ProcessReductionSpecifier(
     PopScope();
   }
 
-  reductionDetails->AddDecl(&wholeOmpConstruct);
+  reductionDetails->AddDecl(construct);
 
   if (!symbol) {
     symbol = &MakeSymbol(mangledName, Attrs{}, std::move(*reductionDetails));
@@ -2017,8 +2016,7 @@ bool OmpVisitor::Pre(const parser::OmpDirectiveSpecification &x) {
     if (maybeArgs && maybeClauses) {
       const parser::OmpArgument &first{maybeArgs->v.front()};
       if (auto *spec{std::get_if<parser::OmpReductionSpecifier>(&first.u)}) {
-        CHECK(metaDirective_);
-        ProcessReductionSpecifier(*spec, maybeClauses, *metaDirective_);
+        ProcessReductionSpecifier(*spec, maybeClauses, declaratives_.back());
       }
     }
     break;

[github-actions]

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

[kparzysz]

PR stack

1. [flang][OpenMP] Simplify handling of UserReductionDetails a bit #159944 (this PR)
2. [flang][OpenMP] Stop tracking metadirective level in name resolution #159945
3. [flang][OpenMP] Resolve all components of OmpDirectiveSpecification #159946

[tblah]

I don't love that we have these member variables in OmpVisitor but this is a clear improvement to what was there before so LGTM. Thanks for the cleanup.