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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.
This revision is now accepted and ready to land.

If you can add the autogen portion that will be great. It can be added either as part of this patch or a separate patch.  But it need not be a blocker for this. I am approving this patch.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1015
+        std::visit(common::visitors{
+                       [&](const parser::OmpClause::Private &x) {
+                         if (enclosingClauseSet.test(
----------------
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.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1015
+        std::visit(common::visitors{
+                       [&](const parser::OmpClause::Private &x) {
+                         if (enclosingClauseSet.test(
----------------
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.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:864
+  if (currDirSet.test(GetContext().directive)) {
+    if (auto *enclosingContext{GetEnclosingDirContext()}) {
+      if (enclosingDirSet.test(enclosingContext->directive)) {
----------------
praveen wrote:
> kiranchandramohan wrote:
> > praveen wrote:
> > > kiranchandramohan wrote:
> > > > praveen wrote:
> > > > > praveen wrote:
> > > > > > kiranchandramohan wrote:
> > > > > > > Is there an assumption here that the enclosing directive is the parallel/teams region?
> > > > > > > 
> > > > > > > Can it happen that the enclosing immediate directive is not the parallel/teams region to which the worksharing construct binds? Something like the following? 
> > > > > > > ```
> > > > > > > parallel private(x)
> > > > > > >   some_omp_construct
> > > > > > >     worksharing_construct firstprivate(x)
> > > > > > > ```
> > > > > > @kiranchandramohan I am not sure if there can be any immediate enclosing directives for workshare constructs . 
> > > > > > 
> > > > > > Since we already check 'HasInvalidWorksharingNesting' , do we need to check for the '//firstprivate/lastprivate variables that are private in enclosing context//' for the variables on the worksharing constructs?
> > > > > > 
> > > > > > The task construct can contain directives other than parallel such as the following.
> > > > > > 
> > > > > >   !$omp parallel reduction (+:a)
> > > > > >     !$omp single
> > > > > >       !$omp task firstprivate (a)
> > > > > >         a = a + b
> > > > > >       !$omp end task
> > > > > >     !$omp end single
> > > > > >   !$omp end parallel
> > > > > > 
> > > > > > Should we thrown an error for the cases such as the one above ?
> > > > > @kiranchandramohan Have updated the code to identify the enclosing constructs that are not immediate enclosing directives and binds to the current directive.
> > > > ```
> > > > A list item that appears in a reduction clause of a parallel construct must not appear in a
> > > > firstprivate clause on a worksharing, task, or taskloop construct if any of the
> > > > worksharing or task regions arising from the worksharing, task, or taskloop construct ever
> > > > bind to any of the parallel regions arising from the parallel construct.
> > > > ```
> > > > 
> > > > As per the above statement from the standard, yes it has to be flagged. If there are issues in handling this then please let me know.
> > > @kiranchandramohan I have updated the code to handle cases such as the above example where the binding region may  not be the immediate enclosing directive.
> > Could you point me to where and how this was handled?
> @kiranchandramohan The function **GetEnclosingContextWithDir** returns the matching directive in the dirContext and it is called in check-omp-structure.cpp:1011
> 
>   DirectiveContext *GetEnclosingContextWithDir(D dir) {
>     CHECK(!dirContext_.empty());
>     auto it{dirContext_.rbegin()};
>     while (++it != dirContext_.rend()) {
>       if (it->directive == dir) {
>         return &(*it);
>       }
>     }
>     return nullptr;
>   }
OK.


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

https://reviews.llvm.org/D91920