[PATCH] D91159: [flang]Add General Semantic Checks for Allocate Directive

[Valentin Clement via Phabricator via llvm-commits]

clementval requested changes to this revision.
clementval added inline comments.
This revision now requires changes to proceed.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:431-471
-void OmpStructureChecker::Enter(const parser::OmpClause::Priority &x) {
-  CheckAllowed(llvm::omp::Clause::OMPC_priority);
-  RequiresPositiveParameter(llvm::omp::Clause::OMPC_priority, x.v);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Private &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_private);
-}
----------------
kiranchandramohan wrote:
> Why are all these removed in this patch?
This would break any allowance check on these clauses. I don't see the reason to remove them in this patch. 


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:119
     Walk(std::get<std::list<parser::OpenACCDeclarativeConstruct>>(x.t));
-    return false;
+    return true;
   }
----------------
kiranchandramohan wrote:
> Why is this change in OpenACC required?
Please revert this change. OpenACC part should not be touch for OpenMP related code. 


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:772
+    PushContext(beginDir.source, beginDir.v);
+    inTarget = true;
+    break;
----------------
I guess you can query the current context instead of using this global variable. 


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1057
+void OmpAttributeVisitor::Post(const parser::OpenMPExecutableAllocate &x) {
+  if (inTarget && !hasAllocator)
+    // TODO: expand this check to exclude the case when a requires
----------------
I would remove those two globals and for the `inTarget` you can query the context stack. There is an example in OpenACC (https://github.com/llvm/llvm-project/blob/d02eac0c000984865dd1ce2474715538f8439470/flang/lib/Semantics/check-acc-structure.cpp#L67)

For the `hasAllocator` you can probably go through the list here to get the information. 


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1136
 
+Symbol *OmpAttributeVisitor::ResolveOmpObjectScope(const parser::Name *name) {
+
----------------
Can you reuse the `ResolveOmpName` function instead of this one? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91159