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

[Praveen G via Phabricator via llvm-commits]

praveen added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:294
+  parser::CharBlock src{"cycle"};
+  if (collapseLevel) {
+    if (const auto &loopConstruct{
----------------
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
    ^^^



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92732