[PATCH] D92741: [flang][openacc] Enforce delcare directive restriction

[sameeran joshi via Phabricator via llvm-commits]

sameeranjoshi added a comment.

Thanks for the patch.
A few comments below.

I came to know quite a few new things from Flang code base.

- How to check scoping.
- Given an ObjectList or similar representation how to extract symbol and use interfaces from `tools.h`.



================
Comment at: flang/lib/Semantics/resolve-directives.cpp:117-120
-  bool Pre(const parser::SpecificationPart &x) {
-    Walk(std::get<std::list<parser::OpenACCDeclarativeConstruct>>(x.t));
-    return false;
-  }
----------------
Trying to understand- not a blocking review comment.
The `Walk` should be handled in some tree-visitor files and not here, is my understanding correct?


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:467
+  } else if (const auto *copyoutClause =
+                 std::get_if<Fortran::parser::AccClause::Copyin>(&clause.u)) {
+    const Fortran::parser::AccObjectListWithModifier &listWithModifier =
----------------
Possibly a bug.
Is this `Copyout` ?


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:487
+    return linkClause->v;
+  } else {
+    llvm_unreachable("Clause without object list!");
----------------
I see in my OACC 3.1 copy `create` clause as one of them in the list.
Is this missing to add over here with a test below ?


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:496
+  for (const auto &clause : clauseList.v) {
+    DoNotAllowAssumedSizedArray(GetAccObjectList(clause));
+  }
----------------
Do you need somewhere the index into standard for this?
`2414` from 3.1 spec.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:497
+    DoNotAllowAssumedSizedArray(GetAccObjectList(clause));
+  }
+}
----------------
Are checks from line `2410-2411` and `2412-2413` from OACC 3.1 missing or are handled somewhere else in previous patches ?


================
Comment at: flang/test/Semantics/acc-declare-validity.f90:53-55
+!deviceptr( var-list )
+!device_resident( var-list )
+!link( var-list )
----------------
Any plans to add these ?
Until then can you add a `TODO` or remove them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92741