[PATCH] D92533: [flang][openacc] Add missing loop construct restriction and validity tests

[sameeran joshi via Phabricator via llvm-commits]

sameeranjoshi added a comment.

The tests look good, just a few comments/queries?
I am really not familiar with canonicalization, but I could grasp what's defined in this patch.



================
Comment at: flang/lib/Semantics/canonicalize-acc.cpp:68
+        if (outer->IsDoConcurrent())
+          return; // Tile is not allowed on DO CONURRENT
         for (const parser::DoConstruct *loop{&*outer}; loop && tileArgNb > 0;
----------------
If that's a restriction why is this not in the form of a semantic error message?


================
Comment at: flang/lib/Semantics/canonicalize-acc.cpp:87
 
+  // Check constraint on line 1835 in Section 2.9
+  // A tile and collapse clause may not appear on loop that is associated with
----------------
Which standard ? 
There are patches for 3.1 up gradation, will line numbers change in that case? 
Is 3.0 assumed when no reference?


================
Comment at: flang/lib/Semantics/canonicalize-acc.cpp:99
+    for (const auto &clause : accClauseList.v) {
+      if (std::get_if<parser::AccClause::Collapse>(&clause.u) ||
+          std::get_if<parser::AccClause::Tile>(&clause.u)) {
----------------
A C++ beginners query -
Why use `std::get_if` over `std::holds_alternative` if we are not utilizing the return pointer? 


================
Comment at: flang/lib/Semantics/canonicalize-acc.cpp:102
+        messages_.Say(beginLoopDirective.source,
+            "TILE and COLLAPSE clause may not appear on loop construct "
+            "associated with DO CONCURRENT"_err_en_US);
----------------
`nit`:Was that `or` instead of `and` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92533