[PATCH] D141010: OpenMPOpt: Check nested parallelism in target region

[Jose Manuel Monsalve Diaz via Phabricator via llvm-commits]

josemonsalve2 added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:602
+  /// Flag that indicates if the kernel has nested Parallelism
+  bool NestedParallelism = false;
+
----------------
jdoerfert wrote:
> randreshg wrote:
> > tianshilei1992 wrote:
> > > randreshg wrote:
> > > > tianshilei1992 wrote:
> > > > > Is it good enough to just have two states? Is it possible that we can't tell if a parallel region is nested or not, or do we just conservatively take it as `true`?
> > > > What others states we might possibly have? The only way we can't tell if a parallel region is nested or not is when we have functions with parallel regions in another TU, for this case we conservatively take it as `false`, for all the other cases this approach should work.
> > > If a function containing a parallel region can be reached from different paths, and the parallel region may or may not be nested, that would be the case right? What's more, I suppose we could do more aggressive optimization if we do not have nested parallelism, which means the conservative way would be to take it as a `true`. Did I misunderstand anything here?
> > The case is summarized in the remark [[ https://openmp.llvm.org//remarks/OMP133.html#omp133 | omp133]]. This is the case when a function inside the target region may contain parallel regions but since we don't have information about it, we assume it has no nested parallelism
> We start optimistic, so assuming no nested parallelism is right.
Shilei, are you referring to something like this:

```
void foo () {
  #pragma omp parallel
  {}
}

void bar() {
  #target
      foo();
}

void zoo() {
  #target parallel
      foo();
}
```

I believe the optimization is not from the perspective of the inner parallel region, but the kernel itself. So this would be consider nested. 

Rafa, Correct me if I am wrong. 


Repository:
  rOMP OpenMP

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141010/new/

https://reviews.llvm.org/D141010