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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:4364
+            *this, IRPosition::function(*ParallelRegion), DepClassTy::OPTIONAL);
+        if (!FnAA.ReachedKnownParallelRegions.empty() ||
+            !FnAA.ReachedUnknownParallelRegions.empty())
----------------
randreshg wrote:
> josemonsalve2 wrote:
> > jdoerfert wrote:
> > > 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() || ...`
> > Rafa. I believe this comment by johannes has not been solved. The the if statement before the assignment. It can also be added to this line. 
> I think what he meant was to check if the AA was a valid state before reading 
> ReachedKnownParallelRegions and 
> ReachedUnknownParallelRegions. I can also add it to the same line like this:
> 
> ```
> NestedParallelism |= (!FnAA.ReachedKnownParallelRegions.empty() || !FnAA.ReachedUnknownParallelRegions.empty()) && FnAA.getState().isValidState()
> ```
> 
> Johannes, please correct me if I’m wrong. 
You got the default wrong. If the state of FnAA is invalid we need to assume the worst. Right now you ignore invalid AAs, which is bad. So if Invalid, or if the other condition, then we assume nested parallelism.


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

https://reviews.llvm.org/D141010