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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

A few more questions.



================
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:
> > > 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?


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


================
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
----------------
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.


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

https://reviews.llvm.org/D91920