[Flang][OpenMP][Lower] Clause lowering cleanup

[skatrak]

This patch removes the ClauseProcessor::processDefault method due to it having been implemented in DataSharingProcessor::collectDefaultSymbols instead.

Also, some genXyzClauses functions are updated to avoid triggering TODO errors for clauses not supported by the corresponding construct and to keep alphabetical sorting on the order in which clauses are processed.

[llvmbot]

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

Author: Sergio Afonso (skatrak)

Changes

This patch removes the ClauseProcessor::processDefault method due to it having been implemented in DataSharingProcessor::collectDefaultSymbols instead.

Also, some genXyzClauses functions are updated to avoid triggering TODO errors for clauses not supported by the corresponding construct and to keep alphabetical sorting on the order in which clauses are processed.

---

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

3 Files Affected:

• (modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (-22)
• (modified) flang/lib/Lower/OpenMP/ClauseProcessor.h (-1)
• (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+10-11)

diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index c30844f40b7e0..da6c21730dfba 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -256,28 +256,6 @@ bool ClauseProcessor::processCollapse(
   return found;
 }
 
-bool ClauseProcessor::processDefault() const {
-  if (auto *clause = findUniqueClause<omp::clause::Default>()) {
-    // Private, Firstprivate, Shared, None
-    switch (clause->v) {
-    case omp::clause::Default::DataSharingAttribute::Shared:
-    case omp::clause::Default::DataSharingAttribute::None:
-      // Default clause with shared or none do not require any handling since
-      // Shared is the default behavior in the IR and None is only required
-      // for semantic checks.
-      break;
-    case omp::clause::Default::DataSharingAttribute::Private:
-      // TODO Support default(private)
-      break;
-    case omp::clause::Default::DataSharingAttribute::Firstprivate:
-      // TODO Support default(firstprivate)
-      break;
-    }
-    return true;
-  }
-  return false;
-}
-
 bool ClauseProcessor::processDevice(lower::StatementContext &stmtCtx,
                                     mlir::omp::DeviceClauseOps &result) const {
   const parser::CharBlock *source = nullptr;
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.h b/flang/lib/Lower/OpenMP/ClauseProcessor.h
index 2c4b3857fda9f..ea4db3e6db0cc 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.h
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.h
@@ -57,7 +57,6 @@ class ClauseProcessor {
   processCollapse(mlir::Location currentLocation, lower::pft::Evaluation &eval,
                   mlir::omp::LoopRelatedOps &result,
                   llvm::SmallVectorImpl<const semantics::Symbol *> &iv) const;
-  bool processDefault() const;
   bool processDevice(lower::StatementContext &stmtCtx,
                      mlir::omp::DeviceClauseOps &result) const;
   bool processDeviceType(mlir::omp::DeviceTypeClauseOps &result) const;
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 26825468df9b1..d900f7fbf7309 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -1055,8 +1055,9 @@ static void genFlushClauses(lower::AbstractConverter &converter,
   if (!objects.empty())
     genObjectList(objects, converter, operandRange);
 
-  if (!clauses.empty())
-    TODO(converter.getCurrentLocation(), "Handle OmpMemoryOrderClause");
+  ClauseProcessor cp(converter, semaCtx, clauses);
+  cp.processTODO<clause::AcqRel, clause::Acquire, clause::Release,
+                 clause::SeqCst>(loc, llvm::omp::OMPD_flush);
 }
 
 static void
@@ -1096,7 +1097,6 @@ static void genParallelClauses(
     llvm::SmallVectorImpl<const semantics::Symbol *> &reductionSyms) {
   ClauseProcessor cp(converter, semaCtx, clauses);
   cp.processAllocate(clauseOps);
-  cp.processDefault();
   cp.processIf(llvm::omp::Directive::OMPD_parallel, clauseOps);
   cp.processNumThreads(stmtCtx, clauseOps);
   cp.processProcBind(clauseOps);
@@ -1129,7 +1129,7 @@ static void genSimdClauses(lower::AbstractConverter &converter,
   cp.processSimdlen(clauseOps);
 
   // TODO Support delayed privatization.
-  cp.processTODO<clause::Allocate, clause::Linear, clause::Nontemporal>(
+  cp.processTODO<clause::Linear, clause::Nontemporal>(
       loc, llvm::omp::Directive::OMPD_simd);
 }
 
@@ -1167,15 +1167,15 @@ static void genTargetClauses(
   cp.processIsDevicePtr(clauseOps, devicePtrTypes, devicePtrLocs,
                         devicePtrSyms);
   cp.processMap(loc, stmtCtx, clauseOps, &mapSyms, &mapLocs, &mapTypes);
-  cp.processThreadLimit(stmtCtx, clauseOps);
 
   if (processHostOnlyClauses)
     cp.processNowait(clauseOps);
 
+  cp.processThreadLimit(stmtCtx, clauseOps);
+
   cp.processTODO<clause::Allocate, clause::Defaultmap, clause::Firstprivate,
-                 clause::InReduction, clause::Reduction,
-                 clause::UsesAllocators>(loc,
-                                         llvm::omp::Directive::OMPD_target);
+                 clause::InReduction, clause::UsesAllocators>(
+      loc, llvm::omp::Directive::OMPD_target);
 
   // `target private(..)` is only supported in delayed privatization mode.
   if (!enableDelayedPrivatizationStaging)
@@ -1223,7 +1223,6 @@ static void genTargetEnterExitUpdateDataClauses(
   cp.processDepend(clauseOps);
   cp.processDevice(stmtCtx, clauseOps);
   cp.processIf(directive, clauseOps);
-  cp.processNowait(clauseOps);
 
   if (directive == llvm::omp::Directive::OMPD_target_update) {
     cp.processMotionClauses<clause::To>(stmtCtx, clauseOps);
@@ -1231,6 +1230,8 @@ static void genTargetEnterExitUpdateDataClauses(
   } else {
     cp.processMap(loc, stmtCtx, clauseOps);
   }
+  
+  cp.processNowait(clauseOps);
 }
 
 static void genTaskClauses(lower::AbstractConverter &converter,
@@ -1240,7 +1241,6 @@ static void genTaskClauses(lower::AbstractConverter &converter,
                            mlir::omp::TaskOperands &clauseOps) {
   ClauseProcessor cp(converter, semaCtx, clauses);
   cp.processAllocate(clauseOps);
-  cp.processDefault();
   cp.processDepend(clauseOps);
   cp.processFinal(stmtCtx, clauseOps);
   cp.processIf(llvm::omp::Directive::OMPD_task, clauseOps);
@@ -1279,7 +1279,6 @@ static void genTeamsClauses(lower::AbstractConverter &converter,
                             mlir::omp::TeamsOperands &clauseOps) {
   ClauseProcessor cp(converter, semaCtx, clauses);
   cp.processAllocate(clauseOps);
-  cp.processDefault();
   cp.processIf(llvm::omp::Directive::OMPD_teams, clauseOps);
   cp.processNumTeams(stmtCtx, clauseOps);
   cp.processThreadLimit(stmtCtx, clauseOps);

[github-actions]

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

[Meinersbur]

Also, some genXyzClauses functions are updated to avoid triggering TODO errors for clauses not supported by the corresponding construct

This makes this patch non-NFC. I assume this is for combined/composite constructs (supported -> applicable).

[skatrak]

Thank you @Meinersbur for the review!

This makes this patch non-NFC. I assume this is for combined/composite constructs (supported -> applicable).

Not particularly, that's referring to the allocate clause when present in the list of clauses for simd and the reduction clause for target. These constructs don't accept these clauses, so they shouldn't trigger a TODO error. At the moment we just ignore clauses that don't apply to the construct we're lowering, so removing them from the list of TODOs would give the expected behavior.

I think that, once #102613 is merged, we could think about actually adding asserts to lowering when an inapplicable clause is attached to a construct (one that the construct can't receive in any case).

I'll update the PR title, since there's indeed some minor behavior change as a result.

[mjklemm]

LGTM