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

[Valentin Clement via Phabricator via llvm-commits]

clementval marked 2 inline comments as done.
clementval added inline comments.


================
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;
----------------
sameeranjoshi wrote:
> If that's a restriction why is this not in the form of a semantic error message?
The error message is issued in CheckDoConcurrentClauseRestriction in case there is a tile clause associated. Here we just bypass the logic and we do not issue the message a second time. 


================
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
----------------
sameeranjoshi wrote:
> Which standard ? 
> There are patches for 3.1 up gradation, will line numbers change in that case? 
> Is 3.0 assumed when no reference?
>From no one all reference are from 3.1. Right now it's like a transition phase until patch D92120 is approved and landed. 


================
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)) {
----------------
sameeranjoshi wrote:
> A C++ beginners query -
> Why use `std::get_if` over `std::holds_alternative` if we are not utilizing the return pointer? 
`std::holds_alternative` would be fine in this case. I'll update the patch. Thx.


================
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);
----------------
sameeranjoshi wrote:
> `nit`:Was that `or` instead of `and` ?
I just use the text from the standard. `or` would work as well. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92533