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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I think this is pretty good. Minor nits. I need to check when I'm less sleepy again.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:4364
+            *this, IRPosition::function(*ParallelRegion), DepClassTy::OPTIONAL);
+        if (!FnAA.ReachedKnownParallelRegions.empty() ||
+            !FnAA.ReachedUnknownParallelRegions.empty())
----------------
josemonsalve2 wrote:
> 
> ```
> NestedParallelism = !FnAA.ReachedKnownParallelRegions.empty() || !FnAA.ReachedUnknownParallelRegions.empty()
> ```
If you change it, then `|=`, the current way is OK too.

One thing to add though: `!FnAA.getState().isValidState() || ...`


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:3344
+        ConstantInt::get(Int1Ty, NestedParallelism ? 1 : 0),
+        Kernel->getName() + "_nested_parallelism");
+
----------------
jdoerfert wrote:
> Use an int8 type, int1 is always tricky. 
Unused variable NP.


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

https://reviews.llvm.org/D141010