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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added inline comments.
This revision now requires changes to proceed.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:109
+            std::get<std::optional<parser::DoConstruct>>(x.t)}) {
+      if (doConstruct.value().IsDoWhile()) {
+        const auto &doStmt{std::get<parser::Statement<parser::NonLabelDoStmt>>(
----------------
If more than one loop is associated with the OpenMP do loop, should this check be performed on all the loops?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:147-148
+  std::int64_t collapseLevel{GetCollapseLevel(x)};
+  if (!collapseLevel)
+    collapseLevel++;
+
----------------
Why is collapse level used here but not in CheckLoopItrVariableIsInt?


================
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)};
----------------
Is there an assumption here that if statement will be present when a cycle statement exists? Does the standard say that?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:337
+
+  threadPrivateSymbols.clear();
+  const auto &list{std::get<parser::OmpObjectList>(x.t)};
----------------
We do symbol related checks in resolve-directives right? Is there any particular reason this code should be here?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:120
+
+void OmpStructureChecker::CheckLoopItrVariableIsInt(
+    const parser::OpenMPLoopConstruct &x) {
----------------
kiranchandramohan wrote:
> CheckDoControl and CheckDoVariable in flang/lib/Semantics/check-do-forall.cpp does the same check but are probably OFF or set as warning by default. Can the code in check-do-forall be reused?
Is the answer here NO?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:205
+
+void OmpStructureChecker::CheckCycleConstraints(
+    const parser::OpenMPLoopConstruct &x) {
----------------
kiranchandramohan wrote:
> Are you handling cycle with the name of a loop?
I probably did not make myself clear here. What i was suggesting was that it is not enough to look at where the cycle exists but also have to consider where the cycle branches to. Consider the testcase below.

```
program test
  integer i, j, k
  !$omp do  collapse(2)
  foo: do i = 0, 10
         do j = 0, 10
           do k  = 0, 10
             if (k .lt. 1) cycle foo
             print *, i, j, k
           end do
         end do
  end do foo
  !$omp end do
end program
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92732