[PATCH] D91920: [Flang] [OpenMP] Add semantic checks for OpenMP firstprivate , lastprivate and copyprivate clauses

[Praveen G via Phabricator via llvm-commits]

praveen marked 3 inline comments as done.
praveen added a comment.

@kiranchandramohan @sameeranjoshi Will the patch https://reviews.llvm.org/D93105 be committed to the master again? 
If so , the function **void GetSymbolsInDesignatorList(const std::list<parser::Designator> &, SymbolSourceMap &);** would not be required.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:687
+
+  if (currDirSet.test(GetContext().directive)) {
+    if (auto *enclosingContext{GetEnclosingDirContext()}) {
----------------
kiranchandramohan wrote:
> If this section is common code with firstprivate then sharing code would be good.
Moved the common code to separate function. Thanks!


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:690
+      if (outerDirSet.test(enclosingContext->directive)) {
+        if (const auto *clause{FindClauseInContext(
+                llvm::omp::Clause::OMPC_reduction, *enclosingContext)}) {
----------------
kiranchandramohan wrote:
> "A list item that is private within a parallel region, or that appears in the reduction clause of a parallel construct, must not appear in a lastprivate clause on a worksharing construct if any of the corresponding worksharing regions ever binds to any of the corresponding parallel regions."
> 
> It seems you are checking for reduction only here. From the standard (quoted above) it appears that we have to check for all kind of privates and reduction.
@kiranchandramohan Handled all the privates and reduction for firstprivate and lastprivate clauses. Thanks!


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:805
 
+void OmpStructureChecker::GetSymbolsInDesignatorList(
+    const std::list<parser::Designator> &designatorList,
----------------
kiranchandramohan wrote:
> kiranchandramohan wrote:
> > Can this function go to the base class?
> Could evaluate::CollectSymbols have been used instead of this since a Designator can also be an expression?
Moved this function to base class


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:805
 
+void OmpStructureChecker::GetSymbolsInDesignatorList(
+    const std::list<parser::Designator> &designatorList,
----------------
praveen wrote:
> kiranchandramohan wrote:
> > kiranchandramohan wrote:
> > > Can this function go to the base class?
> > Could evaluate::CollectSymbols have been used instead of this since a Designator can also be an expression?
> Moved this function to base class
@kiranchandramohan I wasnt able to pass the designator as an argument to evaluate::CollectSymbols() . Do we have to convert the designator to some form of expression so that it can be processed as an expression? 


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

https://reviews.llvm.org/D91920