[flang-commits] [flang] [flang][OpenMP] Lower REDUCTION clause for SECTIONS (PR #97858)

[Sergio Afonso via flang-commits]

================
@@ -1530,11 +1525,52 @@ genSectionsOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
   }
 
   // SECTIONS construct.
-  mlir::omp::SectionsOp sectionsOp = genOpWithBody<mlir::omp::SectionsOp>(
-      OpWithBodyGenInfo(converter, symTable, semaCtx, loc, eval,
-                        llvm::omp::Directive::OMPD_sections)
-          .setClauses(&nonDsaClauses),
-      queue, item, clauseOps);
+  auto sectionsOp = builder.create<mlir::omp::SectionsOp>(loc, clauseOps);
+
+  auto reductionCallback = [&](mlir::Operation *op) {
+    genReductionVars(op, converter, loc, reductionSyms, reductionTypes);
+    return reductionSyms;
+  };
+
+  reductionCallback(sectionsOp);
+  // genReductionVars adds a hlfir.declare for the reduction block argument
+  // but only terminators and sectionOps are allowed inside of a SectionsOp
+  llvm::SmallVector<mlir::Operation *> toErase;
+  toErase.reserve(reductionSyms.size());
+  for (auto decl : sectionsOp.getOps<hlfir::DeclareOp>())
+    toErase.push_back(decl);
+  for (mlir::Operation *op : toErase)
+    op->erase();
+
+  mlir::Operation *terminator =
+      lower::genOpenMPTerminator(builder, sectionsOp, loc);
+
+  // Generate nested SECTION constructs.
+  // This is done here rather than in genOMP([...], OpenMPSectionConstruct )
+  // because we need to run genReductionVars on each omp.section so that the
+  // reduction variable gets mapped to the private version
+  for (auto [construct, nestedEval] :
+       llvm::zip(sectionBlocks.v, eval.getNestedEvaluations())) {
+    const auto *sectionConstruct =
+        std::get_if<parser::OpenMPSectionConstruct>(&construct.u);
+    if (!sectionConstruct) {
+      assert(false &&
+             "unexpected construct nested inside of SECTIONS construct");
+      continue;
+    }
+
+    ConstructQueue sectionQueue{buildConstructQueue(
+        converter.getFirOpBuilder().getModule(), semaCtx, nestedEval,
+        sectionConstruct->source, llvm::omp::Directive::OMPD_section, {})};
+
+    builder.setInsertionPoint(terminator);
+    genOpWithBody<mlir::omp::SectionOp>(
+        OpWithBodyGenInfo(converter, symTable, semaCtx, loc, nestedEval,
+                          llvm::omp::Directive::OMPD_section)
+            .setClauses(&sectionQueue.begin()->clauses)
+            .setGenRegionEntryCb(reductionCallback),
----------------
skatrak wrote:

I see, thank you for the explanation. I think that approach makes sense, so I won't oppose it. I'd suggest documenting this correspondence between entry block arguments in the op descriptions for `omp.sections` and `omp.section`, but that can be a separate NFC PR.

I think it's still worth avoiding the addition+deletion of `hlfir.declare` ops inside of `omp.sections` by replacing the call to the `reductionCallback` lambda with my suggestion above, but I'll leave it up to you if you prefer to leave it as it is. In my opinion, it's a minimal amount of code duplication in exchange for making it shorter and easier to follow.

https://github.com/llvm/llvm-project/pull/97858