[flang] [OpenMP 4.5] Add semantic checks for OpenMP Private clause

[praveen]

Summary

Add the semantic checks for the OpenMP 4.5 - 2.15.3.3 Private clause.

• Pointers with the INTENT(IN) attribute may not appear in a private clause.

• Variables that appear in namelist statements may not appear in a private clause. (This check holds for firstprivate and lastprivate clauses too)

Test cases : omp-private01.f90 , omp-private02.f90 , omp-private03.f90, omp-private04.f90

[praveen]

Updated formatting

[sameeranjoshi]

flang/lib/Semantics/check-omp-structure.cpp

413–431

Is it possible to extract this into a function and make it more generalized ?
I see copyprivate in OMP 4.5 standard restriction section point 10 in [2.15.4.2] too has a similar restriction, we can probably reuse it over there.

[praveen]

Moved the pointer check to separate function as per review comment

[kiranchandramohan]

Thanks for this patch. A few comments inline.

flang/lib/Semantics/check-omp-structure.h

165–166

Can this be private?

flang/lib/Semantics/resolve-directives.cpp

1087

why is this check required?

1092

Can you check what is happening for this testcase here or above and why the error is being missed?

program omp_target
  integer :: a, b, c
  namelist /mylist/ a, b, c

  a = 5
  b = 10
  call sb()
contains
  subroutine sb()
  !$omp parallel private(a)
  c = a+b
  !$omp end parallel

  write(*, mylist)
  end subroutine
end program omp_target

1094

Nit: Use braced initializer here.

[kiranchandramohan]

ping.

[praveen]

Modified the logic to identify all the variables in namelist statements

[praveen]

Rebase with latest master

[kiranchandramohan]

flang/lib/Semantics/resolve-directives.cpp

948

"Variables that appear in namelist statements, in variable format expressions, and in expressions for statement function definitions, may not appear in a private clause."

The specific restriction that this code checks seems to have two more components. Will the checks for "in variable format expressions, and in expressions for statement function definitions," be in a separate patch?

1102

Can this error message be like, Variable in namelist %s cannot be in a ^s clause?

flang/test/Semantics/omp-private01.f90

22

Is this check required given that dummy arguments can never be part of a common block?

flang/test/Semantics/omp-private02.f90

2

Can we move omp-private03.f90, omp-private04.f90also into this file?

[praveen]

Address review comments.

[kiranchandramohan]

@praveen can you reply inline in each comment whether you have made the changes suggested, reply to the question asked, or give your opinion? That way it will be easier for me to review.

[praveen]

@kiranchandramohan Updated the reply for each of the inline comments.
Thanks!

flang/lib/Semantics/check-omp-structure.cpp

413–431

Moved the code to a function as suggested . Thanks !

flang/lib/Semantics/resolve-directives.cpp

948

@kiranchandramohan The checks for "Variable format expressions and in expressions for statement function definitions" will be handled in a separate patch.

1087

Removed the redundant check.

1092

@kiranchandramohan Thanks for pointing this out . Since only the namelist variables in the enclosing scope are considered , this error is getting missed out.

Working on modifying the code to get this working for all possible cases.

1092

@kiranchandramohan Modified the logic to identify all the namelist variables defined in different scopes and added the test cases for the same.

Thanks!

1094

Made the change as suggested.

1102

Modified the error message. Thanks!

flang/test/Semantics/omp-private01.f90

22

Removed the redundant check as suggested. Thanks!

flang/test/Semantics/omp-private02.f90

2

Moved the test cases in omp-private03.f90 and omp-private04.f90 to omp-private02.f90

[kiranchandramohan]

flang/lib/Semantics/resolve-directives.cpp

1087–1091

Calling GetNamelistSymbols for finding all the namelist objects each time CheckObjectInNameList (for each variable in the private list) is called seems to be not optimal.

@tskeith Is there a better way to check whether a variable is part of a namelist?

[kiranchandramohan]

flang/lib/Semantics/resolve-directives.cpp

1087–1091

I checked with others, what Peter suggested is to collect all the namelist groups' symbols once and then use it for the private clause checks. Can you try that?

[tskeith]

flang/lib/Semantics/resolve-directives.cpp

1087–1091

Another option would be to add an InNamelist flag to Symbol::Flags, similar to InDataStmt. It's probably easy to set that in resolve-names.cpp.

[praveen]

Added a flag in symbol.h to identify the symbols in namelist

[kiranchandramohan]

LGTM. Thanks @praveen.

flang/lib/Semantics/resolve-directives.cpp

1087–1091

Thanks @tskeith