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

Kiran Chandramohan via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 1 14:21:30 PST 2021


kiranchandramohan added a subscriber: praveen.
kiranchandramohan added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:113
+      context_.Say(*cyclesource_,
+          "CYCLE statement to non-innermost collapsed !$OMP DO loop"_err_en_US);
+    }
----------------
yhegde wrote:
> kiranchandramohan wrote:
> > yhegde wrote:
> > > kiranchandramohan wrote:
> > > > We usually do not add the "!$" in the error message.
> > >  That is gfortran error message. Kindly suggest a better one. Will look into the rest of the review comments. Thank you. 
> > Would the following work?
> > 
> > "CYCLE statement to non-innermost associated loop of an OpenMP DO construct"
> can that be "CYCLE statement to non-innermost associated loop of an OpenMP collapsed DO construct" ?
Associated loops can be due to both collapse as well as ordered clauses. Hence I would prefer that you not add "collapsed" to the message.


================
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:
> > > > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92732



More information about the llvm-commits mailing list