[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:
> > 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?
> @kiranchandramohan I will add an entry to the spreadsheet for this case. 
> I guess we may have to explore to check if a separate parse-tree walk would help for such cases . 
> 
> I am not sure if I am missing something or gfortran (gcc 9.3.0) does not catch the above mentioned error as expected . 
> 
> firstprivate01.f90
> ```
> program mn
> integer :: a(10), b(10), c(10)
> !$omp parallel shared(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
> ```
> 
> firstprivate02.f90
> ```
> program mn
> integer :: a(10), b(10), c(10)
> !$omp parallel
> call tmp()
> !$omp end parallel
> contains
>   subroutine tmp()
>   !$omp do firstprivate(a)
>   do i = 1, 10
>     c(i) = a(i) + b(i) + i
>   end do
>   !$omp end do
>   end subroutine
> end program
> ```
> 
> firstprivate03.f90
> ```
> program mn
> integer :: a(10), b(10), c(10), i
> !$omp do firstprivate(a)
> do i = 1, 10
>   c(i) = a(i) + b(i) + i
> end do
> !$omp end do
> end program
> ```
> 
> In the above test cases firstprivate01.f90, firstprivate02.f90 and firstprivate03.f90 , the firstprivate symbol is not added as private in the enclosing context. 
> 
> But //**gfortran identifies the above cases as errors**//.  In gfortran , it seems that if the variable is not explicitly specified in any of the clauses , then it is assumed as **private** implicity.
> 
> Please let me know if it is the expected behaviour?
> 
> 
> nesting.f90
> ```
> program mn
> integer :: a(10), b(10), c(10)
> !$omp parallel
> !$omp single
> call tmp()
> !$omp end single
> !$omp end parallel
> contains
>   subroutine tmp()
>   !$omp do
>   do i = 1, 10
>     c(i) = a(i) + b(i) + i
>   end do
>   !$omp end do
>   end subroutine
> end program
> ```
> 
> In the above program nesting.f90, should there be an "//**invalid nesting of worksharing regions**//" error ?
> 
> 
> copyprivate.f90
> ```
> program mn
> integer :: a(10), b(10), c(10)
> !$omp parallel
> call tmp()
> !$omp end parallel
> contains
>   subroutine tmp()
>   !$omp single
>    c = a + b
>   !$omp end single copyprivate(a)
>   end subroutine
> end program
> ```
> 
> For the above case copyprivate.f90, we are identifying this as an error since "**//copyprivate variable is not private or thread-private in outer context//**" , but gfortran does not throw the error similar to the above cases.
Thanks @praveen for your questions. 
I believe we are talking about the following restrictions for,
1) firstprivate:  "A list item that is private within a parallel region must not appear in a firstprivate clause on a worksharing construct if any of the worksharing regions arising from the worksharing construct ever bind to any of the parallel regions arising from the parallel construct."

2) nesting: "A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region"

3) copyprivate: "All list items that appear in the copyprivate clause must be either threadprivate or private in the enclosing context"

I think gfortran is giving an incorrect messages here since for (1), (2), (3). 
But for (3) we must be careful since it talks about "enclosing context" and not regions. Enclosing context in Fortran is defined as the innermost scoping unit enclosing an OpenMP directive.


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

https://reviews.llvm.org/D91920