[flang-commits] [flang] [llvm] [flang][OpenMP] Decompose compound constructs, do recursive lowering (PR #90098)

[Sergio Afonso via flang-commits]

================
@@ -1447,43 +1397,47 @@ genParallelOp(Fortran::lower::AbstractConverter &converter,
 
   // TODO Merge with the reduction CB.
   genInfo.setGenRegionEntryCb(genRegionEntryCB).setDataSharingProcessor(&dsp);
-  return genOpWithBody<mlir::omp::ParallelOp>(genInfo, clauseOps);
+  return genOpWithBody<mlir::omp::ParallelOp>(genInfo, queue, item, clauseOps);
 }
 
 static mlir::omp::SectionOp
 genSectionOp(Fortran::lower::AbstractConverter &converter,
              Fortran::lower::SymMap &symTable,
              Fortran::semantics::SemanticsContext &semaCtx,
-             Fortran::lower::pft::Evaluation &eval, bool genNested,
-             mlir::Location loc, const List<Clause> &clauses) {
+             Fortran::lower::pft::Evaluation &eval, mlir::Location loc,
+             const List<Clause> &clauses, const ConstructQueue &queue,
+             ConstructQueue::iterator item) {
   // Currently only private/firstprivate clause is handled, and
   // all privatization is done within `omp.section` operations.
   return genOpWithBody<mlir::omp::SectionOp>(
       OpWithBodyGenInfo(converter, symTable, semaCtx, loc, eval,
                         llvm::omp::Directive::OMPD_section)
-          .setGenNested(genNested)
-          .setClauses(&clauses));
+          .setClauses(&clauses),
+      queue, item);
 }
 
 static mlir::omp::SectionsOp
 genSectionsOp(Fortran::lower::AbstractConverter &converter,
               Fortran::lower::SymMap &symTable,
               Fortran::semantics::SemanticsContext &semaCtx,
               Fortran::lower::pft::Evaluation &eval, mlir::Location loc,
-              const mlir::omp::SectionsClauseOps &clauseOps) {
+              const List<Clause> &clauses, const ConstructQueue &queue,
+              ConstructQueue::iterator item) {
+  mlir::omp::SectionsClauseOps clauseOps;
+  genSectionsClauses(converter, semaCtx, clauses, loc, clauseOps);
----------------
skatrak wrote:

Yes, it's a good point. What we had before would treat the combined form of the construct differently from nesting one construct inside of the other, when both should ideally produce the same MLIR representation.

This change makes the combined variant work the same as the nested variant and not the other way around (which I agree would be quite a bit more difficult to implement). So my concern is mainly about the chance that this might be breaking something that used to work if the right way to handle this case (according to the spec) was what was implemented for the combined variant of the construct rather than what's done for the nested variant.

By looking at the original code comments in `genOMP` it seems like this was intentional. There are a couple of tests for this in flang/test/Lower/OpenMP/parallel-sections.f90 and flang/test/Lower/OpenMP/copyin.f90. so assuming those properly test the expected behavior I suppose if they aren't broken by this change we're fine.

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