[PATCH] D92732: [Flang][OpenMP 4.5] Add semantic check for OpenMP Do Loop Constructs

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:294
+  parser::CharBlock src{"cycle"};
+  if (collapseLevel) {
+    if (const auto &loopConstruct{
----------------
praveen wrote:
> yhegde wrote:
> > kiranchandramohan wrote:
> > > yhegde wrote:
> > > > kiranchandramohan wrote:
> > > > > yhegde wrote:
> > > > > > kiranchandramohan wrote:
> > > > > > > Will checks only happen for loops with collapse clause? What about those without collapse? Should it be handled elsewhere?
> > > > > > > ```
> > > > > > >   !$omp parallel
> > > > > > >   foo: do i = 0, 10
> > > > > > >     !$omp do
> > > > > > >     bar: do j = 0, 10
> > > > > > >            cycle foo
> > > > > > >          end do bar
> > > > > > >     !$omp end do
> > > > > > >     !$omp end parallel
> > > > > > >   end do foo
> > > > > > > ```
> > > > > > Cases with OpenMP blocks in between are not handled . It requires different error message. But cases with out collapse - like the following case, works fine. Will add the test cases with out collapse. Thank you.  
> > > > > > program test
> > > > > > !$omp parallel
> > > > > > foo: do i = 0, 10
> > > > > >   !!$omp do
> > > > > >   bar: do j = 0, 10
> > > > > >          cycle foo
> > > > > >   end do bar
> > > > > >   !!$omp end do
> > > > > > end do foo
> > > > > > !$omp end parallel
> > > > > > end program test
> > > > > My concern was because the cyclechecker is entered only if the collapse level is greater than 0. If there is no collapse then i guess it will return 0 and hence the cyclechecker might not be called.
> > > > > ```
> > > > >   std::int64_t collapseLevel{GetCollapseLevel(x)};
> > > > >   if (collapseLevel) {
> > > > > ```
> > > > All negative test cases including those cases which have been suggested with parallel /parallel do collapse are all positive without collapse. means they give no error. please let me know if it is required to check for cycles in do loops with out collapse clause.  
> > > Is your point that it should be handled by https://reviews.llvm.org/D92735 and not this patch? If so, I am fine if @praveen can confirm.
> > Ok. Fine. I was under the impression that the cycles are the cause of error only with the collapse clause. I can add test cases with ordered clause also. 
> > Is your point that it should be handled by https://reviews.llvm.org/D92735 and not this patch? If so, I am fine if @praveen can confirm.
> 
> @kiranchandramohan The patch https://reviews.llvm.org/D92735 can be extended to check for cases such as the one below.
> 
>   !$omp parallel
>   foo: do i = 0, 10
>     !$omp do
>     bar: do j = 0, 10
>            cycle foo
>          end do bar
>     !$omp end do
>     !$omp end parallel
>   end do foo
> 
>   ./omp-do-cycle.f90:15:18: error: invalid branch into an OpenMP structured block
>              cycle foo
>                    ^^^
>   ./omp-do-cycle.f90:11:3: In the enclosing PARALLEL directive branched into
>       foo: do i = 0, 10
>       ^^^
>   ./omp-do-cycle.f90:15:18: error: invalid branch leaving an OpenMP structured block
>              cycle foo
>                    ^^^
>   ./omp-do-cycle.f90:11:3: Outside the enclosing DO directive
>     foo: do i = 0, 10
>     ^^^
> 
@praveen I was hopeful that CheckNoBranching will catch  the following error. Do you know why it does not catch in the current state? Is it just about calling CheckNoBranching in a few more places?
```
./omp-do-cycle.f90:15:18: error: invalid branch leaving an OpenMP structured block
           cycle foo
                 ^^^
./omp-do-cycle.f90:11:3: Outside the enclosing DO directive
  foo: do i = 0, 10
```

Since the source and destination of the branch are in the parallel region there should be no error for the parallel directive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92732