Page MenuHomePhabricator

[Flang][OpenMP 4.5] Add semantic check for OpenMP copyin clause
ClosedPublic

Authored by praveen on Oct 14 2020, 4:48 AM.

Details

Summary

Add semantic checks for the OpenMP 4.5 - 2.15.4.1 copyin clause.

Resolve OpenMPThreadprivate directive since the list of items specified in copyin clause should be threadprivate.

Test cases : omp-copyin01.f90 , omp-copyin02.f90 , omp-copyin03.f90 , omp-copyin04.f90 , omp-copyin05.f90

Diff Detail

Event Timeline

praveen created this revision.Oct 14 2020, 4:48 AM
praveen requested review of this revision.Oct 14 2020, 4:48 AM
praveen updated this revision to Diff 298124.Oct 14 2020, 5:12 AM

Updated formatting

Thanks @praveen for this patch. Can you add a few positive tests as well?

flang/lib/Semantics/resolve-directives.cpp
226–239

Why is this change required?

961

Nit: I think in general early returns are not used widely in the code base.

flang/test/Semantics/omp-copyin01.f90
12

Nit: a space before 'j'.

flang/test/Semantics/omp-copyin02.f90
20

Nit: A space between print and *.

praveen updated this revision to Diff 298351.Oct 15 2020, 4:53 AM
praveen edited the summary of this revision. (Show Details)

Added positive test cases , formatting as per review comments

SouraVX added inline comments.
flang/include/flang/Semantics/symbol.h
506

NIT: attribute ?

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

Please refer to comment in flang/lib/Semantics/resolve-directives.cpp:898 below

277

Not sure of this version specific comment, since we've not followed that consistently.
@kiranchandramohan @jdoerfert @kiranktp Please share your opinion.

882

It seems to me that you're resolving ThreadPrivate too ? (Since CopyIn requires ThreadPrivate in first place).
Could you please modify the revision summary to reflect the intent.

936

Can we have a better name to convey the intent ?

962

This seems to be un-intentional formatting introduced by your editor. Could you please correct this ?

986

NIT: Formatting ?

1108

NIT: How about re-phrasing this as:

List of items/objects that can appear in an `copyin` clause must be `threadprivate`.
praveen updated this revision to Diff 298979.Oct 19 2020, 3:21 AM
praveen marked 3 inline comments as done.
praveen edited the summary of this revision. (Show Details)
praveen set the repository for this revision to rG LLVM Github Monorepo.

Address the review comments and update the summary.

praveen marked 6 inline comments as done.Oct 19 2020, 3:27 AM

@kiranchandramohan @SouraVX Thanks for the review ! I have addressed the review comments as specified.

flang/lib/Semantics/resolve-directives.cpp
226–239

@kiranchandramohan This change is required to identify the list items present on the OpenMPThreadprivate directive as the semantic checks for the declarative directives are not yet implemented at present .

277

Removed the version specific comment.

882

@SouraVX Yes . I am resolving ThreadPrivate as it is required for Copyin . Updated the revision summary . Thanks!

961

Modified to if - else block instead of early return

flang/lib/Semantics/resolve-directives.cpp
226–239

I didn't check this in detail. By default, the walk should visit all kinds of nodes including Threadprivate and the code in Pre(threadprivate) processes the list of variables.

praveen updated this revision to Diff 301186.Tue, Oct 27, 11:33 PM
praveen marked 2 inline comments as done.

Removed redundant code used to resolve threadprivate directive as per review comments.

This revision is now accepted and ready to land.Mon, Nov 2, 2:44 PM