[Flang][OpenMP] Process motion clauses in a single call (NFC)

[skatrak]

This patch removes the template parameter of the
ClauseProcessor::processMotionClauses() method and instead processes both TO and FROM as part of a single call. This also enables moving the implementation out of the header and makes it simpler for a follow-up patch to potentially refactor processMap(), processMotionClauses(), processUseDeviceAddr() and processUseDevicePtr(), and minimize code duplication among these.

[llvmbot]

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

Author: Sergio Afonso (skatrak)

Changes

This patch removes the template parameter of the
ClauseProcessor::processMotionClauses() method and instead processes both TO and FROM as part of a single call. This also enables moving the implementation out of the header and makes it simpler for a follow-up patch to potentially refactor processMap(), processMotionClauses(), processUseDeviceAddr() and processUseDevicePtr(), and minimize code duplication among these.

---

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

3 Files Affected:

• (modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+31)
• (modified) flang/lib/Lower/OpenMP/ClauseProcessor.h (+2-35)
• (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+3-5)

diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 3f54234b176e3f..bb3a4c30944ddd 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -1114,6 +1114,37 @@ bool ClauseProcessor::processUseDevicePtr(
   return clauseFound;
 }
 
+bool ClauseProcessor::processMotionClauses(lower::StatementContext &stmtCtx,
+                                           mlir::omp::MapClauseOps &result) {
+  std::map<const semantics::Symbol *,
+           llvm::SmallVector<OmpMapMemberIndicesData>>
+      parentMemberIndices;
+  llvm::SmallVector<const semantics::Symbol *> mapSymbols;
+
+  auto callbackFn = [&](const auto &clause, const parser::CharBlock &source) {
+    mlir::Location clauseLocation = converter.genLocation(source);
+
+    // TODO Support motion modifiers: present, mapper, iterator.
+    constexpr llvm::omp::OpenMPOffloadMappingFlags mapTypeBits =
+        std::is_same_v<llvm::remove_cvref_t<decltype(clause)>, omp::clause::To>
+            ? llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO
+            : llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+
+    processMapObjects(stmtCtx, clauseLocation, std::get<ObjectList>(clause.t),
+                      mapTypeBits, parentMemberIndices, result.mapVars,
+                      &mapSymbols);
+  };
+
+  bool clauseFound = findRepeatableClause<omp::clause::To>(callbackFn);
+  clauseFound =
+      findRepeatableClause<omp::clause::From>(callbackFn) || clauseFound;
+
+  insertChildMapInfoIntoParent(converter, parentMemberIndices, result.mapVars,
+                               mapSymbols,
+                               /*mapSymTypes=*/nullptr, /*mapSymLocs=*/nullptr);
+  return clauseFound;
+}
+
 } // namespace omp
 } // namespace lower
 } // namespace Fortran
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.h b/flang/lib/Lower/OpenMP/ClauseProcessor.h
index f6b319c726a2d1..bc8e248e833539 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.h
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.h
@@ -121,6 +121,8 @@ class ClauseProcessor {
       llvm::SmallVectorImpl<const semantics::Symbol *> *mapSyms = nullptr,
       llvm::SmallVectorImpl<mlir::Location> *mapSymLocs = nullptr,
       llvm::SmallVectorImpl<mlir::Type> *mapSymTypes = nullptr) const;
+  bool processMotionClauses(lower::StatementContext &stmtCtx,
+                            mlir::omp::MapClauseOps &result);
   bool processReduction(
       mlir::Location currentLocation, mlir::omp::ReductionClauseOps &result,
       llvm::SmallVectorImpl<mlir::Type> *reductionTypes = nullptr,
@@ -140,9 +142,6 @@ class ClauseProcessor {
       llvm::SmallVectorImpl<mlir::Location> &useDeviceLocs,
       llvm::SmallVectorImpl<const semantics::Symbol *> &useDeviceSyms) const;
 
-  template <typename T>
-  bool processMotionClauses(lower::StatementContext &stmtCtx,
-                            mlir::omp::MapClauseOps &result);
   // Call this method for these clauses that should be supported but are not
   // implemented yet. It triggers a compilation error if any of the given
   // clauses is found.
@@ -190,38 +189,6 @@ class ClauseProcessor {
   List<Clause> clauses;
 };
 
-template <typename T>
-bool ClauseProcessor::processMotionClauses(lower::StatementContext &stmtCtx,
-                                           mlir::omp::MapClauseOps &result) {
-  std::map<const semantics::Symbol *,
-           llvm::SmallVector<OmpMapMemberIndicesData>>
-      parentMemberIndices;
-  llvm::SmallVector<const semantics::Symbol *> mapSymbols;
-
-  bool clauseFound = findRepeatableClause<T>(
-      [&](const T &clause, const parser::CharBlock &source) {
-        mlir::Location clauseLocation = converter.genLocation(source);
-
-        static_assert(std::is_same_v<T, omp::clause::To> ||
-                      std::is_same_v<T, omp::clause::From>);
-
-        // TODO Support motion modifiers: present, mapper, iterator.
-        constexpr llvm::omp::OpenMPOffloadMappingFlags mapTypeBits =
-            std::is_same_v<T, omp::clause::To>
-                ? llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO
-                : llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
-
-        processMapObjects(stmtCtx, clauseLocation,
-                          std::get<ObjectList>(clause.t), mapTypeBits,
-                          parentMemberIndices, result.mapVars, &mapSymbols);
-      });
-
-  insertChildMapInfoIntoParent(converter, parentMemberIndices, result.mapVars,
-                               mapSymbols,
-                               /*mapSymTypes=*/nullptr, /*mapSymLocs=*/nullptr);
-  return clauseFound;
-}
-
 template <typename... Ts>
 void ClauseProcessor::processTODO(mlir::Location currentLocation,
                                   llvm::omp::Directive directive) const {
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 233aacb40354d4..96280443182854 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -1223,12 +1223,10 @@ static void genTargetEnterExitUpdateDataClauses(
   cp.processDevice(stmtCtx, clauseOps);
   cp.processIf(directive, clauseOps);
 
-  if (directive == llvm::omp::Directive::OMPD_target_update) {
-    cp.processMotionClauses<clause::To>(stmtCtx, clauseOps);
-    cp.processMotionClauses<clause::From>(stmtCtx, clauseOps);
-  } else {
+  if (directive == llvm::omp::Directive::OMPD_target_update)
+    cp.processMotionClauses(stmtCtx, clauseOps);
+  else
     cp.processMap(loc, stmtCtx, clauseOps);
-  }
 
   cp.processNowait(clauseOps);
 }

[kparzysz]

LGTM

[kparzysz]

Maaaaybe using | would look nicer?

  bool clauseFound = findRepeatableClause<omp::clause::To>(callbackFn) |
                     findRepeatableClause<omp::clause::From>(callbackFn);

Just a thought...

[skatrak]

I really dislike how it looks using the logical or as well. Though I'm not sure what the LLVM project position is with respect to using bitwise operators to prevent short-circuit evaluation, since it may not be obvious to everyone what's being done there.

[tblah]

Thanks!