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

[Tom Eccles 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),
----------------
tblah wrote:

Thanks for the review and detailed example.

I didn't do this because conceptually the reduction variables are private to each SECTION so references to that variable in one section are operating on different memory to another SECTION (until it is reduced, which is implicit in MLIR).

In practice, the lowering to LLVMIR (https://github.com/llvm/llvm-project/pull/97859) does just map the SECTIONS block arguments to the block arguments of each SECTION (and then this is only used as a mold for the private variable inside of the SECTION).

I'm more than happy to change it as you described if you still think that would be better.

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