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

Tue Feb 2 09:35:07 PST 2021

praveen added inline comments.

================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:294
+  parser::CharBlock src{"cycle"};
+  if (collapseLevel) {
+    if (const auto &loopConstruct{
----------------
kiranchandramohan wrote:
> praveen wrote:
> > kiranchandramohan wrote:
> > > 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.
> > > @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?
> > > ```
> > @kiranchandramohan Yes, as you suggested , CheckNoBranching should be able to catch this error , but since we are not handling "**parser::CycleStmt**" in the NoBranchingEnforce class it is not being checked. 
> > 
> > Also **CheckNoBranching** is not being called for all the OpenMP structured blocks in the current state (It is checked as part of the patch for invalid branching checks)
> > 
> > 
> > The following is the output from handling the "parser::CycleStmt" in NoBranchingEnforce along with the changes in the patch https://reviews.llvm.org/D92735 for the below test case
> > 
> > 
> >   !$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
> > 
> >   error: CYCLE to construct 'foo' outside of DO construct is not allowed
> >              cycle foo
> >              ^^^^^^^^^
> >   ./d1.f90:13:9: Enclosing DO construct
> >     !$omp do
> >           ^^
> > 
> > 
> > For the cases such as this , two errors are identified for both the constructs
> > 
> >   foo: do i = 0, 10
> >   !$omp parallel
> >     !$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:12: error: CYCLE to construct 'foo' outside of PARALLEL construct is not allowed
> >              cycle foo
> >              ^^^^^^^^^
> >   ./omp-do-cycle.f90:11:9: Enclosing PARALLEL construct
> >     !$omp parallel
> >           ^^^^^^^^
> >   ./omp-do-cycle.f90:15:12: error: CYCLE to construct 'foo' outside of DO construct is not allowed
> >              cycle foo
> >              ^^^^^^^^^
> >   ./omp-do-cycle.f90:12:11: Enclosing DO construct
> >       !$omp do
> >             ^^
> > 
> > Please suggest if it is better to add the check as part of CheckNoBranching or in resolve-directives.cpp
> > 
> > 
> > > Since the source and destination of the branch are in the parallel region there should be no error for the parallel directive.
> > 
> > Okay will add a check for the same
> > 
> > 
> @praveen in my opinion handling "parser::CycleStmt" in NoBranchingEnforce and calling checkNoBranching is the right approach.
> 
> The above two errors match my expectation. If it matches yours also then please go ahead and make the change in https://reviews.llvm.org/D92735. If not then let me know the concern.
> 
> Thanks for your help here.
@kiranchandramohan @yhegde Thanks . I have updated the patch https://reviews.llvm.org/D92735 with the changes for parser::CycleStmt in NoBranchingEnforce to handle cycle statements leaving the OpenMP structured blocks

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92732