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

[Kareem Ergawy via flang-commits]

ergawy wrote:

The privatization crashes for both `target parallel` and `target teams` can be resolved by pushing a symbol map scope for both constructs: `parallel` and `teams`:
```diff
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index c13fa471978d..fa11c237282d 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -2668,6 +2668,7 @@ genTeamsOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
            semantics::SemanticsContext &semaCtx, lower::pft::Evaluation &eval,
            mlir::Location loc, const ConstructQueue &queue,
            ConstructQueue::const_iterator item) {
+  lower::SymMapScope scope(symTable);
   mlir::omp::TeamsOperands clauseOps;
   llvm::SmallVector<const semantics::Symbol *> reductionSyms;
   genTeamsClauses(converter, semaCtx, stmtCtx, item->clauses, loc, clauseOps,
@@ -2975,6 +2976,7 @@ static mlir::omp::ParallelOp genStandaloneParallel(
     lower::StatementContext &stmtCtx, semantics::SemanticsContext &semaCtx,
     lower::pft::Evaluation &eval, mlir::Location loc,
     const ConstructQueue &queue, ConstructQueue::const_iterator item) {
+  lower::SymMapScope scope(symTable);
   mlir::omp::ParallelOperands parallelClauseOps;
   llvm::SmallVector<const semantics::Symbol *> parallelReductionSyms;
   genParallelClauses(converter, semaCtx, stmtCtx, item->clauses, loc,
```
I think adding scopes makes sense for these constructs.

This produces correct IR for the sample that @kparzysz shared (and also if we modify it to use `teams` instead of `parallel`). Running `flang-check` with the above changes, all tests pass except for one test: `Lower/OpenMP/copyin.f90`.

The reason this test fails is that `sections` is handled differently from other constructs. In particular, if we have a `parallel sections` construct, the `genOMP([...], OpenMPSectionsConstruct)` function is **not** called from the parent `genOMP(....)` call (e.g. the call to generate the `parallel` op). So, the scope added (by the above diff) to `parallel` is popped before we generate the `sections` op. This special handling of `sections` was added in https://github.com/llvm/llvm-project/pull/97858. In particular, see line 2086 in `OpenMP.cpp` in https://github.com/llvm/llvm-project/pull/97858 to see the change I am referring to. @tblah I am not sure how difficult would it be to go back to making `sections` generation nested within parent constructs!

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