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

[Praveen G via Phabricator via llvm-commits]

praveen marked 4 inline comments as done.
praveen added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:857
+
+void OmpStructureChecker::CheckPrivateSymbolsInOuterCxt(
+    SymbolSourceMap &currSymbols, const OmpDirectiveSet &currDirSet,
----------------
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.


================
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 ?
@kiranchandramohan Have updated the code to identify the enclosing constructs that are not immediate enclosing directives and binds to the current directive.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:864
+  if (currDirSet.test(GetContext().directive)) {
+    if (auto *enclosingContext{GetEnclosingDirContext()}) {
+      if (enclosingDirSet.test(enclosingContext->directive)) {
----------------
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.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:865
+    if (auto *enclosingContext{GetEnclosingDirContext()}) {
+      if (enclosingDirSet.test(enclosingContext->directive)) {
+        for (auto it{enclosingContext->clauseInfo.begin()};
----------------
praveen wrote:
> kiranchandramohan wrote:
> > I think whether this check should be performed is dependent on the triple enclosing_dir, enclosing_clause, cur_dir.
> > 
> > So there should be no error for the following code.
> > 
> > ```
> > program omp_firstprivate
> > 
> >   integer :: i, a(10), b(10), c(10)
> > 
> >   a = 10
> >   b = 20
> > 
> >   !$omp parallel private(a, b)
> >   !$omp task firstprivate(a)
> >   do i = 1, 10
> >     a(i) = a(i) + b(i) - i
> >   end do
> >   !$omp end task
> >   !$omp end parallel
> > end program
> > ```
> > 
> > But there should be an error for the following code.
> > ```
> > program omp_firstprivate
> >   integer :: i, a
> >   a = 10
> >   i = 2
> >   !$omp parallel reduction(+:a)
> >   !$omp task firstprivate(a)
> >     a = a + i
> >   !$omp end task
> >   !$omp end parallel
> > end program
> > ```
> @kiranchandramohan Thanks . Will handle this check 
Updated the logic to check based on the triple.


================
Comment at: flang/test/Semantics/omp-copyprivate03.f90:27
+
+  !$omp parallel sections private(k)
+  !$omp section
----------------
kiranchandramohan wrote:
> A threadprivate variable cannot appear in a private clause.
> A threadprivate variable cannot appear in a private clause.




================
Comment at: flang/test/Semantics/omp-copyprivate03.f90:27
+
+  !$omp parallel sections private(k)
+  !$omp section
----------------
praveen wrote:
> kiranchandramohan wrote:
> > A threadprivate variable cannot appear in a private clause.
> > A threadprivate variable cannot appear in a private clause.
> 
> 
Done


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

https://reviews.llvm.org/D91920