[flang][OpenMP] Restore reduction processor behavior broken by #145837

[ergawy]

Fixes #149089 and #149700.

Before #145837, when processing a reduction symbol not yet supported by OpenMP lowering, the reduction processor would simply skip filling in the reduction symbols and variables. With #145837, this behvaior was slightly changed because the reduction symbols are populated before invoking the reduction processor (this is more convenient to shared the code with do concurrent).

This PR restores the previous behavior.

[llvmbot]

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

@llvm/pr-subscribers-flang-openmp

Author: Kareem Ergawy (ergawy)

Changes

Fixes #149089.

Before #145837, when processing a reduction symbol not yet supported by OpenMP lowering, the reduction processor would simply skip filling in the reduction symbols and variables. With #145837, this behvaior was slightly changed because the reduction symbols are populated before invoking the reduction processor (this is more convenient to shared the code with do concurrent).

This PR restores the previous behavior.

---

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

5 Files Affected:

• (modified) flang/include/flang/Lower/Support/ReductionProcessor.h (+1-1)
• (modified) flang/lib/Lower/Bridge.cpp (+2-1)
• (modified) flang/lib/Lower/OpenMP/ClauseProcessor.cpp (+18-14)
• (modified) flang/lib/Lower/Support/ReductionProcessor.cpp (+7-5)
• (added) flang/test/Lower/OpenMP/wsloop-reduction-non-intrinsic.f90 (+25)

