[PATCH] D99888: [flang][OpenMP] Add semantic checks for occurrence of nested Barrier regions

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:462
+    if (CurrentDirectiveIsNested() &&
+        llvm::omp::nestedBarrierErrSet.test(GetContextParent().directive)) {
+      context_.Say(parser::FindSourceLocation(x),
----------------
arnamoy10 wrote:
> kiranchandramohan wrote:
> > arnamoy10 wrote:
> > > kiranchandramohan wrote:
> > > > arnamoy10 wrote:
> > > > > kiranchandramohan wrote:
> > > > > > arnamoy10 wrote:
> > > > > > > kiranchandramohan wrote:
> > > > > > > > Definition of closely nested region is 
> > > > > > > > "A region nested inside another region with no parallel region nested between them."
> > > > > > > > 
> > > > > > > > Does this check capture this definition correctly?
> > > > > > > Thanks for pointing in out.
> > > > > > > 
> > > > > > > 1.  Just double checking -- is //parallel region// a region enclosed by the `!$omp parallel` construct?
> > > > > > > 2. IIUC, [[ https://github.com/llvm/llvm-project/blob/0116d04d04f20e9ae62ba847075840c3cb298080/flang/lib/Semantics/check-omp-structure.cpp#L130 | this ]] check (by which my code was inspired) also does not cover the definition of "close nesting"?  Please confirm.
> > > > > > > 
> > > > > > 1. Yes. But it is a dynamic concept, i.e it is the body of code executed during the execution.
> > > > > > 2. Yes that code also does not cover the definition. Will appreciate a fix.
> > > > > I need one more clarification before implementing the fix.
> > > > > 
> > > > > From the definition:
> > > > > 
> > > > > ```A region nested inside another region with no parallel region nested between them```
> > > > > 
> > > > > What does nesting "between" mean?  I can think of two cases.
> > > > > 
> > > > > Case 1:
> > > > > 
> > > > > ```
> > > > > !$omp ordered
> > > > >   !$omp parallel <-- Child of ordered
> > > > >     !$omp barrier <-- Child of parallel, grandchild of ordered
> > > > > ```
> > > > > 
> > > > > Case 2:
> > > > > 
> > > > > ```
> > > > > !$omp ordered
> > > > >   !$omp parallel <-- Child of ordered
> > > > >   !$omp barrier <-- Child of ordered
> > > > > ```
> > > > > 
> > > > > Can you comment on which one of the above two cases has a `barrier` closely nested inside the `ordered` construct?
> > > > > 
> > > > Case 2 has a barrier closely nested inside ordered. 
> > > Thanks.
> > > 
> > > So IIUC, basically if your //immediate// parent (say region1) is **not** a `!$omp parallel`, then you are closely nested inside region1.
> > > 
> > > If that is the case, IMO we do not need a fix.  Because `GetContextParent()` gives us our immediate parent.
> > Question is can there be a situation like,
> > 
> > 
> > ```
> > construct 1
> >    construct 2
> >         barrier
> > ```
> > 
> > 
> > The current check is only for barrier and construct 2 and you might conclude that there is no error. If construct 2 is not parallel and barrier and construct 1 is not allowed to be nested then you will miss that error, isn't it?
> I see that makes sense.
> 
> I think I can restrict the check until the grandparent in the construct stack?  For example, I do not need to flag the following case:
> 
> ```
> not allowed construct 1
>   allowed construct 2
>     allowed construct 3
>        barrier
> ```
> 
> Or do I?
Why do you think so?

You might be right for real world benchmarks, since many constructs don't allow other constructs to nest. But theoretically you should.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99888