[flang-commits] [flang] [flang][OpenMP] Add TODOs for target [teams|parallel] private (PR #143706)

[Tom Eccles via flang-commits]

tblah wrote:

Thanks for taking a look at these bugs Kareem.

> @tblah I am not sure how difficult would it be to go back to making `sections` generation nested within parent constructs!

I definitely tried at the time and wasn't able to find a less hacky solution. The problem is the `parser::OmpSectionBlocks` argument for `genSectionsOp`. I tried something like this just now
```diff
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 784749bba5a0..bfee1ddad565 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -49,12 +49,16 @@ using namespace Fortran::common::openmp;
 // Code generation helper functions
 //===----------------------------------------------------------------------===//
 
-static void genOMPDispatch(lower::AbstractConverter &converter,
-                           lower::SymMap &symTable,
-                           semantics::SemanticsContext &semaCtx,
-                           lower::pft::Evaluation &eval, mlir::Location loc,
-                           const ConstructQueue &queue,
-                           ConstructQueue::const_iterator item);
+/// Pointer must not be null. Reference types aren't allowed inside of
+/// std::variant.
+using ExtraCodegenInfo = std::variant<const parser::OmpSectionBlocks *>;
+
+static void
+genOMPDispatch(lower::AbstractConverter &converter, lower::SymMap &symTable,
+               semantics::SemanticsContext &semaCtx,
+               lower::pft::Evaluation &eval, mlir::Location loc,
+               const ConstructQueue &queue, ConstructQueue::const_iterator item,
+               const std::optional<ExtraCodegenInfo> &extraInfo = std::nullopt);
 
 static void processHostEvalClauses(lower::AbstractConverter &converter,
                                    semantics::SemanticsContext &semaCtx,
@@ -3808,7 +3812,8 @@ static void genOMPDispatch(lower::AbstractConverter &converter,
                            semantics::SemanticsContext &semaCtx,
                            lower::pft::Evaluation &eval, mlir::Location loc,
                            const ConstructQueue &queue,
-                           ConstructQueue::const_iterator item) {
+                           ConstructQueue::const_iterator item,
+                           const std::optional<ExtraCodegenInfo> &extraInfo) {
   assert(item != queue.end());
 
   lower::StatementContext stmtCtx;
@@ -3875,10 +3880,16 @@ static void genOMPDispatch(lower::AbstractConverter &converter,
     // Lowered in the enclosing genSectionsOp.
     break;
   case llvm::omp::Directive::OMPD_sections:
-    // Called directly from genOMP([...], OpenMPSectionsConstruct) because it
-    // has a different prototype.
-    // This code path is still taken when iterating through the construct queue
-    // in genBodyOfOp
+    if (extraInfo) {
+      if (const auto *sectionBlocks =
+              std::get_if<const parser::OmpSectionBlocks *>(&*extraInfo)) {
+        assert(*sectionBlocks && "Must not store nullptr in ExtraLoweringInfo");
+        newOp = genSectionsOp(converter, symTable, semaCtx, eval, loc, queue,
+                              item, **sectionBlocks);
+        break;
+      }
+    }
+    llvm_unreachable("OMPD_sections without parser::OmpSectionBlocks");
     break;
   case llvm::omp::Directive::OMPD_simd:
     newOp =
@@ -4438,20 +4449,8 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
                           eval, source, directive, clauses)};
   ConstructQueue::iterator next = queue.begin();
   // Generate constructs that come first e.g. Parallel
-  while (next != queue.end() &&
-         next->id != llvm::omp::Directive::OMPD_sections) {
-    genOMPDispatch(converter, symTable, semaCtx, eval, currentLocation, queue,
-                   next);
-    next = std::next(next);
-  }
-
-  // call genSectionsOp directly (not via genOMPDispatch) so that we can add the
-  // sectionBlocks argument
-  assert(next != queue.end());
-  assert(next->id == llvm::omp::Directive::OMPD_sections);
-  genSectionsOp(converter, symTable, semaCtx, eval, currentLocation, queue,
-                next, sectionBlocks);
-  assert(std::next(next) == queue.end());
+  genOMPDispatch(converter, symTable, semaCtx, eval, currentLocation, queue,
+                 next, ExtraCodegenInfo{&sectionBlocks});
 }
 
 static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
```

This doesn't work because the extra argument isn't added when this comes from a `parallel sections`. But I don't think there's any good way to avoid needing to construct each section inside of the overall sections construct. But I would be very happy if you had a better idea.

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