This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by praveen on Oct 26 2020, 10:59 PM.

Details

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

Diff Detail

Event Timeline

praveen created this revision.Oct 26 2020, 10:59 PM
praveen requested review of this revision.Oct 26 2020, 10:59 PM
praveen updated this revision to Diff 300904.Oct 26 2020, 11:37 PM

Updated formatting

sameeranjoshi added inline comments.
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 updated this revision to Diff 301244.Oct 28 2020, 4:55 AM

Moved the pointer check to separate function as per review comment

kiranchandramohan requested changes to this revision.Oct 29 2020, 4:46 PM

Thanks for this patch. A few comments inline.

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

Can this be private?

flang/lib/Semantics/resolve-directives.cpp
1197

why is this check required?

1202

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
1204

Nit: Use braced initializer here.

This revision now requires changes to proceed.Oct 29 2020, 4:46 PM
praveen updated this revision to Diff 305876.Nov 17 2020, 12:18 PM
praveen marked an inline comment as done.

Modified the logic to identify all the variables in namelist statements

praveen updated this revision to Diff 307009.Nov 23 2020, 2:22 AM
praveen marked 3 inline comments as done.
praveen edited the summary of this revision. (Show Details)

Rebase with latest master

kiranchandramohan requested changes to this revision.Nov 23 2020, 6:24 AM
kiranchandramohan added inline comments.
flang/lib/Semantics/resolve-directives.cpp
1054

"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?

1228

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

flang/test/Semantics/omp-private01.f90
21

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

flang/test/Semantics/omp-private02.f90
1

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

This revision now requires changes to proceed.Nov 23 2020, 6:24 AM
praveen updated this revision to Diff 307121.Nov 23 2020, 10:50 AM

Address review comments.

@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 marked 4 inline comments as done.Nov 23 2020, 10:14 PM

@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
1054

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

1197

Removed the redundant check.

1202

@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.

1202

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

Thanks!

1204

Made the change as suggested.

1228

Modified the error message. Thanks!

flang/test/Semantics/omp-private01.f90
21

Removed the redundant check as suggested. Thanks!

flang/test/Semantics/omp-private02.f90
1

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

kiranchandramohan added inline comments.
flang/lib/Semantics/resolve-directives.cpp
1213–1217

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?

flang/lib/Semantics/resolve-directives.cpp
1213–1217

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 added inline comments.Nov 25 2020, 4:36 PM
flang/lib/Semantics/resolve-directives.cpp
1213–1217

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 updated this revision to Diff 307784.Nov 26 2020, 1:17 AM
praveen marked 4 inline comments as done.

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

LGTM. Thanks @praveen.

flang/lib/Semantics/resolve-directives.cpp
1213–1217

Thanks @tskeith

This revision is now accepted and ready to land.Nov 30 2020, 6:08 AM
This revision was automatically updated to reflect the committed changes.
praveen marked 3 inline comments as done.