R2: Addressing Review Comments

llvm · Jan 23, 2025 · f1c408b · f1c408b
1 parent d8e27cd
commit f1c408b
Show file tree

Hide file tree

Showing 5 changed files with 138 additions and 47 deletions.
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/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));

diff --git a/flang/lib/Lower/OpenMP/ReductionProcessor.cpp b/flang/lib/Lower/OpenMP/ReductionProcessor.cpp
@@ -36,6 +36,8 @@ namespace Fortran {
 namespace lower {
 namespace omp {
 
+using ReductionModifier = omp::clause::Reduction::ReductionModifier;
+
 ReductionProcessor::ReductionIdentifier ReductionProcessor::getReductionType(
     const omp::clause::ProcedureDesignator &pd) {
   auto redType = llvm::StringSwitch<std::optional<ReductionIdentifier>>(
@@ -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()));
   }
 }
 

diff --git a/flang/lib/Lower/OpenMP/ReductionProcessor.h b/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

diff --git a/flang/test/Lower/OpenMP/Todo/reduction-task.f90 b/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

diff --git a/flang/test/Lower/OpenMP/scan.f90 b/flang/test/Lower/OpenMP/scan.f90
@@ -1,33 +1,127 @@
-!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
+! NOTE: Assertions have been autogenerated by utils/generate-test-checks.py
+
+! CHECK-LABEL:   omp.declare_reduction @add_reduction_i32 : i32 init {
+! CHECK:         ^bb0(%[[VAL_0:.*]]: i32):
+! CHECK:           %[[VAL_1:.*]] = arith.constant 0 : i32
+! CHECK:           omp.yield(%[[VAL_1]] : i32)
+!
+! CHECK-LABEL:   } combiner {
+! CHECK:         ^bb0(%[[VAL_0:.*]]: i32, %[[VAL_1:.*]]: i32):
+! CHECK:           %[[VAL_2:.*]] = arith.addi %[[VAL_0]], %[[VAL_1]] : i32
+! CHECK:           omp.yield(%[[VAL_2]] : i32)
+! CHECK:         }
+!
+! CHECK-LABEL:   func.func @_QPinclusive_scan(
+! CHECK-SAME:                                 %[[VAL_0:.*]]: !fir.box<!fir.array<?xi32>> {fir.bindc_name = "a"},
+! CHECK-SAME:                                 %[[VAL_1:.*]]: !fir.box<!fir.array<?xi32>> {fir.bindc_name = "b"},
+! CHECK-SAME:                                 %[[VAL_2:.*]]: !fir.ref<i32> {fir.bindc_name = "n"}) {
+! CHECK:           %[[VAL_3:.*]] = fir.dummy_scope : !fir.dscope
+! CHECK:           %[[VAL_4:.*]]:2 = hlfir.declare %[[VAL_0]] dummy_scope %[[VAL_3]] {uniq_name = "_QFinclusive_scanEa"} : (!fir.box<!fir.array<?xi32>>, !fir.dscope) -> (!fir.box<!fir.array<?xi32>>, !fir.box<!fir.array<?xi32>>)
+! CHECK:           %[[VAL_5:.*]]:2 = hlfir.declare %[[VAL_1]] dummy_scope %[[VAL_3]] {uniq_name = "_QFinclusive_scanEb"} : (!fir.box<!fir.array<?xi32>>, !fir.dscope) -> (!fir.box<!fir.array<?xi32>>, !fir.box<!fir.array<?xi32>>)
+! CHECK:           %[[VAL_6:.*]] = fir.alloca i32 {bindc_name = "k", uniq_name = "_QFinclusive_scanEk"}
+! CHECK:           %[[VAL_7:.*]]:2 = hlfir.declare %[[VAL_6]] {uniq_name = "_QFinclusive_scanEk"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+! CHECK:           %[[VAL_8:.*]]:2 = hlfir.declare %[[VAL_2]] dummy_scope %[[VAL_3]] {uniq_name = "_QFinclusive_scanEn"} : (!fir.ref<i32>, !fir.dscope) -> (!fir.ref<i32>, !fir.ref<i32>)
+! CHECK:           %[[VAL_9:.*]] = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFinclusive_scanEx"}
+! CHECK:           %[[VAL_10:.*]]:2 = hlfir.declare %[[VAL_9]] {uniq_name = "_QFinclusive_scanEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+! CHECK:           omp.parallel {
+! CHECK:             %[[VAL_11:.*]] = fir.alloca i32 {bindc_name = "k", pinned, uniq_name = "_QFinclusive_scanEk"}
+! CHECK:             %[[VAL_12:.*]]:2 = hlfir.declare %[[VAL_11]] {uniq_name = "_QFinclusive_scanEk"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+! CHECK:             %[[VAL_13:.*]] = arith.constant 1 : i32
+! CHECK:             %[[VAL_14:.*]] = fir.load %[[VAL_8]]#0 : !fir.ref<i32>
+! CHECK:             %[[VAL_15:.*]] = arith.constant 1 : i32
+! CHECK:             omp.wsloop reduction(mod: inscan, @add_reduction_i32 %[[VAL_10]]#0 -> %[[VAL_16:.*]] : !fir.ref<i32>) {
+! CHECK:               omp.loop_nest (%[[VAL_17:.*]]) : i32 = (%[[VAL_13]]) to (%[[VAL_14]]) inclusive step (%[[VAL_15]]) {
+! CHECK:                 %[[VAL_18:.*]]:2 = hlfir.declare %[[VAL_16]] {uniq_name = "_QFinclusive_scanEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+! CHECK:                 fir.store %[[VAL_17]] to %[[VAL_12]]#1 : !fir.ref<i32>
+! CHECK:                 %[[VAL_19:.*]] = fir.load %[[VAL_18]]#0 : !fir.ref<i32>
+! CHECK:                 %[[VAL_20:.*]] = fir.load %[[VAL_12]]#0 : !fir.ref<i32>
+! CHECK:                 %[[VAL_21:.*]] = fir.convert %[[VAL_20]] : (i32) -> i64
+! CHECK:                 %[[VAL_22:.*]] = hlfir.designate %[[VAL_4]]#0 (%[[VAL_21]])  : (!fir.box<!fir.array<?xi32>>, i64) -> !fir.ref<i32>
+! CHECK:                 %[[VAL_23:.*]] = fir.load %[[VAL_22]] : !fir.ref<i32>
+! CHECK:                 %[[VAL_24:.*]] = arith.addi %[[VAL_19]], %[[VAL_23]] : i32
+! CHECK:                 hlfir.assign %[[VAL_24]] to %[[VAL_18]]#0 : i32, !fir.ref<i32>
+! CHECK:                 omp.scan inclusive(%[[VAL_18]]#1 : !fir.ref<i32>)
+! CHECK:                 %[[VAL_25:.*]] = fir.load %[[VAL_18]]#0 : !fir.ref<i32>
+! CHECK:                 %[[VAL_26:.*]] = fir.load %[[VAL_12]]#0 : !fir.ref<i32>
+! CHECK:                 %[[VAL_27:.*]] = fir.convert %[[VAL_26]] : (i32) -> i64
+! CHECK:                 %[[VAL_28:.*]] = hlfir.designate %[[VAL_5]]#0 (%[[VAL_27]])  : (!fir.box<!fir.array<?xi32>>, i64) -> !fir.ref<i32>
+! CHECK:                 hlfir.assign %[[VAL_25]] to %[[VAL_28]] : i32, !fir.ref<i32>
+! CHECK:                 omp.yield
+! CHECK:               }
+! CHECK:             }
+! CHECK:             omp.terminator
+! CHECK:           }
+! CHECK:           return
+! CHECK:         }
+!
+! CHECK-LABEL:   func.func @_QPexclusive_scan(
+! CHECK-SAME:                                 %[[VAL_0:.*]]: !fir.box<!fir.array<?xi32>> {fir.bindc_name = "a"},
+! CHECK-SAME:                                 %[[VAL_1:.*]]: !fir.box<!fir.array<?xi32>> {fir.bindc_name = "b"},
+! CHECK-SAME:                                 %[[VAL_2:.*]]: !fir.ref<i32> {fir.bindc_name = "n"}) {
+! CHECK:           %[[VAL_3:.*]] = fir.dummy_scope : !fir.dscope
+! CHECK:           %[[VAL_4:.*]]:2 = hlfir.declare %[[VAL_0]] dummy_scope %[[VAL_3]] {uniq_name = "_QFexclusive_scanEa"} : (!fir.box<!fir.array<?xi32>>, !fir.dscope) -> (!fir.box<!fir.array<?xi32>>, !fir.box<!fir.array<?xi32>>)
+! CHECK:           %[[VAL_5:.*]]:2 = hlfir.declare %[[VAL_1]] dummy_scope %[[VAL_3]] {uniq_name = "_QFexclusive_scanEb"} : (!fir.box<!fir.array<?xi32>>, !fir.dscope) -> (!fir.box<!fir.array<?xi32>>, !fir.box<!fir.array<?xi32>>)
+! CHECK:           %[[VAL_6:.*]] = fir.alloca i32 {bindc_name = "k", uniq_name = "_QFexclusive_scanEk"}
+! CHECK:           %[[VAL_7:.*]]:2 = hlfir.declare %[[VAL_6]] {uniq_name = "_QFexclusive_scanEk"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+! CHECK:           %[[VAL_8:.*]]:2 = hlfir.declare %[[VAL_2]] dummy_scope %[[VAL_3]] {uniq_name = "_QFexclusive_scanEn"} : (!fir.ref<i32>, !fir.dscope) -> (!fir.ref<i32>, !fir.ref<i32>)
+! CHECK:           %[[VAL_9:.*]] = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFexclusive_scanEx"}
+! CHECK:           %[[VAL_10:.*]]:2 = hlfir.declare %[[VAL_9]] {uniq_name = "_QFexclusive_scanEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+! CHECK:           omp.parallel {
+! CHECK:             %[[VAL_11:.*]] = fir.alloca i32 {bindc_name = "k", pinned, uniq_name = "_QFexclusive_scanEk"}
+! CHECK:             %[[VAL_12:.*]]:2 = hlfir.declare %[[VAL_11]] {uniq_name = "_QFexclusive_scanEk"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+! CHECK:             %[[VAL_13:.*]] = arith.constant 1 : i32
+! CHECK:             %[[VAL_14:.*]] = fir.load %[[VAL_8]]#0 : !fir.ref<i32>
+! CHECK:             %[[VAL_15:.*]] = arith.constant 1 : i32
+! CHECK:             omp.wsloop reduction(mod: inscan, @add_reduction_i32 %[[VAL_10]]#0 -> %[[VAL_16:.*]] : !fir.ref<i32>) {
+! CHECK:               omp.loop_nest (%[[VAL_17:.*]]) : i32 = (%[[VAL_13]]) to (%[[VAL_14]]) inclusive step (%[[VAL_15]]) {
+! CHECK:                 %[[VAL_18:.*]]:2 = hlfir.declare %[[VAL_16]] {uniq_name = "_QFexclusive_scanEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+! CHECK:                 fir.store %[[VAL_17]] to %[[VAL_12]]#1 : !fir.ref<i32>
+! CHECK:                 %[[VAL_19:.*]] = fir.load %[[VAL_18]]#0 : !fir.ref<i32>
+! CHECK:                 %[[VAL_20:.*]] = fir.load %[[VAL_12]]#0 : !fir.ref<i32>
+! CHECK:                 %[[VAL_21:.*]] = fir.convert %[[VAL_20]] : (i32) -> i64
+! CHECK:                 %[[VAL_22:.*]] = hlfir.designate %[[VAL_4]]#0 (%[[VAL_21]])  : (!fir.box<!fir.array<?xi32>>, i64) -> !fir.ref<i32>
+! CHECK:                 %[[VAL_23:.*]] = fir.load %[[VAL_22]] : !fir.ref<i32>
+! CHECK:                 %[[VAL_24:.*]] = arith.addi %[[VAL_19]], %[[VAL_23]] : i32
+! CHECK:                 hlfir.assign %[[VAL_24]] to %[[VAL_18]]#0 : i32, !fir.ref<i32>
+! CHECK:                 omp.scan exclusive(%[[VAL_18]]#1 : !fir.ref<i32>)
+! CHECK:                 %[[VAL_25:.*]] = fir.load %[[VAL_18]]#0 : !fir.ref<i32>
+! CHECK:                 %[[VAL_26:.*]] = fir.load %[[VAL_12]]#0 : !fir.ref<i32>
+! CHECK:                 %[[VAL_27:.*]] = fir.convert %[[VAL_26]] : (i32) -> i64
+! CHECK:                 %[[VAL_28:.*]] = hlfir.designate %[[VAL_5]]#0 (%[[VAL_27]])  : (!fir.box<!fir.array<?xi32>>, i64) -> !fir.ref<i32>
+! CHECK:                 hlfir.assign %[[VAL_25]] to %[[VAL_28]] : i32, !fir.ref<i32>
+! CHECK:                 omp.yield
+! CHECK:               }
+! CHECK:             }
+! CHECK:             omp.terminator
+! CHECK:           }
+! CHECK:           return
+! CHECK:         }
+
+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
+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