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

[Yashaswini Hegde via Phabricator via llvm-commits]

yhegde added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:293
+  std::map<std::string, std::int64_t> labelNamesandLevels;
+  parser::CharBlock src{"cycle"};
+  if (collapseLevel) {
----------------
kiranchandramohan wrote:
> Is this variable used?
Will remove that. :) 


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


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:308-310
+            if (const auto *actionstmt{
+                    std::get_if<parser::Statement<parser::ActionStmt>>(
+                        &ec->u)}) {
----------------
kiranchandramohan wrote:
> The concern with the approach is that you might have to call ompCycleBlockChecker for each individual construct. That is likely to be costly. Can parser::Walk happen on the entire block?
> 
> Do you have an estimate on how many additional visits will be there to a statement inside a Do loop due to this Walk? Will it be only 1 or more than that? If it is one additional visit it should be OK.
parser::Walk on entire block was throwing error twice in few cases. Hence I called it with in a construct. Will check that. 


================
Comment at: flang/test/Semantics/omp-do13.f90:1
+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
----------------
kiranchandramohan wrote:
> Please add a test for an openmp do loop with collapse nested inside an openmp do loop with collapse.
ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92732