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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:857
+
+void OmpStructureChecker::CheckPrivateSymbolsInOuterCxt(
+    SymbolSourceMap &currSymbols, const OmpDirectiveSet &currDirSet,
----------------
praveen wrote:
> kiranchandramohan wrote:
> > Can you check why we cannot catch the error in the following test. gfortran seems able to.
> > ```
> > program mn
> > integer :: a(10), b(10), c(10)
> > !$omp parallel private(a,b)
> > call tmp()
> > !$omp end parallel
> > contains
> >   subroutine tmp()
> >   !$omp do firstprivate(b)
> >   do i = 1, 10
> >     c(i) = a(i) + b(i) + i
> >   end do
> >   !$omp end do
> >   end subroutine
> > end program
> > ```
> @kiranchandramohan We are not able to catch the error in the above case since the context information is cleared in the ResetPartialContext() after the OpenMP End construct is encountered. 
> 
> Hence, when checking the enclosing directives for firstprivate(b), it returns null. 
> I tried to keep track of the context information past the execution of the construct , but it ran into other issues for cases which expects the context information to be cleared. 
What did you finally do for this case? If it is not handled can you add an entry into the spreadsheet for this case?


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


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

https://reviews.llvm.org/D91920