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

[Yashaswini Hegde via Phabricator via llvm-commits]

yhegde added inline comments.


================
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) {
----------------
kiranchandramohan wrote:
> yhegde wrote:
> > kiranchandramohan wrote:
> > > yhegde wrote:
> > > > kiranchandramohan wrote:
> > > > > 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.
> > > > ok. Then I suppose 'single'  binds to immediate outer directive context irrespective of that context. I have moved code before pushing the context. Thanks for the review comments.  
> > > Can you point me to this restriction for single in the standard?
> > I was under an impression that FindInEnclosingDircontext check is required in one such following case: 
> > 
> > program omp_do
> >   integer a(10),b(10),c(10)
> > !$omp   parallel sections
> > !$omp     section
> > !$omp       critical
> > !$omp do 
> >   do i=1,10
> > !$omp         parallel
> >     a(i) = b(i) + c(i)
> > !$omp           single
> >      j = j + 1
> > !$omp           end single
> > !$omp         end parallel
> >  end do
> > !$omp       end critical
> > !$omp   end parallel sections
> > 
> > But may be it is enough to check immediate outer dir context. So I have updated the diff with suggested changes. 
> To confirm whether it is a closely nested (construct/region) check or a general nested check, I would like to see which specific restriction you are implementing here from the standard. I could not find the single restriction in the worksharing loop section (2.7.1). Did I miss?
> 
> However, I see the following in Section 2.17
> • A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region
> 
> Please clarify which specific restriction you are implementing here.
The case related to 'single' - (omp-do05.f90)  is listed under 2.7.1  with restriction description  "All loops associated with the loop construct must be perfectly nested; that is, there must be no intervening code nor any OpenMP directive between any two loops.". 

The spec https://www.openmp.org/wp-content/uploads/openmp-examples-4.5.0.pdf , page 277 , gives a similar example calling it as "non-conforming example because the loop and single regions are closely nested.

gfortran throws an error - "work-sharing region may not be closely nested inside of work-sharing, ‘critical’, ‘ordered’, ‘master’, explicit ‘task’ or ‘taskloop’ region"  - for these cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93205