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

[Praveen G via Phabricator via llvm-commits]

praveen added a comment.

@kiranchandramohan The test case flang/test/Semantics/resolve102.f90 is failing with the error messages possibly due to the commit

https://github.com/llvm/llvm-project/commit/07de0846a5055015b55dc2b8faa2143f9902e549#diff-bc82e30d81709b26f2c8030300784e95003f1b968e1ed7842fa717858e497336

actual at 13: Procedure 'p' is recursively defined.  Procedures in the cycle: 'p2', 'sub', 'p'
expect at 13: Procedure 'p' is recursively defined.  Procedures in the cycle: 'p2', 'p', 'sub'
actual at 25: Procedure 'p' is recursively defined.  Procedures in the cycle: 'p2', 'sub', 'p'
expect at 25: Procedure 'p' is recursively defined.  Procedures in the cycle: 'p2', 'p', 'sub'

Can I push this patch now or only after the above failed case is fixed ?



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1015
+        std::visit(common::visitors{
+                       [&](const parser::OmpClause::Private &x) {
+                         if (enclosingClauseSet.test(
----------------
kiranchandramohan wrote:
> kiranchandramohan wrote:
> > clementval wrote:
> > > kiranchandramohan wrote:
> > > > praveen wrote:
> > > > > kiranchandramohan wrote:
> > > > > > Having to hard-code this looks unfortunate. Can you add a TODO to replace this hardcoding? Try one of the following,
> > > > > > 1) autogen checks
> > > > > > 2) or an autogenerated function which maps parser::OmpClause::<name> to llvm::omp::Clause::OMPC_<name>
> > > > > @kiranchandramohan Will try to add autogenerated function / autogen checks for the same
> > > > @clementval Is it currently possible to get the llvm::omp::Clause corresponding to a parser::OmpClause in a function? 
> > > > From the top of your head is there a different way to write this std::visit?
> > > We do not have such a function generated by TableGen right now. That would be a nice addition and useful in many places. 
> > Thanks valentine.
> OK @praveen, see whether you can add them but that is not a blocker for this patch.
@kiranchandramohan @clementval  Thanks . I have added a TODO to replace the hard-coded names as part of this patch. 

Will create a separate patch if it is possible to add an auto generated function to maintain the map.


================
Comment at: flang/lib/Semantics/check-omp-structure.h:85-87
+using SymbolSourceMap = std::multimap<const Symbol *, parser::CharBlock>;
+using DirectivesClauseTriple = std::multimap<llvm::omp::Directive,
+    std::pair<llvm::omp::Directive, const OmpClauseSet>>;
----------------
kiranchandramohan wrote:
> Can you add comments on why these Data Structures are necessary either here or where they are used?
Added the comments.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1502
           checkSymbol->name());
+  } else if (GetContext().directive == llvm::omp::Directive::OMPD_single) {
+    // A list item that appears in a 'copyprivate' clause may not appear on a
----------------
kiranchandramohan wrote:
> Nit: would it be better to add a check for  Symbol::Flag::OmpCopyPrivate here and then do the remaining checks inside?
> 
> Otherwise add a check or assert that it is CopyPrivate.
Added a check for Symbol::Flag::OmpCopyPrivate


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

https://reviews.llvm.org/D91920