R2: Addressing Review Comments

Author: anchuraj

flang/lib/Lower/OpenMP/ClauseProcessor.cpp

@@ -347,14 +347,15 @@ bool ClauseProcessor::processDistSchedule(
 bool ClauseProcessor::processExclusive(
     mlir::Location currentLocation,
     mlir::omp::ExclusiveClauseOps &result) const {
-  return findRepeatableClause<omp::clause::Exclusive>(
-      [&](const omp::clause::Exclusive &clause, const parser::CharBlock &) {
-        for (const Object &object : clause.v) {
-          const semantics::Symbol *symbol = object.sym();
-          mlir::Value symVal = converter.getSymbolAddress(*symbol);
-          result.exclusiveVars.push_back(symVal);
-        }
-      });
+  if (auto *clause = findUniqueClause<omp::clause::Exclusive>()) {
+    for (const Object &object : clause->v) {
+      const semantics::Symbol *symbol = object.sym();
+      mlir::Value symVal = converter.getSymbolAddress(*symbol);
+      result.exclusiveVars.push_back(symVal);
+    }
+    return true;
+  }
+  return false;
 }
 
 bool ClauseProcessor::processFilter(lower::StatementContext &stmtCtx,
@@ -396,14 +397,15 @@ bool ClauseProcessor::processHint(mlir::omp::HintClauseOps &result) const {
 bool ClauseProcessor::processInclusive(
     mlir::Location currentLocation,
     mlir::omp::InclusiveClauseOps &result) const {
-  return findRepeatableClause<omp::clause::Inclusive>(
-      [&](const omp::clause::Inclusive &clause, const parser::CharBlock &) {
-        for (const Object &object : clause.v) {
-          const semantics::Symbol *symbol = object.sym();
-          mlir::Value symVal = converter.getSymbolAddress(*symbol);
-          result.inclusiveVars.push_back(symVal);
-        }
-      });
+  if (auto *clause = findUniqueClause<omp::clause::Inclusive>()) {
+    for (const Object &object : clause->v) {
+      const semantics::Symbol *symbol = object.sym();
+      mlir::Value symVal = converter.getSymbolAddress(*symbol);
+      result.inclusiveVars.push_back(symVal);
+    }
+    return true;
+  }
+  return false;
 }
 
 bool ClauseProcessor::processMergeable(
@@ -1163,7 +1165,7 @@ bool ClauseProcessor::processReduction(
         ReductionProcessor rp;
         rp.addDeclareReduction(
             currentLocation, converter, clause, reductionVars, reduceVarByRef,
-            reductionDeclSymbols, reductionSyms, &result.reductionMod);
+            reductionDeclSymbols, reductionSyms, result.reductionMod);
         // Copy local lists into the output.
         llvm::copy(reductionVars, std::back_inserter(result.reductionVars));
         llvm::copy(reduceVarByRef, std::back_inserter(result.reductionByref));

flang/lib/Lower/OpenMP/ReductionProcessor.cpp

@@ -25,13 +25,15 @@
 #include "flang/Parser/tools.h"
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
 #include "llvm/Support/CommandLine.h"
-#include <string>
 
 static llvm::cl::opt<bool> forceByrefReduction(
     "force-byref-reduction",
     llvm::cl::desc("Pass all reduction arguments by reference"),
     llvm::cl::Hidden);
 
+using ReductionModifier =
+    Fortran::lower::omp::clause::Reduction::ReductionModifier;
+
 namespace Fortran {
 namespace lower {
 namespace omp {
@@ -515,9 +517,8 @@ static bool doReductionByRef(mlir::Value reductionVar) {
   return false;
 }
 
-mlir::omp::ReductionModifier
-translateReductionModifier(const ReductionModifier &m) {
-  switch (m) {
+mlir::omp::ReductionModifier translateReductionModifier(ReductionModifier mod) {
+  switch (mod) {
   case ReductionModifier::Default:
     return mlir::omp::ReductionModifier::defaultmod;
   case ReductionModifier::Inscan:
@@ -535,15 +536,16 @@ void ReductionProcessor::addDeclareReduction(
     llvm::SmallVectorImpl<bool> &reduceVarByRef,
     llvm::SmallVectorImpl<mlir::Attribute> &reductionDeclSymbols,
     llvm::SmallVectorImpl<const semantics::Symbol *> &reductionSymbols,
-    mlir::omp::ReductionModifierAttr *reductionMod) {
+    mlir::omp::ReductionModifierAttr &reductionMod) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
 
   auto mod = std::get<std::optional<ReductionModifier>>(reduction.t);
-  if (mod.has_value() && (mod.value() != ReductionModifier::Inscan)) {
-    std::string modStr = "default";
+  if (mod.has_value()) {
     if (mod.value() == ReductionModifier::Task)
-      modStr = "task";
-    TODO(currentLocation, "Reduction modifier " + modStr + " is not supported");
+      TODO(currentLocation, "Reduction modifier `task` is not supported");
+    else
+      reductionMod = mlir::omp::ReductionModifierAttr::get(
+          firOpBuilder.getContext(), translateReductionModifier(mod.value()));
   }
 
   mlir::omp::DeclareReductionOp decl;
@@ -668,11 +670,6 @@ void ReductionProcessor::addDeclareReduction(
                                   currentLocation, isByRef);
     reductionDeclSymbols.push_back(
         mlir::SymbolRefAttr::get(firOpBuilder.getContext(), decl.getSymName()));
-    auto redMod = std::get<std::optional<ReductionModifier>>(reduction.t);
-    if (redMod.has_value())
-      *reductionMod = mlir::omp::ReductionModifierAttr::get(
-          firOpBuilder.getContext(),
-          translateReductionModifier(redMod.value()));
   }
 }
 

flang/lib/Lower/OpenMP/ReductionProcessor.h

@@ -128,7 +128,7 @@ class ReductionProcessor {
       llvm::SmallVectorImpl<bool> &reduceVarByRef,
       llvm::SmallVectorImpl<mlir::Attribute> &reductionDeclSymbols,
       llvm::SmallVectorImpl<const semantics::Symbol *> &reductionSymbols,
-      mlir::omp::ReductionModifierAttr *reductionMod);
+      mlir::omp::ReductionModifierAttr &reductionMod);
 };
 
 template <typename FloatOp, typename IntegerOp>
@@ -158,8 +158,6 @@ ReductionProcessor::getReductionOperation(fir::FirOpBuilder &builder,
   return builder.create<ComplexOp>(loc, op1, op2);
 }
 
-using ReductionModifier = omp::clause::Reduction::ReductionModifier;
-
 } // namespace omp
 } // namespace lower
 } // namespace Fortran

flang/test/Lower/OpenMP/Todo/reduction-task.f90

@@ -1,7 +1,7 @@
 ! RUN: %not_todo_cmd bbc -emit-fir -fopenmp -o - %s 2>&1 | FileCheck %s
 ! RUN: %not_todo_cmd %flang_fc1 -emit-fir -fopenmp -o - %s 2>&1 | FileCheck %s
 
-! CHECK: not yet implemented: Reduction modifier task is not supported
+! CHECK: not yet implemented: Reduction modifier `task` is not supported
 subroutine reduction_task()
   integer :: i
   i = 0

flang/test/Lower/OpenMP/scan.f90

@@ -1,33 +1,35 @@
-!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+! RUN: bbc -emit-hlfir -fopenmp %s -o - | FileCheck %s
+! RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
 
-subroutine inclusive_scan
+! CHECK: omp.wsloop reduction(mod: inscan, @add_reduction_i32 %{{.*}} -> %[[RED_ARG_1:.*]] : {{.*}}) {
+! CHECK: %[[RED_DECL_1:.*]]:2 = hlfir.declare %[[RED_ARG_1]]
+! CHECK: omp.scan inclusive(%[[RED_DECL_1]]#1 : {{.*}})
+
+subroutine inclusive_scan(a, b, n)
  implicit none
- integer, parameter :: n = 100
- integer a(n), b(n)
- integer x, k
+ integer a(:), b(:)
+ integer x, k, n
 
- !CHECK: omp.wsloop reduction(mod: inscan, {{.*}}) {
  !$omp parallel do reduction(inscan, +: x)
  do k = 1, n
    x = x + a(k)
-   !CHECK: omp.scan inclusive({{.*}})
    !$omp scan inclusive(x)
    b(k) = x
  end do
 end subroutine inclusive_scan
 
 
-subroutine exclusive_scan
+! CHECK: omp.wsloop reduction(mod: inscan, @add_reduction_i32 %{{.*}} -> %[[RED_ARG_2:.*]] : {{.*}}) {
+! CHECK: %[[RED_DECL_2:.*]]:2 = hlfir.declare %[[RED_ARG_2]]
+! CHECK: omp.scan exclusive(%[[RED_DECL_2]]#1 : {{.*}})
+subroutine exclusive_scan(a, b, n)
  implicit none
- integer, parameter :: n = 100
- integer a(n), b(n)
- integer x, k
+ integer a(:), b(:)
+ integer x, k, n
 
- !CHECK: omp.wsloop reduction(mod: inscan, {{.*}}) {
  !$omp parallel do reduction(inscan, +: x)
  do k = 1, n
    x = x + a(k)
-   !CHECK: omp.scan exclusive({{.*}})
    !$omp scan exclusive(x)
    b(k) = x
  end do