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] Convert processTODO and remove unused objects #81627

Merged
merged 7 commits into from Mar 21, 2024

Conversation

kparzysz
Copy link
Contributor

Remove ClauseIterator2 and clauses2 from ClauseProcessor.

[Clause representation 5/6]

@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp labels Feb 13, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 13, 2024

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

@llvm/pr-subscribers-flang-openmp

Author: Krzysztof Parzyszek (kparzysz)

Changes

Remove ClauseIterator2 and clauses2 from ClauseProcessor.

[Clause representation 5/6]


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

1 Files Affected:

  • (modified) flang/lib/Lower/OpenMP.cpp (+28-47)
diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index 7cf8fcc0a3d27..8dcd0708e6245 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -1668,13 +1668,11 @@ void DataSharingProcessor::defaultPrivatize() {
 /// methods that relate to clauses that can impact the lowering of that
 /// construct.
 class ClauseProcessor {
-  using ClauseTy = Fortran::parser::OmpClause;
-
 public:
   ClauseProcessor(Fortran::lower::AbstractConverter &converter,
                   Fortran::semantics::SemanticsContext &semaCtx,
                   const Fortran::parser::OmpClauseList &clauses)
-      : converter(converter), semaCtx(semaCtx), clauses2(clauses),
+      : converter(converter), semaCtx(semaCtx),
         clauses(omp::makeList(clauses, semaCtx)) {}
 
   // 'Unique' clauses: They can appear at most once in the clause list.
@@ -1775,7 +1773,6 @@ class ClauseProcessor {
 
 private:
   using ClauseIterator = omp::List<omp::Clause>::const_iterator;
-  using ClauseIterator2 = std::list<ClauseTy>::const_iterator;
 
   /// Utility to find a clause within a range in the clause list.
   template <typename T>
@@ -1835,7 +1832,6 @@ class ClauseProcessor {
 
   Fortran::lower::AbstractConverter &converter;
   Fortran::semantics::SemanticsContext &semaCtx;
-  const Fortran::parser::OmpClauseList &clauses2;
   omp::List<omp::Clause> clauses;
 };
 
@@ -3131,19 +3127,17 @@ bool ClauseProcessor::processMotionClauses(
 template <typename... Ts>
 void ClauseProcessor::processTODO(mlir::Location currentLocation,
                                   llvm::omp::Directive directive) const {
-  auto checkUnhandledClause = [&](const auto *x) {
+  auto checkUnhandledClause = [&](llvm::omp::Clause id, const auto *x) {
     if (!x)
       return;
     TODO(currentLocation,
-         "Unhandled clause " +
-             llvm::StringRef(Fortran::parser::ParseTreeDumper::GetNodeName(*x))
-                 .upper() +
+         "Unhandled clause " + llvm::omp::getOpenMPClauseName(id).upper() +
              " in " + llvm::omp::getOpenMPDirectiveName(directive).upper() +
              " construct");
   };
 
-  for (ClauseIterator2 it = clauses2.v.begin(); it != clauses2.v.end(); ++it)
-    (checkUnhandledClause(std::get_if<Ts>(&it->u)), ...);
+  for (ClauseIterator it = clauses.begin(); it != clauses.end(); ++it)
+    (checkUnhandledClause(it->id, std::get_if<Ts>(&it->u)), ...);
 }
 
 //===----------------------------------------------------------------------===//
@@ -3722,8 +3716,8 @@ genSingleOp(Fortran::lower::AbstractConverter &converter,
 
   ClauseProcessor cp(converter, semaCtx, beginClauseList);
   cp.processAllocate(allocatorOperands, allocateOperands);
-  cp.processTODO<Fortran::parser::OmpClause::Copyprivate>(
-      currentLocation, llvm::omp::Directive::OMPD_single);
+  cp.processTODO<omp::clause::Copyprivate>(currentLocation,
+                                           llvm::omp::Directive::OMPD_single);
 
   ClauseProcessor(converter, semaCtx, endClauseList).processNowait(nowaitAttr);
 
@@ -3756,10 +3750,9 @@ genTaskOp(Fortran::lower::AbstractConverter &converter,
   cp.processMergeable(mergeableAttr);
   cp.processPriority(stmtCtx, priorityClauseOperand);
   cp.processDepend(dependTypeOperands, dependOperands);
-  cp.processTODO<Fortran::parser::OmpClause::InReduction,
-                 Fortran::parser::OmpClause::Detach,
-                 Fortran::parser::OmpClause::Affinity>(
-      currentLocation, llvm::omp::Directive::OMPD_task);
+  cp.processTODO<omp::clause::InReduction, omp::clause::Detach,
+                 omp::clause::Affinity>(currentLocation,
+                                        llvm::omp::Directive::OMPD_task);
 
   return genOpWithBody<mlir::omp::TaskOp>(
       OpWithBodyGenInfo(converter, semaCtx, currentLocation, eval)
@@ -3784,7 +3777,7 @@ genTaskGroupOp(Fortran::lower::AbstractConverter &converter,
   llvm::SmallVector<mlir::Value> allocateOperands, allocatorOperands;
   ClauseProcessor cp(converter, semaCtx, clauseList);
   cp.processAllocate(allocatorOperands, allocateOperands);
-  cp.processTODO<Fortran::parser::OmpClause::TaskReduction>(
+  cp.processTODO<omp::clause::TaskReduction>(
       currentLocation, llvm::omp::Directive::OMPD_taskgroup);
   return genOpWithBody<mlir::omp::TaskGroupOp>(
       OpWithBodyGenInfo(converter, semaCtx, currentLocation, eval)
@@ -3868,8 +3861,7 @@ genEnterExitUpdateDataOp(Fortran::lower::AbstractConverter &converter,
     cp.processMap(currentLocation, directive, stmtCtx, mapOperands);
   }
 
-  cp.processTODO<Fortran::parser::OmpClause::Depend>(currentLocation,
-                                                     directive);
+  cp.processTODO<omp::clause::Depend>(currentLocation, directive);
 
   return firOpBuilder.create<OpTy>(currentLocation, ifClauseOperand,
                                    deviceOperand, nullptr, mlir::ValueRange(),
@@ -4052,16 +4044,11 @@ genTargetOp(Fortran::lower::AbstractConverter &converter,
   cp.processNowait(nowaitAttr);
   cp.processMap(currentLocation, directive, stmtCtx, mapOperands, &mapSymTypes,
                 &mapSymLocs, &mapSymbols);
-  cp.processTODO<Fortran::parser::OmpClause::Private,
-                 Fortran::parser::OmpClause::Depend,
-                 Fortran::parser::OmpClause::Firstprivate,
-                 Fortran::parser::OmpClause::IsDevicePtr,
-                 Fortran::parser::OmpClause::HasDeviceAddr,
-                 Fortran::parser::OmpClause::Reduction,
-                 Fortran::parser::OmpClause::InReduction,
-                 Fortran::parser::OmpClause::Allocate,
-                 Fortran::parser::OmpClause::UsesAllocators,
-                 Fortran::parser::OmpClause::Defaultmap>(
+  cp.processTODO<omp::clause::Private, omp::clause::Depend,
+                 omp::clause::Firstprivate, omp::clause::IsDevicePtr,
+                 omp::clause::HasDeviceAddr, omp::clause::Reduction,
+                 omp::clause::InReduction, omp::clause::Allocate,
+                 omp::clause::UsesAllocators, omp::clause::Defaultmap>(
       currentLocation, llvm::omp::Directive::OMPD_target);
 
   // 5.8.1 Implicit Data-Mapping Attribute Rules
@@ -4164,8 +4151,8 @@ genTeamsOp(Fortran::lower::AbstractConverter &converter,
   cp.processDefault();
   cp.processNumTeams(stmtCtx, numTeamsClauseOperand);
   cp.processThreadLimit(stmtCtx, threadLimitClauseOperand);
-  cp.processTODO<Fortran::parser::OmpClause::Reduction>(
-      currentLocation, llvm::omp::Directive::OMPD_teams);
+  cp.processTODO<omp::clause::Reduction>(currentLocation,
+                                         llvm::omp::Directive::OMPD_teams);
 
   return genOpWithBody<mlir::omp::TeamsOp>(
       OpWithBodyGenInfo(converter, semaCtx, currentLocation, eval)
@@ -4217,7 +4204,7 @@ static mlir::omp::DeclareTargetDeviceType getDeclareTargetInfo(
     cp.processEnter(symbolAndClause);
     cp.processLink(symbolAndClause);
     cp.processDeviceType(deviceType);
-    cp.processTODO<Fortran::parser::OmpClause::Indirect>(
+    cp.processTODO<omp::clause::Indirect>(
         converter.getCurrentLocation(),
         llvm::omp::Directive::OMPD_declare_target);
   }
@@ -4276,8 +4263,7 @@ genOmpSimpleStandalone(Fortran::lower::AbstractConverter &converter,
     break;
   case llvm::omp::Directive::OMPD_taskwait:
     ClauseProcessor(converter, semaCtx, opClauseList)
-        .processTODO<Fortran::parser::OmpClause::Depend,
-                     Fortran::parser::OmpClause::Nowait>(
+        .processTODO<omp::clause::Depend, omp::clause::Nowait>(
             currentLocation, llvm::omp::Directive::OMPD_taskwait);
     firOpBuilder.create<mlir::omp::TaskwaitOp>(currentLocation);
     break;
@@ -4427,11 +4413,9 @@ createSimdLoop(Fortran::lower::AbstractConverter &converter,
   cp.processIf(omp::clause::If::DirectiveNameModifier::Simd, ifClauseOperand);
   cp.processSimdlen(simdlenClauseOperand);
   cp.processSafelen(safelenClauseOperand);
-  cp.processTODO<Fortran::parser::OmpClause::Aligned,
-                 Fortran::parser::OmpClause::Allocate,
-                 Fortran::parser::OmpClause::Linear,
-                 Fortran::parser::OmpClause::Nontemporal,
-                 Fortran::parser::OmpClause::Order>(loc, ompDirective);
+  cp.processTODO<omp::clause::Aligned, omp::clause::Allocate,
+                 omp::clause::Linear, omp::clause::Nontemporal,
+                 omp::clause::Order>(loc, ompDirective);
 
   convertLoopBounds(converter, loc, lowerBound, upperBound, step,
                     loopVarTypeSize);
@@ -4486,8 +4470,7 @@ static void createWsLoop(Fortran::lower::AbstractConverter &converter,
                      loopVarTypeSize);
   cp.processScheduleChunk(stmtCtx, scheduleChunkClauseOperand);
   cp.processReduction(loc, reductionVars, reductionDeclSymbols);
-  cp.processTODO<Fortran::parser::OmpClause::Linear,
-                 Fortran::parser::OmpClause::Order>(loc, ompDirective);
+  cp.processTODO<omp::clause::Linear, omp::clause::Order>(loc, ompDirective);
 
   convertLoopBounds(converter, loc, lowerBound, upperBound, step,
                     loopVarTypeSize);
@@ -4547,11 +4530,9 @@ static void createSimdWsLoop(
     const Fortran::parser::OmpClauseList &beginClauseList,
     const Fortran::parser::OmpClauseList *endClauseList, mlir::Location loc) {
   ClauseProcessor cp(converter, semaCtx, beginClauseList);
-  cp.processTODO<
-      Fortran::parser::OmpClause::Aligned, Fortran::parser::OmpClause::Allocate,
-      Fortran::parser::OmpClause::Linear, Fortran::parser::OmpClause::Safelen,
-      Fortran::parser::OmpClause::Simdlen, Fortran::parser::OmpClause::Order>(
-      loc, ompDirective);
+  cp.processTODO<omp::clause::Aligned, omp::clause::Allocate,
+                 omp::clause::Linear, omp::clause::Safelen,
+                 omp::clause::Simdlen, omp::clause::Order>(loc, ompDirective);
   // TODO: Add support for vectorization - add vectorization hints inside loop
   // body.
   // OpenMP standard does not specify the length of vector instructions.

Copy link
Contributor

@skatrak skatrak left a comment

Choose a reason for hiding this comment

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

LGTM.

Introduce a set of generic classes (templates) that represent OpenMP
clauses in a language-agnostic manner. OpenMP clauses can contain
expressions and data objects and the exact representation of each
depends on the source language of the compiled program. To deal with
this, the templates depend on two type parameters:
- IdType: type that represent object's identity (in a way that
  satisfied OpenMP requirements), and
- ExprType: type that can represent numeric values, as well as
  data references (e.g. x.y[1].z[2]).

In addition to that, implement code instantiating these templates
from flang's AST.

This patch only introduces the new classes, they are not yet used
anywhere.
Temporarily rename old clause list to `clauses2`, old clause iterator
to `ClauseIterator2`.
Change `findUniqueClause` to iterate over `omp::Clause` objects,
modify all handlers to operate on 'omp::clause::xyz` equivalents.
…essor

Rename `findRepeatableClause` to `findRepeatableClause2`, and make the
new `findRepeatableClause` operate on new `omp::Clause` objects.

Leave `Map` unchanged, because it will require more changes for it to
work.
The related functions are `gatherDataOperandAddrAndBounds` and
`genBoundsOps`. The former is used in OpenACC as well, and it was
updated to pass evaluate::Expr instead of parser objects.

The difference in the test case comes from unfolded conversions
of index expressions, which are explicitly of type integer(kind=8).

Delete now unused `findRepeatableClause2` and `findClause2`.

Add `AsGenericExpr` that takes std::optional. It already returns optional
Expr. Making it accept an optional Expr as input would reduce the number
of necessary checks when handling frequent optional values in evaluator.
Remove `ClauseIterator2` and `clauses2` from ClauseProcessor.
Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Base automatically changed from users/kparzysz/spr/b04-rep2 to main March 20, 2024 20:00
Copy link

github-actions bot commented Mar 21, 2024

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

@kparzysz kparzysz merged commit 2ab106c into main Mar 21, 2024
4 checks passed
@kparzysz kparzysz deleted the users/kparzysz/spr/b05-todo branch March 21, 2024 20:12
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
…1627)

Remove `ClauseIterator2` and `clauses2` from ClauseProcessor.

[Clause representation 5/6]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang:openmp flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants