Skip to content

[flang][OpenMP] Move clause/object conversion to happen early, in genOMP #87074

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

Conversation

kparzysz
Copy link
Contributor

This removes the last use of genOmpObectList2, which has now been removed.

This removes the last use of genOmpObectList2, which has now been removed.
@kparzysz kparzysz requested review from ergawy and skatrak March 29, 2024 14:36
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Mar 29, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 29, 2024

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

Author: Krzysztof Parzyszek (kparzysz)

Changes

This removes the last use of genOmpObectList2, which has now been removed.


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

5 Files Affected:

  • (modified) flang/lib/Lower/OpenMP/ClauseProcessor.h (+2-3)
  • (modified) flang/lib/Lower/OpenMP/DataSharingProcessor.h (+2-3)
  • (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+194-215)
  • (modified) flang/lib/Lower/OpenMP/Utils.cpp (+11-19)
  • (modified) flang/lib/Lower/OpenMP/Utils.h (+2-4)
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.h b/flang/lib/Lower/OpenMP/ClauseProcessor.h
index db7a1b8335f818..f4d659b70cfee7 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.h
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.h
@@ -49,9 +49,8 @@ class ClauseProcessor {
 public:
   ClauseProcessor(Fortran::lower::AbstractConverter &converter,
                   Fortran::semantics::SemanticsContext &semaCtx,
-                  const Fortran::parser::OmpClauseList &clauses)
-      : converter(converter), semaCtx(semaCtx),
-        clauses(makeClauses(clauses, semaCtx)) {}
+                  const List<Clause> &clauses)
+      : converter(converter), semaCtx(semaCtx), clauses(clauses) {}
 
   // 'Unique' clauses: They can appear at most once in the clause list.
   bool processCollapse(
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index c11ee299c5d085..ef7b14327278e3 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -78,13 +78,12 @@ class DataSharingProcessor {
 public:
   DataSharingProcessor(Fortran::lower::AbstractConverter &converter,
                        Fortran::semantics::SemanticsContext &semaCtx,
-                       const Fortran::parser::OmpClauseList &opClauseList,
+                       const List<Clause> &clauses,
                        Fortran::lower::pft::Evaluation &eval,
                        bool useDelayedPrivatization = false,
                        Fortran::lower::SymMap *symTable = nullptr)
       : hasLastPrivateOp(false), converter(converter),
-        firOpBuilder(converter.getFirOpBuilder()),
-        clauses(omp::makeClauses(opClauseList, semaCtx)), eval(eval),
+        firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), eval(eval),
         useDelayedPrivatization(useDelayedPrivatization), symTable(symTable) {}
 
   // Privatisation is split into two steps.
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 692d81f9188be3..77d6c8503f0c34 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -17,6 +17,7 @@
 #include "DataSharingProcessor.h"
 #include "DirectivesCommon.h"
 #include "ReductionProcessor.h"
+#include "Utils.h"
 #include "flang/Common/idioms.h"
 #include "flang/Lower/Bridge.h"
 #include "flang/Lower/ConvertExpr.h"
@@ -310,14 +311,15 @@ static void getDeclareTargetInfo(
   } else if (const auto *clauseList{
                  Fortran::parser::Unwrap<Fortran::parser::OmpClauseList>(
                      spec.u)}) {
-    if (clauseList->v.empty()) {
+    List<Clause> clauses = makeClauses(*clauseList, semaCtx);
+    if (clauses.empty()) {
       // Case: declare target, implicit capture of function
       symbolAndClause.emplace_back(
           mlir::omp::DeclareTargetCaptureClause::to,
           eval.getOwningProcedure()->getSubprogramSymbol());
     }
 
-    ClauseProcessor cp(converter, semaCtx, *clauseList);
+    ClauseProcessor cp(converter, semaCtx, clauses);
     cp.processDeviceType(clauseOps);
     cp.processEnter(symbolAndClause);
     cp.processLink(symbolAndClause);
@@ -597,14 +599,11 @@ static void removeStoreOp(mlir::Operation *reductionOp, mlir::Value symVal) {
 // TODO: Generate the reduction operation during lowering instead of creating
 // and removing operations since this is not a robust approach. Also, removing
 // ops in the builder (instead of a rewriter) is probably not the best approach.
-static void
-genOpenMPReduction(Fortran::lower::AbstractConverter &converter,
-                   Fortran::semantics::SemanticsContext &semaCtx,
-                   const Fortran::parser::OmpClauseList &clauseList) {
+static void genOpenMPReduction(Fortran::lower::AbstractConverter &converter,
+                               Fortran::semantics::SemanticsContext &semaCtx,
+                               const List<Clause> &clauses) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
 
-  List<Clause> clauses{makeClauses(clauseList, semaCtx)};
-
   for (const Clause &clause : clauses) {
     if (const auto &reductionClause =
             std::get_if<clause::Reduction>(&clause.u)) {
@@ -737,7 +736,7 @@ struct OpWithBodyGenInfo {
     return *this;
   }
 
-  OpWithBodyGenInfo &setClauses(const Fortran::parser::OmpClauseList *value) {
+  OpWithBodyGenInfo &setClauses(const List<Clause> *value) {
     clauses = value;
     return *this;
   }
@@ -773,7 +772,7 @@ struct OpWithBodyGenInfo {
   /// [in] is this an outer operation - prevents privatization.
   bool outerCombined = false;
   /// [in] list of clauses to process.
-  const Fortran::parser::OmpClauseList *clauses = nullptr;
+  const List<Clause> *clauses = nullptr;
   /// [in] if provided, processes the construct's data-sharing attributes.
   DataSharingProcessor *dsp = nullptr;
   /// [in] if provided, list of reduction symbols
@@ -1151,36 +1150,33 @@ static OpTy genOpWithBody(OpWithBodyGenInfo &info, Args &&...args) {
 // Code generation functions for clauses
 //===----------------------------------------------------------------------===//
 
-static void genCriticalDeclareClauses(
-    Fortran::lower::AbstractConverter &converter,
-    Fortran::semantics::SemanticsContext &semaCtx,
-    const Fortran::parser::OmpClauseList &clauses, mlir::Location loc,
-    mlir::omp::CriticalClauseOps &clauseOps, llvm::StringRef name) {
+static void
+genCriticalDeclareClauses(Fortran::lower::AbstractConverter &converter,
+                          Fortran::semantics::SemanticsContext &semaCtx,
+                          const List<Clause> &clauses, mlir::Location loc,
+                          mlir::omp::CriticalClauseOps &clauseOps,
+                          llvm::StringRef name) {
   ClauseProcessor cp(converter, semaCtx, clauses);
   cp.processHint(clauseOps);
   clauseOps.nameAttr =
       mlir::StringAttr::get(converter.getFirOpBuilder().getContext(), name);
 }
 
-static void genFlushClauses(
-    Fortran::lower::AbstractConverter &converter,
-    Fortran::semantics::SemanticsContext &semaCtx,
-    const std::optional<Fortran::parser::OmpObjectList> &objects,
-    const std::optional<std::list<Fortran::parser::OmpMemoryOrderClause>>
-        &clauses,
-    mlir::Location loc, llvm::SmallVectorImpl<mlir::Value> &operandRange) {
-  if (objects)
-    genObjectList2(*objects, converter, operandRange);
-
-  if (clauses && clauses->size() > 0)
+static void genFlushClauses(Fortran::lower::AbstractConverter &converter,
+                            Fortran::semantics::SemanticsContext &semaCtx,
+                            const ObjectList &objects,
+                            const List<Clause> &clauses, mlir::Location loc,
+                            llvm::SmallVectorImpl<mlir::Value> &operandRange) {
+  genObjectList(objects, converter, operandRange);
+
+  if (clauses.size() > 0)
     TODO(converter.getCurrentLocation(), "Handle OmpMemoryOrderClause");
 }
 
 static void
 genOrderedRegionClauses(Fortran::lower::AbstractConverter &converter,
                         Fortran::semantics::SemanticsContext &semaCtx,
-                        const Fortran::parser::OmpClauseList &clauses,
-                        mlir::Location loc,
+                        const List<Clause> &clauses, mlir::Location loc,
                         mlir::omp::OrderedRegionClauseOps &clauseOps) {
   ClauseProcessor cp(converter, semaCtx, clauses);
   cp.processTODO<clause::Simd>(loc, llvm::omp::Directive::OMPD_ordered);
@@ -1189,9 +1185,9 @@ genOrderedRegionClauses(Fortran::lower::AbstractConverter &converter,
 static void genParallelClauses(
     Fortran::lower::AbstractConverter &converter,
     Fortran::semantics::SemanticsContext &semaCtx,
-    Fortran::lower::StatementContext &stmtCtx,
-    const Fortran::parser::OmpClauseList &clauses, mlir::Location loc,
-    bool processReduction, mlir::omp::ParallelClauseOps &clauseOps,
+    Fortran::lower::StatementContext &stmtCtx, const List<Clause> &clauses,
+    mlir::Location loc, bool processReduction,
+    mlir::omp::ParallelClauseOps &clauseOps,
     llvm::SmallVectorImpl<mlir::Type> &reductionTypes,
     llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> &reductionSyms) {
   ClauseProcessor cp(converter, semaCtx, clauses);
@@ -1211,8 +1207,7 @@ static void genParallelClauses(
 
 static void genSectionsClauses(Fortran::lower::AbstractConverter &converter,
                                Fortran::semantics::SemanticsContext &semaCtx,
-                               const Fortran::parser::OmpClauseList &clauses,
-                               mlir::Location loc,
+                               const List<Clause> &clauses, mlir::Location loc,
                                bool clausesFromBeginSections,
                                mlir::omp::SectionsClauseOps &clauseOps) {
   ClauseProcessor cp(converter, semaCtx, clauses);
@@ -1229,9 +1224,8 @@ static void genSimdLoopClauses(
     Fortran::lower::AbstractConverter &converter,
     Fortran::semantics::SemanticsContext &semaCtx,
     Fortran::lower::StatementContext &stmtCtx,
-    Fortran::lower::pft::Evaluation &eval,
-    const Fortran::parser::OmpClauseList &clauses, mlir::Location loc,
-    mlir::omp::SimdLoopClauseOps &clauseOps,
+    Fortran::lower::pft::Evaluation &eval, const List<Clause> &clauses,
+    mlir::Location loc, mlir::omp::SimdLoopClauseOps &clauseOps,
     llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> &iv) {
   ClauseProcessor cp(converter, semaCtx, clauses);
   cp.processCollapse(loc, eval, clauseOps, iv);
@@ -1249,9 +1243,8 @@ static void genSimdLoopClauses(
 
 static void genSingleClauses(Fortran::lower::AbstractConverter &converter,
                              Fortran::semantics::SemanticsContext &semaCtx,
-                             const Fortran::parser::OmpClauseList &beginClauses,
-                             const Fortran::parser::OmpClauseList &endClauses,
-                             mlir::Location loc,
+                             const List<Clause> &beginClauses,
+                             const List<Clause> &endClauses, mlir::Location loc,
                              mlir::omp::SingleClauseOps &clauseOps) {
   ClauseProcessor bcp(converter, semaCtx, beginClauses);
   bcp.processAllocate(clauseOps);
@@ -1265,9 +1258,8 @@ static void genSingleClauses(Fortran::lower::AbstractConverter &converter,
 static void genTargetClauses(
     Fortran::lower::AbstractConverter &converter,
     Fortran::semantics::SemanticsContext &semaCtx,
-    Fortran::lower::StatementContext &stmtCtx,
-    const Fortran::parser::OmpClauseList &clauses, mlir::Location loc,
-    bool processHostOnlyClauses, bool processReduction,
+    Fortran::lower::StatementContext &stmtCtx, const List<Clause> &clauses,
+    mlir::Location loc, bool processHostOnlyClauses, bool processReduction,
     mlir::omp::TargetClauseOps &clauseOps,
     llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> &mapSyms,
     llvm::SmallVectorImpl<mlir::Location> &mapSymLocs,
@@ -1293,9 +1285,8 @@ static void genTargetClauses(
 static void genTargetDataClauses(
     Fortran::lower::AbstractConverter &converter,
     Fortran::semantics::SemanticsContext &semaCtx,
-    Fortran::lower::StatementContext &stmtCtx,
-    const Fortran::parser::OmpClauseList &clauses, mlir::Location loc,
-    mlir::omp::TargetDataClauseOps &clauseOps,
+    Fortran::lower::StatementContext &stmtCtx, const List<Clause> &clauses,
+    mlir::Location loc, mlir::omp::TargetDataClauseOps &clauseOps,
     llvm::SmallVectorImpl<mlir::Type> &useDeviceTypes,
     llvm::SmallVectorImpl<mlir::Location> &useDeviceLocs,
     llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> &useDeviceSyms) {
@@ -1326,9 +1317,8 @@ static void genTargetDataClauses(
 static void genTargetEnterExitUpdateDataClauses(
     Fortran::lower::AbstractConverter &converter,
     Fortran::semantics::SemanticsContext &semaCtx,
-    Fortran::lower::StatementContext &stmtCtx,
-    const Fortran::parser::OmpClauseList &clauses, mlir::Location loc,
-    llvm::omp::Directive directive,
+    Fortran::lower::StatementContext &stmtCtx, const List<Clause> &clauses,
+    mlir::Location loc, llvm::omp::Directive directive,
     mlir::omp::TargetEnterExitUpdateDataClauseOps &clauseOps) {
   ClauseProcessor cp(converter, semaCtx, clauses);
   cp.processDepend(clauseOps);
@@ -1347,8 +1337,7 @@ static void genTargetEnterExitUpdateDataClauses(
 static void genTaskClauses(Fortran::lower::AbstractConverter &converter,
                            Fortran::semantics::SemanticsContext &semaCtx,
                            Fortran::lower::StatementContext &stmtCtx,
-                           const Fortran::parser::OmpClauseList &clauses,
-                           mlir::Location loc,
+                           const List<Clause> &clauses, mlir::Location loc,
                            mlir::omp::TaskClauseOps &clauseOps) {
   ClauseProcessor cp(converter, semaCtx, clauses);
   cp.processAllocate(clauseOps);
@@ -1367,8 +1356,7 @@ static void genTaskClauses(Fortran::lower::AbstractConverter &converter,
 
 static void genTaskgroupClauses(Fortran::lower::AbstractConverter &converter,
                                 Fortran::semantics::SemanticsContext &semaCtx,
-                                const Fortran::parser::OmpClauseList &clauses,
-                                mlir::Location loc,
+                                const List<Clause> &clauses, mlir::Location loc,
                                 mlir::omp::TaskgroupClauseOps &clauseOps) {
   ClauseProcessor cp(converter, semaCtx, clauses);
   cp.processAllocate(clauseOps);
@@ -1378,8 +1366,7 @@ static void genTaskgroupClauses(Fortran::lower::AbstractConverter &converter,
 
 static void genTaskwaitClauses(Fortran::lower::AbstractConverter &converter,
                                Fortran::semantics::SemanticsContext &semaCtx,
-                               const Fortran::parser::OmpClauseList &clauses,
-                               mlir::Location loc,
+                               const List<Clause> &clauses, mlir::Location loc,
                                mlir::omp::TaskwaitClauseOps &clauseOps) {
   ClauseProcessor cp(converter, semaCtx, clauses);
   cp.processTODO<clause::Depend, clause::Nowait>(
@@ -1389,8 +1376,7 @@ static void genTaskwaitClauses(Fortran::lower::AbstractConverter &converter,
 static void genTeamsClauses(Fortran::lower::AbstractConverter &converter,
                             Fortran::semantics::SemanticsContext &semaCtx,
                             Fortran::lower::StatementContext &stmtCtx,
-                            const Fortran::parser::OmpClauseList &clauses,
-                            mlir::Location loc,
+                            const List<Clause> &clauses, mlir::Location loc,
                             mlir::omp::TeamsClauseOps &clauseOps) {
   ClauseProcessor cp(converter, semaCtx, clauses);
   cp.processAllocate(clauseOps);
@@ -1407,9 +1393,8 @@ static void genWsloopClauses(
     Fortran::lower::AbstractConverter &converter,
     Fortran::semantics::SemanticsContext &semaCtx,
     Fortran::lower::StatementContext &stmtCtx,
-    Fortran::lower::pft::Evaluation &eval,
-    const Fortran::parser::OmpClauseList &beginClauses,
-    const Fortran::parser::OmpClauseList *endClauses, mlir::Location loc,
+    Fortran::lower::pft::Evaluation &eval, const List<Clause> &beginClauses,
+    const List<Clause> *endClauses, mlir::Location loc,
     mlir::omp::WsloopClauseOps &clauseOps,
     llvm::SmallVectorImpl<const Fortran::semantics::Symbol *> &iv,
     llvm::SmallVectorImpl<mlir::Type> &reductionTypes,
@@ -1450,8 +1435,7 @@ static mlir::omp::CriticalOp
 genCriticalOp(Fortran::lower::AbstractConverter &converter,
               Fortran::semantics::SemanticsContext &semaCtx,
               Fortran::lower::pft::Evaluation &eval, bool genNested,
-              mlir::Location loc,
-              const Fortran::parser::OmpClauseList &clauseList,
+              mlir::Location loc, const List<Clause> &clauses,
               const std::optional<Fortran::parser::Name> &name) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
   mlir::FlatSymbolRefAttr nameAttr;
@@ -1462,7 +1446,7 @@ genCriticalOp(Fortran::lower::AbstractConverter &converter,
     auto global = mod.lookupSymbol<mlir::omp::CriticalDeclareOp>(nameStr);
     if (!global) {
       mlir::omp::CriticalClauseOps clauseOps;
-      genCriticalDeclareClauses(converter, semaCtx, clauseList, loc, clauseOps,
+      genCriticalDeclareClauses(converter, semaCtx, clauses, loc, clauseOps,
                                 nameStr);
 
       mlir::OpBuilder modBuilder(mod.getBodyRegion());
@@ -1481,8 +1465,7 @@ static mlir::omp::DistributeOp
 genDistributeOp(Fortran::lower::AbstractConverter &converter,
                 Fortran::semantics::SemanticsContext &semaCtx,
                 Fortran::lower::pft::Evaluation &eval, bool genNested,
-                mlir::Location loc,
-                const Fortran::parser::OmpClauseList &clauseList) {
+                mlir::Location loc, const List<Clause> &clauses) {
   TODO(loc, "Distribute construct");
   return nullptr;
 }
@@ -1491,12 +1474,9 @@ static mlir::omp::FlushOp
 genFlushOp(Fortran::lower::AbstractConverter &converter,
            Fortran::semantics::SemanticsContext &semaCtx,
            Fortran::lower::pft::Evaluation &eval, mlir::Location loc,
-           const std::optional<Fortran::parser::OmpObjectList> &objectList,
-           const std::optional<std::list<Fortran::parser::OmpMemoryOrderClause>>
-               &clauseList) {
+           const ObjectList &objects, const List<Clause> &clauses) {
   llvm::SmallVector<mlir::Value> operandRange;
-  genFlushClauses(converter, semaCtx, objectList, clauseList, loc,
-                  operandRange);
+  genFlushClauses(converter, semaCtx, objects, clauses, loc, operandRange);
 
   return converter.getFirOpBuilder().create<mlir::omp::FlushOp>(
       converter.getCurrentLocation(), operandRange);
@@ -1516,7 +1496,7 @@ static mlir::omp::OrderedOp
 genOrderedOp(Fortran::lower::AbstractConverter &converter,
              Fortran::semantics::SemanticsContext &semaCtx,
              Fortran::lower::pft::Evaluation &eval, mlir::Location loc,
-             const Fortran::parser::OmpClauseList &clauseList) {
+             const List<Clause> &clauses) {
   TODO(loc, "OMPD_ordered");
   return nullptr;
 }
@@ -1525,10 +1505,9 @@ static mlir::omp::OrderedRegionOp
 genOrderedRegionOp(Fortran::lower::AbstractConverter &converter,
                    Fortran::semantics::SemanticsContext &semaCtx,
                    Fortran::lower::pft::Evaluation &eval, bool genNested,
-                   mlir::Location loc,
-                   const Fortran::parser::OmpClauseList &clauseList) {
+                   mlir::Location loc, const List<Clause> &clauses) {
   mlir::omp::OrderedRegionClauseOps clauseOps;
-  genOrderedRegionClauses(converter, semaCtx, clauseList, loc, clauseOps);
+  genOrderedRegionClauses(converter, semaCtx, clauses, loc, clauseOps);
 
   return genOpWithBody<mlir::omp::OrderedRegionOp>(
       OpWithBodyGenInfo(converter, semaCtx, loc, eval).setGenNested(genNested),
@@ -1540,8 +1519,7 @@ genParallelOp(Fortran::lower::AbstractConverter &converter,
               Fortran::lower::SymMap &symTable,
               Fortran::semantics::SemanticsContext &semaCtx,
               Fortran::lower::pft::Evaluation &eval, bool genNested,
-              mlir::Location loc,
-              const Fortran::parser::OmpClauseList &clauseList,
+              mlir::Location loc, const List<Clause> &clauses,
               bool outerCombined = false) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
   Fortran::lower::StatementContext stmtCtx;
@@ -1549,7 +1527,7 @@ genParallelOp(Fortran::lower::AbstractConverter &converter,
   llvm::SmallVector<const Fortran::semantics::Symbol *> privateSyms;
   llvm::SmallVector<mlir::Type> reductionTypes;
   llvm::SmallVector<const Fortran::semantics::Symbol *> reductionSyms;
-  genParallelClauses(converter, semaCtx, stmtCtx, clauseList, loc,
+  genParallelClauses(converter, semaCtx, stmtCtx, clauses, loc,
                      /*processReduction=*/!outerCombined, clauseOps,
                      reductionTypes, reductionSyms);
 
@@ -1562,7 +1540,7 @@ genParallelOp(Fortran::lower::Ab...
[truncated]

@kparzysz
Copy link
Contributor Author

Replaced by #87086

@kparzysz kparzysz closed this Mar 29, 2024
@kparzysz kparzysz deleted the users/kparzysz/spr/a04-makeclause branch March 29, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants