[flang-commits] [flang] [llvm] [flang][OpenMP] Try to unify induction var privatization for OMP regions. (PR #91116)

[Kareem Ergawy via flang-commits]

================
@@ -289,12 +290,34 @@ void DataSharingProcessor::collectSymbolsInNestedRegions(
        eval.getNestedEvaluations()) {
     if (nestedEval.hasNestedEvaluations()) {
       if (nestedEval.isConstruct())
-        // Recursively look for OpenMP constructs within `nestedEval`'s region
         collectSymbolsInNestedRegions(nestedEval, flag, symbolsInNestedRegions);
-      else
-        converter.collectSymbolSet(nestedEval, symbolsInNestedRegions, flag,
-                                   /*collectSymbols=*/true,
-                                   /*collectHostAssociatedSymbols=*/false);
+      else {
+        bool isOrderedConstruct = [&]() {
----------------
ergawy wrote:

I can describe the problem in more details but cannot claim my solution is the proper one.

For the above example program, this is the output of `flang-new -fc1 -fopenmp -fdebug-unparse-with-symbols`:
```fortram
!DEF: /foo (Subroutine) Subprogram
subroutine foo
 !DEF: /foo/a ObjectEntity INTEGER(4)
 !DEF: /foo/i ObjectEntity INTEGER(4)
 integer a(10), i
!$omp do  ordered
 !DEF: /foo/OtherConstruct1/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
 do i=2,10
!$omp ordered 
  !REF: /foo/a
  !REF: /foo/OtherConstruct1/i
  a(i) = a(i-1)+1
!$omp end ordered 
 end do
!$omp end do 
end subroutine
```

So, `i` is marked as being `REF`ed from within the `omp ordered` construct (not defined).

However, when you call `converter.collectSymbolSet(nestedEval, symbolsInNestedRegions, ...)` when `nestedEval` is the `omp ordered` construct, `i` will be one of the symbols collected in that nested region.

This in turn, causes `i` **not** to be privatized when privatizing symbols that should be privatized for the `omp do ordered` construct (`i` is skipped because it is referenced by a nested region).

Since, `omp ordered` does not have a data environment of its own (as far as I understand), I thought it would reasonable to not exclude the symbols collected for a nested eval when that nested eval is an `ordered` directive.

That said, I am not sure this is the proper location for that kind of fix. But a similar fix to what was introduced in https://github.com/llvm/llvm-project/pull/90671 does not seem to work. Tried adding that in `bool OmpAttributeVisitor::Pre(const parser::OpenMPBlockConstruct &x);` but I get the same problem described above.

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