diff --git a/flang/include/flang/Lower/Support/ReductionProcessor.h b/flang/include/flang/Lower/Support/ReductionProcessor.h
index 72d8a0096f511..0b49049d43ed2 100644
--- a/flang/include/flang/Lower/Support/ReductionProcessor.h
+++ b/flang/include/flang/Lower/Support/ReductionProcessor.h
@@ -124,7 +124,7 @@ class ReductionProcessor {
   /// Creates a reduction declaration and associates it with an OpenMP block
   /// directive.
   template <typename OpType, typename RedOperatorListTy>
-  static void processReductionArguments(
+  static bool processReductionArguments(
       mlir::Location currentLocation, lower::AbstractConverter &converter,
       const RedOperatorListTy &redOperatorList,
       llvm::SmallVectorImpl<mlir::Value> &reductionVars,
diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index b94833d852b2e..13420f365919d 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -2125,9 +2125,10 @@ class FirConverter : public Fortran::lower::AbstractConverter {
 
     llvm::SmallVector<mlir::Value> reduceVars;
     Fortran::lower::omp::ReductionProcessor rp;
-    rp.processReductionArguments<fir::DeclareReductionOp>(
+    bool result = rp.processReductionArguments<fir::DeclareReductionOp>(
         toLocation(), *this, info.reduceOperatorList, reduceVars,
         reduceVarByRef, reductionDeclSymbols, info.reduceSymList);
+    assert(result && "Failed to process `do concurrent` reductions");
 
     doConcurrentLoopOp.getReduceVarsMutable().assign(reduceVars);
     doConcurrentLoopOp.setReduceSymsAttr(
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 8b3ad57c53810..594f95ecdda63 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -1116,11 +1116,12 @@ bool ClauseProcessor::processInReduction(
         collectReductionSyms(clause, inReductionSyms);
 
         ReductionProcessor rp;
-        rp.processReductionArguments<mlir::omp::DeclareReductionOp>(
-            currentLocation, converter,
-            std::get<typename omp::clause::ReductionOperatorList>(clause.t),
-            inReductionVars, inReduceVarByRef, inReductionDeclSymbols,
-            inReductionSyms);
+        if (!rp.processReductionArguments<mlir::omp::DeclareReductionOp>(
+                currentLocation, converter,
+                std::get<typename omp::clause::ReductionOperatorList>(clause.t),
+                inReductionVars, inReduceVarByRef, inReductionDeclSymbols,
+                inReductionSyms))
+          inReductionSyms.clear();
 
         // Copy local lists into the output.
         llvm::copy(inReductionVars, std::back_inserter(result.inReductionVars));
@@ -1461,10 +1462,12 @@ bool ClauseProcessor::processReduction(
         }
 
         ReductionProcessor rp;
-        rp.processReductionArguments<mlir::omp::DeclareReductionOp>(
-            currentLocation, converter,
-            std::get<typename omp::clause::ReductionOperatorList>(clause.t),
-            reductionVars, reduceVarByRef, reductionDeclSymbols, reductionSyms);
+        if (!rp.processReductionArguments<mlir::omp::DeclareReductionOp>(
+                currentLocation, converter,
+                std::get<typename omp::clause::ReductionOperatorList>(clause.t),
+                reductionVars, reduceVarByRef, reductionDeclSymbols,
+                reductionSyms))
+          reductionSyms.clear();
         // Copy local lists into the output.
         llvm::copy(reductionVars, std::back_inserter(result.reductionVars));
         llvm::copy(reduceVarByRef, std::back_inserter(result.reductionByref));
@@ -1486,11 +1489,12 @@ bool ClauseProcessor::processTaskReduction(
         collectReductionSyms(clause, taskReductionSyms);
 
         ReductionProcessor rp;
-        rp.processReductionArguments<mlir::omp::DeclareReductionOp>(
-            currentLocation, converter,
-            std::get<typename omp::clause::ReductionOperatorList>(clause.t),
-            taskReductionVars, taskReduceVarByRef, taskReductionDeclSymbols,
-            taskReductionSyms);
+        if (!rp.processReductionArguments<mlir::omp::DeclareReductionOp>(
+                currentLocation, converter,
+                std::get<typename omp::clause::ReductionOperatorList>(clause.t),
+                taskReductionVars, taskReduceVarByRef, taskReductionDeclSymbols,
+                taskReductionSyms))
+          taskReductionSyms.clear();
         // Copy local lists into the output.
         llvm::copy(taskReductionVars,
                    std::back_inserter(result.taskReductionVars));
diff --git a/flang/lib/Lower/Support/ReductionProcessor.cpp b/flang/lib/Lower/Support/ReductionProcessor.cpp
index 80c32d066a38d..605a5b6b20b94 100644
--- a/flang/lib/Lower/Support/ReductionProcessor.cpp
+++ b/flang/lib/Lower/Support/ReductionProcessor.cpp
@@ -39,7 +39,7 @@ namespace lower {
 namespace omp {
 
 // explicit template declarations
-template void ReductionProcessor::processReductionArguments<
+template bool ReductionProcessor::processReductionArguments<
     mlir::omp::DeclareReductionOp, omp::clause::ReductionOperatorList>(
     mlir::Location currentLocation, lower::AbstractConverter &converter,
     const omp::clause::ReductionOperatorList &redOperatorList,
@@ -48,7 +48,7 @@ template void ReductionProcessor::processReductionArguments<
     llvm::SmallVectorImpl<mlir::Attribute> &reductionDeclSymbols,
     const llvm::SmallVectorImpl<const semantics::Symbol *> &reductionSymbols);
 
-template void ReductionProcessor::processReductionArguments<
+template bool ReductionProcessor::processReductionArguments<
     fir::DeclareReductionOp, llvm::SmallVector<fir::ReduceOperationEnum>>(
     mlir::Location currentLocation, lower::AbstractConverter &converter,
     const llvm::SmallVector<fir::ReduceOperationEnum> &redOperatorList,
@@ -607,7 +607,7 @@ static bool doReductionByRef(mlir::Value reductionVar) {
 }
 
 template <typename OpType, typename RedOperatorListTy>
-void ReductionProcessor::processReductionArguments(
+bool ReductionProcessor::processReductionArguments(
     mlir::Location currentLocation, lower::AbstractConverter &converter,
     const RedOperatorListTy &redOperatorList,
     llvm::SmallVectorImpl<mlir::Value> &reductionVars,
@@ -627,10 +627,10 @@ void ReductionProcessor::processReductionArguments(
               std::get_if<omp::clause::ProcedureDesignator>(&redOperator.u)) {
         if (!ReductionProcessor::supportedIntrinsicProcReduction(
                 *reductionIntrinsic)) {
-          return;
+          return false;
         }
       } else {
-        return;
+        return false;
       }
     }
   }
@@ -765,6 +765,8 @@ void ReductionProcessor::processReductionArguments(
 
   if (isDoConcurrent)
     builder.restoreInsertionPoint(dcIP);
+
+  return true;
 }
 
 const semantics::SourceName
diff --git a/flang/test/Lower/OpenMP/wsloop-reduction-non-intrinsic.f90 b/flang/test/Lower/OpenMP/wsloop-reduction-non-intrinsic.f90
new file mode 100644
index 0000000000000..70b2ae1111a50
--- /dev/null
+++ b/flang/test/Lower/OpenMP/wsloop-reduction-non-intrinsic.f90
@@ -0,0 +1,25 @@
+! Tests reduction processor behavior when a reduction symbol is not supported.
+
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s
+
+subroutine foo
+  implicit none
+  integer :: k, i
+
+  interface max
+    function max(m1,m2)
+      integer :: m1, m2
+    end function
+  end interface
+
+  !$omp do reduction (max: k)
+  do i=1,10
+  end do
+  !$omp end do
+end
+
+! Verify that unsupported reduction is ignored.
+! CHECK: omp.wsloop 
+! CHECK-SAME: private(@{{[^[:space:]]+}} %{{[^[:space:]]+}}
+! CHECK-SAME:         -> %{{[^[:space:]]+}} : !{{[^[:space:]]+}}) {
+! CHECK: }

[tblah]

Thanks for the quick patch. I wonder if for now this should be a NYI error rather than keeping the old behavior of silently ignoring it and producing the wrong result.

This also "fixes" #149700

Do you plan to implement these reductions or should I go ahead?

[ergawy]

Thanks for the quick patch. I wonder if for now this should be a NYI error rather than keeping the old behavior of silently ignoring it and producing the wrong result.

I also think it should be handled somehow. But in this PR, I am restoring what was happening already for OpenMP. Maybe handling should happen in a separate PR?

Do you plan to implement these reductions or should I go ahead?

I would suggest not in this PR.

[tblah]

Okay I guess there is a danger the NYI could regress applications that use this feature but don't actually care about a correct result so maybe it should land separately so it can be more easily reverted.

Please could you backport this to the LLVM 21 branch.

[ergawy]

Backport PR #150196.

[ergawy]

/cherry-pick 36c37b0

[llvmbot]

/pull-request #150200