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

[Praveen G via Phabricator via llvm-commits]

praveen marked an inline comment 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:
> 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. 


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


================
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()};
----------------
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 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:805
 
+void OmpStructureChecker::GetSymbolsInDesignatorList(
+    const std::list<parser::Designator> &designatorList,
----------------
praveen wrote:
> praveen wrote:
> > kiranchandramohan wrote:
> > > kiranchandramohan wrote:
> > > > Can this function go to the base class?
> > > Could evaluate::CollectSymbols have been used instead of this since a Designator can also be an expression?
> > Moved this function to base class
> @kiranchandramohan I wasnt able to pass the designator as an argument to evaluate::CollectSymbols() . Do we have to convert the designator to some form of expression so that it can be processed as an expression? 
Removed the function as OmpObjectList is being used for Reduction clause instead of a list of Designators.


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

https://reviews.llvm.org/D91920