[PATCH] D93205: [Flang][OpenMP 4.5] Add semantic check for OpenMP Do Loop Constructs for single directive

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Looks mostly OK. Just one issue (FindEnclosingDirContext) to take a decision.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:153
+  case llvm::omp::OMPD_single:
+    for (const auto &dirContext : dirContext_) {
+      if (dirContext.directive == llvm::omp::Directive::OMPD_do) {
----------------
yhegde wrote:
> kiranchandramohan wrote:
> > yhegde wrote:
> > > kiranchandramohan wrote:
> > > > yhegde wrote:
> > > > > kiranchandramohan wrote:
> > > > > > yhegde wrote:
> > > > > > > clementval wrote:
> > > > > > > > `dirContext_` act as a stack so do you really intent to go from the bottom of the stack to the top? If so no problem. Just asking. 
> > > > > > > Yes. Check on nesting is called only if Do directive present in that stack. 
> > > > > > Should this call to HasInvalidWorksharingNesting be similar to the one in OpenMPLoopDirective and happen before pushing the context and not iterate over the entire stack of contexts?
> > > > > In OpenMPLoopDirective OMPD_Do context is pushed after calling HasInvalidWorksharingNesting. So I could not do this check in a similar way. 
> > > > Is there any issue with pushing the context after calling HasInvalidWorksharingNesting here also?
> > > I suppose you mean pushing OMPD_Do context after calling HasInvalidWorksharingNesting ? . If so , then it is not required to push that context later.  Control reaches OpenMPLoopDirective first and then OMPD_single. 
> > I was asking whether it is OK to implement it the following way so that we don't have to use FindInEnclosingDirContext function?
> > 
> > ```
> >   if (beginDir.v == llvm::omp::Directive::OMPD_single) {
> >     HasInvalidWorksharingNesting(...);
> >   }
> > 
> >   PushContextAndClauseSets(beginDir.source, beginDir.v);
> > 
> >   switch (beginDir.v) {
> >   case llvm::omp::OMPD_parallel:
> >     CheckNoBranching(block, llvm::omp::OMPD_parallel, beginDir.source);
> >     break;
> >   default:
> >     break;
> >   }
> > }
> > ```
> FindEnclosingDirContext function can check any restrictive directives for "single" , like "critical" or "task"  - if in case it is required to check .  Otherwise I can call  
> 
> if (beginDir.v == llvm::omp::Directive::OMPD_single) {
>     HasInvalidWorksharingNesting(HasInvalidWorksharingNesting(
>           beginDir.source, {llvm::omp::Directive::OMPD_do}););
>   }
> 
> before pushing the  PushContextAndClauseSets(beginDir.source, beginDir.v) and get the expected error.
> 
If you want to check for restrictions besides OMPD_do, then you can add them to the set like the following isn't it?
```
if (beginDir.v == llvm::omp::Directive::OMPD_single) {
  HasInvalidWorksharingNesting(
       beginDir.source{llvm::omp::Directive::OMPD_do,
                                   llvm::omp::Directive::OMPD_critical,
                                   llvm::omp::Directive::OMPD_task});
}
```

Anyway the FindEnclosingDirContext is probably not needed for this patch. If we need it later then we can introduce it there. If you agree please remove FindEnclosingDirContext.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93205