Clean up code and add TODO to address review comments.

Author: jsjodin

flang/lib/Lower/OpenMP/OpenMP.cpp

@@ -3565,72 +3565,73 @@ genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
     TODO(converter.getCurrentLocation(), "OmpDeclareVariantDirective");
 }
 
-static bool
+static ReductionProcessor::GenCombinerCBTy
 processReductionCombiner(lower::AbstractConverter &converter,
                          lower::SymMap &symTable,
                          semantics::SemanticsContext &semaCtx,
-                         const parser::OmpReductionSpecifier &specifier,
-                         ReductionProcessor::GenCombinerCBTy &genCombinerCB) {
+                         const parser::OmpReductionSpecifier &specifier) {
+  ReductionProcessor::GenCombinerCBTy genCombinerCB;
   const auto &combinerExpression =
       std::get<std::optional<parser::OmpCombinerExpression>>(specifier.t)
           .value();
   const parser::OmpStylizedInstance &combinerInstance =
       combinerExpression.v.front();
   const parser::OmpStylizedInstance::Instance &instance =
       std::get<parser::OmpStylizedInstance::Instance>(combinerInstance.t);
-  if (const auto *as = std::get_if<parser::AssignmentStmt>(&instance.u)) {
-    auto &expr = std::get<parser::Expr>(as->t);
-    genCombinerCB = [&](fir::FirOpBuilder &builder, mlir::Location loc,
-                        mlir::Type type, mlir::Value lhs, mlir::Value rhs,
-                        bool isByRef) {
-      const auto &evalExpr = makeExpr(expr, semaCtx);
-      lower::SymMapScope scope(symTable);
-      const std::list<parser::OmpStylizedDeclaration> &declList =
-          std::get<std::list<parser::OmpStylizedDeclaration>>(
-              combinerInstance.t);
-      for (const parser::OmpStylizedDeclaration &decl : declList) {
-        auto &name = std::get<parser::ObjectName>(decl.var.t);
-        mlir::Value addr = lhs;
-        mlir::Type type = lhs.getType();
-        bool isRhs = name.ToString() == std::string("omp_in");
-        if (isRhs) {
-          addr = rhs;
-          type = rhs.getType();
-        }
 
-        assert(name.symbol && "Reduction object name does not have a symbol");
-        if (!fir::conformsWithPassByRef(type)) {
-          addr = builder.createTemporary(loc, type);
-          fir::StoreOp::create(builder, loc, isRhs ? rhs : lhs, addr);
-        }
-        fir::FortranVariableFlagsEnum extraFlags = {};
-        fir::FortranVariableFlagsAttr attributes =
-            Fortran::lower::translateSymbolAttributes(builder.getContext(),
-                                                      *name.symbol, extraFlags);
-        auto declareOp = hlfir::DeclareOp::create(
-            builder, loc, addr, name.ToString(), nullptr, {}, nullptr, nullptr,
-            0, attributes);
-        symTable.addVariableDefinition(*name.symbol, declareOp);
+  const auto *as = std::get_if<parser::AssignmentStmt>(&instance.u);
+  if (!as) {
+    TODO(converter.getCurrentLocation(),
+         "A combiner that is a subroutine call is not yet supported");
+  }
+  auto &expr = std::get<parser::Expr>(as->t);
+  genCombinerCB = [&](fir::FirOpBuilder &builder, mlir::Location loc,
+                      mlir::Type type, mlir::Value lhs, mlir::Value rhs,
+                      bool isByRef) {
+    const auto &evalExpr = makeExpr(expr, semaCtx);
+    lower::SymMapScope scope(symTable);
+    const std::list<parser::OmpStylizedDeclaration> &declList =
+        std::get<std::list<parser::OmpStylizedDeclaration>>(combinerInstance.t);
+    for (const parser::OmpStylizedDeclaration &decl : declList) {
+      auto &name = std::get<parser::ObjectName>(decl.var.t);
+      mlir::Value addr = lhs;
+      mlir::Type type = lhs.getType();
+      bool isRhs = name.ToString() == std::string("omp_in");
+      if (isRhs) {
+        addr = rhs;
+        type = rhs.getType();
       }
 
-      lower::StatementContext stmtCtx;
-      mlir::Value result = fir::getBase(
-          convertExprToValue(loc, converter, evalExpr, symTable, stmtCtx));
-      if (auto refType = llvm::dyn_cast<fir::ReferenceType>(result.getType()))
-        if (lhs.getType() == refType.getElementType())
-          result = fir::LoadOp::create(builder, loc, result);
-      stmtCtx.finalizeAndPop();
-      if (isByRef) {
-        fir::StoreOp::create(builder, loc, result, lhs);
-        mlir::omp::YieldOp::create(builder, loc, lhs);
-      } else {
-        mlir::omp::YieldOp::create(builder, loc, result);
+      assert(name.symbol && "Reduction object name does not have a symbol");
+      if (!fir::conformsWithPassByRef(type)) {
+        addr = builder.createTemporary(loc, type);
+        fir::StoreOp::create(builder, loc, isRhs ? rhs : lhs, addr);
       }
+      fir::FortranVariableFlagsEnum extraFlags = {};
+      fir::FortranVariableFlagsAttr attributes =
+          Fortran::lower::translateSymbolAttributes(builder.getContext(),
+                                                    *name.symbol, extraFlags);
+      auto declareOp =
+          hlfir::DeclareOp::create(builder, loc, addr, name.ToString(), nullptr,
+                                   {}, nullptr, nullptr, 0, attributes);
+      symTable.addVariableDefinition(*name.symbol, declareOp);
+    }
 
-      return result;
-    };
-  }
-  return true;
+    lower::StatementContext stmtCtx;
+    mlir::Value result = fir::getBase(
+        convertExprToValue(loc, converter, evalExpr, symTable, stmtCtx));
+    if (auto refType = llvm::dyn_cast<fir::ReferenceType>(result.getType()))
+      if (lhs.getType() == refType.getElementType())
+        result = fir::LoadOp::create(builder, loc, result);
+    stmtCtx.finalizeAndPop();
+    if (isByRef) {
+      fir::StoreOp::create(builder, loc, result, lhs);
+      mlir::omp::YieldOp::create(builder, loc, lhs);
+    } else {
+      mlir::omp::YieldOp::create(builder, loc, result);
+    }
+  };
+  return genCombinerCB;
 }
 
 // Getting the type from a symbol compared to a DeclSpec is simpler since we do
@@ -3657,45 +3658,43 @@ static void genOMP(
     lower::AbstractConverter &converter, lower::SymMap &symTable,
     semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
     const parser::OpenMPDeclareReductionConstruct &declareReductionConstruct) {
-  if (!semaCtx.langOptions().OpenMPSimd) {
-    const parser::OmpArgumentList &args{
-        declareReductionConstruct.v.Arguments()};
-    const parser::OmpArgument &arg{args.v.front()};
-    const auto &specifier = std::get<parser::OmpReductionSpecifier>(arg.u);
-
-    if (std::get<parser::OmpTypeNameList>(specifier.t).v.size() > 1)
-      TODO(converter.getCurrentLocation(),
-           "multiple types in declare reduction is not yet supported");
-
-    mlir::Type reductionType = getReductionType(converter, specifier);
-    ReductionProcessor::GenCombinerCBTy genCombinerCB;
-    processReductionCombiner(converter, symTable, semaCtx, specifier,
-                             genCombinerCB);
-    const parser::OmpClauseList &initializer =
-        declareReductionConstruct.v.Clauses();
-    if (initializer.v.size() > 0) {
-      List<Clause> clauses = makeClauses(initializer, semaCtx);
-      ReductionProcessor::GenInitValueCBTy genInitValueCB;
-      ClauseProcessor cp(converter, semaCtx, clauses);
-      const parser::OmpClause::Initializer &iclause{
-          std::get<parser::OmpClause::Initializer>(initializer.v.front().u)};
-      cp.processInitializer(symTable, iclause, genInitValueCB);
-      const auto &identifier =
-          std::get<parser::OmpReductionIdentifier>(specifier.t);
-      const auto &designator =
-          std::get<parser::ProcedureDesignator>(identifier.u);
-      const auto &reductionName = std::get<parser::Name>(designator.u);
-      bool isByRef = ReductionProcessor::doReductionByRef(reductionType);
-      ReductionProcessor::createDeclareReductionHelper<
-          mlir::omp::DeclareReductionOp>(
-          converter, reductionName.ToString(), reductionType,
-          converter.getCurrentLocation(), isByRef, genCombinerCB,
-          genInitValueCB);
-    } else {
-      TODO(converter.getCurrentLocation(),
-           "declare reduction without an initializer clause is not yet "
-           "supported");
-    }
+  if (semaCtx.langOptions().OpenMPSimd)
+    return;
+
+  const parser::OmpArgumentList &args{declareReductionConstruct.v.Arguments()};
+  const parser::OmpArgument &arg{args.v.front()};
+  const auto &specifier = std::get<parser::OmpReductionSpecifier>(arg.u);
+
+  if (std::get<parser::OmpTypeNameList>(specifier.t).v.size() > 1)
+    TODO(converter.getCurrentLocation(),
+         "multiple types in declare reduction is not yet supported");
+
+  mlir::Type reductionType = getReductionType(converter, specifier);
+  ReductionProcessor::GenCombinerCBTy genCombinerCB =
+      processReductionCombiner(converter, symTable, semaCtx, specifier);
+  const parser::OmpClauseList &initializer =
+      declareReductionConstruct.v.Clauses();
+  if (initializer.v.size() > 0) {
+    List<Clause> clauses = makeClauses(initializer, semaCtx);
+    ReductionProcessor::GenInitValueCBTy genInitValueCB;
+    ClauseProcessor cp(converter, semaCtx, clauses);
+    const parser::OmpClause::Initializer &iclause{
+        std::get<parser::OmpClause::Initializer>(initializer.v.front().u)};
+    cp.processInitializer(symTable, iclause, genInitValueCB);
+    const auto &identifier =
+        std::get<parser::OmpReductionIdentifier>(specifier.t);
+    const auto &designator =
+        std::get<parser::ProcedureDesignator>(identifier.u);
+    const auto &reductionName = std::get<parser::Name>(designator.u);
+    bool isByRef = ReductionProcessor::doReductionByRef(reductionType);
+    ReductionProcessor::createDeclareReductionHelper<
+        mlir::omp::DeclareReductionOp>(
+        converter, reductionName.ToString(), reductionType,
+        converter.getCurrentLocation(), isByRef, genCombinerCB, genInitValueCB);
+  } else {
+    TODO(converter.getCurrentLocation(),
+         "declare reduction without an initializer clause is not yet "
+         "supported");
   }
 }