[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 a subscriber: sameeranjoshi.
kiranchandramohan added inline comments.
This revision now requires changes to proceed.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:120
+
+void OmpStructureChecker::CheckLoopItrVariableIsInt(
+    const parser::OpenMPLoopConstruct &x) {
----------------
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?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:154
+
+  if (!doWhileFlag && loopIv) {
+    if (const auto &loopConstruct{
----------------
Can we avoid having these global variables?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:205
+
+void OmpStructureChecker::CheckCycleConstraints(
+    const parser::OpenMPLoopConstruct &x) {
----------------
Are you handling cycle with the name of a loop?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:497
       if (auto *clause2{FindClause(llvm::omp::Clause::OMPC_safelen)}) {
+
         const auto &simdlenClause{
----------------
Nit: Is this new line needed?


================
Comment at: flang/lib/Semantics/check-omp-structure.h:192-194
+  bool doWhileFlag{false};
+  Symbol *loopIv{nullptr};
+  std::list<Symbol *> threadPrivateSymbols;
----------------
Can we keep the declarations at the top or at the bottom. Please refer to style elsewhere.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:800
+  if (!doWhileFlag) {
+    PrivatizeAssociatedLoopIndexAndCheckLoopLevel(x);
+  }
----------------
FYI @sameeranjoshi has a patch which does something similarhttps://reviews.llvm.org/D92638.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92732