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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan 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) {
----------------
Is this variable used?


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


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:308-310
+            if (const auto *actionstmt{
+                    std::get_if<parser::Statement<parser::ActionStmt>>(
+                        &ec->u)}) {
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92732