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

[Yashaswini Hegde via Phabricator via llvm-commits]

yhegde marked an inline comment as done.
yhegde added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:213
+        for (auto iit{blk.begin()}; iit != blk.end(); ++iit) {
+          if (const auto *ifstmt{parser::Unwrap<parser::IfStmt>(*iit)}) {
+            const auto &actionstmt{std::get<1>(ifstmt->t)};
----------------
kiranchandramohan wrote:
> yhegde wrote:
> > kiranchandramohan wrote:
> > > Is there an assumption here that if statement will be present when a cycle statement exists? Does the standard say that?
> > Cycles with out  ifstmts are also handled. 
> I think it might not work with else if, nested if etc.
> 
> Anyway, my point was that cycle statements can appear inside other fortran constructs, for e.g in a case statement like in the example below. So this portion of the code has to be generic and not limited to if statements.
> ```
> program test
>   integer i, j, k
>   !$omp do collapse(3)
>   foo: do i = 0, 10
>     bar: do j = 0, 10
>            do k = 0, 10
>              select case (k)
>                case (1)
>                  cycle foo
>                case (2)
>                  cycle bar
>                case default
>                  cycle
>              end select
>            end do
>          end do bar
>   end do foo
>   !$omp end do
> end program
> ```
Thanks for pointing out these issues. Can I have a class (OmpCycleBlockChecker) and parser::Walk to identify the cycles.?  I have tried that for existing test cases (0mp-do08.f90 and omp-do13.f90 negative cases. and omp-do12.f90 and omp-do14.f90 - positive cases). I can extend it to other cases if it is acceptable. 


================
Comment at: flang/lib/Semantics/check-omp-structure.h:229
+  std::vector<Symbol *> threadPrivateSymbols;
+  const parser::OmpClause::Ordered *ordClause{nullptr};
 };
----------------
kiranchandramohan wrote:
> Are you resetting this to null anywhere? Stale values might cause issues.
> 
> Instead of this method can you travel up the stack, find the do construct if any and check its clauses for the ordered clause?
Thanks for the review comments. CheckIfDoOrderedClause () is called before pushingthecontext and associated clauses of the OMPD_do context are searched for the ordered clause. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92732