[PATCH] D89860: [Flang][OpenMP 4.5] Add semantic check for OpenMP ordered and collapse clause

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

A few questions. Otherwise LGTM.



================
Comment at: flang/lib/Semantics/resolve-directives.cpp:787
 
 std::int64_t OmpAttributeVisitor::GetAssociatedLoopLevelFromClauses(
     const parser::OmpClauseList &x) {
----------------
Can this function return a pair of std::int64_t parser::OmpClause *?


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:811
   if (orderedLevel && (!collapseLevel || orderedLevel >= collapseLevel)) {
+    SetAssociatedClause(*ordClause);
     return orderedLevel;
----------------
Nit: Can this function return a pair? Question because the name of the function is Get*.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:860
   }
-  CHECK(level == 0);
+  // CHECK(level == 0);
+  if (const auto *clause{GetAssociatedClause()}) {
----------------
You can remove this commented code.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:861-868
+  if (const auto *clause{GetAssociatedClause()}) {
+    if (level != 0) {
+      context_.Say(clause->source,
+          "The value of the parameter in the collapse or ordered clause must"
+          " not be larger than the number of nested loops"
+          " following the construct."_err_en_US);
+    }
----------------
Nit: This function's name (PrivatizeAssociatedLoopIndex) does not suggest that this kind of check happens here. Is it possible to pull this out or do it at a different place? Any strong reasons why it should be here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89860