Page MenuHomePhabricator

[Flang] [OpenMP] Add semantic checks for OpenMP firstprivate , lastprivate and copyprivate clauses
Needs ReviewPublic

Authored by praveen on Sat, Nov 21, 11:19 AM.

Details

Summary

Add semantic checks and the corresponding lit test cases for the following OpenMP 4.5 clauses

  • 2.15.4.2 - Copyprivate clause
  • 2.15.3.4 - Firstprivate clause
  • 2.15.3.5 - Lastprivate clause

Diff Detail

Event Timeline

praveen created this revision.Sat, Nov 21, 11:19 AM
praveen requested review of this revision.Sat, Nov 21, 11:19 AM

Flang.Semantics::omp-clause-validity01.f90

Script:

: 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/flang/test/Semantics/test_errors.sh /mnt/disks/ssd0/agent/llvm-project/flang/test/Semantics/omp-clause-validity01.f90 /mnt/disks/ssd0/agent/llvm- project/build/tools/flang/test/Semantics/Output/omp-clause-validity01.f90.tmp /mnt/disks/ssd0/agent/llvm-project/build/bin/f18 -intrinsic-module-directory /mnt/disks/ssd0/agent/llvm-project/build/tools/flang/include/flang -fopenmp

Can you check why a unit test is failing?

@kiranchandramohan In the test file omp-clause-validity01.f90 , the following no symbol found errors are not being thrown.

!ERROR: Internal: no symbol found for 'i'
!$omp taskwait depend(sink:i-1)

!ERROR: Internal: no symbol found for 'first'
!$omp critical (first)

Since the errors related to copyprivate clause in omp-clause-validity01.f90 (line numbers 324 and 340 above) being thrown as part of the checks for copyprivate in resolve-directives.cpp are marked as fatal errors , the flag errorOnUnresolvedName_ is being set to false and the no symbol found error is not being thrown.

 RewriteMutator(SemanticsContext &context)
 : errorOnUnresolvedName_{!context.AnyFatalError()},
    messages_{context.messages()} {}

void RewriteMutator::Post(parser::Name &name) {
   if (!name.symbol && errorOnUnresolvedName_) {
     messages_.Say(name.source, "Internal: no symbol found for '%s'"_err_en_US,
       name.source);
    }
 }

Is it not necessary to throw the "no symbol found" error if there is any error marked as fatal while resolving the directives ?

Should all the checks related to copyprivate be moved to check-omp-structure.cpp?

Can you please suggest the approach to follow for these changes?

Thanks!

@kiranchandramohan In the test file omp-clause-validity01.f90 , the following no symbol found errors are not being thrown.

!ERROR: Internal: no symbol found for 'i'
!$omp taskwait depend(sink:i-1)

!ERROR: Internal: no symbol found for 'first'
!$omp critical (first)

Since the errors related to copyprivate clause in omp-clause-validity01.f90 (line numbers 324 and 340 above) being thrown as part of the checks for copyprivate in resolve-directives.cpp are marked as fatal errors , the flag errorOnUnresolvedName_ is being set to false and the no symbol found error is not being thrown.

 RewriteMutator(SemanticsContext &context)
 : errorOnUnresolvedName_{!context.AnyFatalError()},
    messages_{context.messages()} {}

void RewriteMutator::Post(parser::Name &name) {
   if (!name.symbol && errorOnUnresolvedName_) {
     messages_.Say(name.source, "Internal: no symbol found for '%s'"_err_en_US,
       name.source);
    }
 }

Is it not necessary to throw the "no symbol found" error if there is any error marked as fatal while resolving the directives ?

Should all the checks related to copyprivate be moved to check-omp-structure.cpp?

Can you please suggest the approach to follow for these changes?

Thanks!

I think these checks can stay in the current file since they need to check the data sharing attributes.
For the depend clause I think the test need to be changed to a valid test. Is that OK @kiranktp.
For critical I believe we may not have done the semantic check and hence the symbol is not available. For this I think it is OK to comment out the check.

Is it not necessary to throw the "no symbol found" error if there is any error marked as fatal while resolving the directives ?

Is this OK @clementval ?

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

Is this function part of this patch? If not can you either submit the other patch which introduces this code or make this patch a child of the other patch?

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

Will this test erroneously include DSA other than private, firstprivate?

1274

Should there be a check for the SINGLE construct here?

flang/test/Semantics/omp-firstprivate01.f90
2

Can similar tests be all put in the same file? Like all tests which test for 2.15.3.4.

flang/test/Semantics/omp-lastprivate01.f90
5

Can tests be added for the following cases. If it is not covered elsewhere?
-> module variable which is protected.
-> subroutine argument which is intent(in)

flang/test/Semantics/omp-lastprivate02.f90
2

Combine this test and the next one if they are testing the same thing for different worksharing constructs.
Add a test for single as well if possible.

praveen updated this revision to Diff 308987.Wed, Dec 2, 9:14 AM

Addressed review comments and combined the test cases

praveen marked 6 inline comments as done.Wed, Dec 2, 9:22 AM

@kiranchandramohan In the test file omp-clause-validity01.f90 , the following no symbol found errors are not being thrown.

!ERROR: Internal: no symbol found for 'i'
!$omp taskwait depend(sink:i-1)

!ERROR: Internal: no symbol found for 'first'
!$omp critical (first)

Since the errors related to copyprivate clause in omp-clause-validity01.f90 (line numbers 324 and 340 above) being thrown as part of the checks for copyprivate in resolve-directives.cpp are marked as fatal errors , the flag errorOnUnresolvedName_ is being set to false and the no symbol found error is not being thrown.

 RewriteMutator(SemanticsContext &context)
 : errorOnUnresolvedName_{!context.AnyFatalError()},
    messages_{context.messages()} {}

void RewriteMutator::Post(parser::Name &name) {
   if (!name.symbol && errorOnUnresolvedName_) {
     messages_.Say(name.source, "Internal: no symbol found for '%s'"_err_en_US,
       name.source);
    }
 }

Is it not necessary to throw the "no symbol found" error if there is any error marked as fatal while resolving the directives ?

Should all the checks related to copyprivate be moved to check-omp-structure.cpp?

Can you please suggest the approach to follow for these changes?

Thanks!

I think these checks can stay in the current file since they need to check the data sharing attributes.
For the depend clause I think the test need to be changed to a valid test. Is that OK @kiranktp.
For critical I believe we may not have done the semantic check and hence the symbol is not available. For this I think it is OK to comment out the check.

Is it not necessary to throw the "no symbol found" error if there is any error marked as fatal while resolving the directives ?

Is this OK @clementval ?

@kiranchandramohan @kiranktp @clementval I have commented the test cases which were failing with "no symbol found" error.
The symbol in the sink of the depend clause (OmpDependSinkVec) is not resolved . Hence we are running into "no symbol found" error . Do we resolve it as part of a separate patch or can I include it as part of this patch?

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

@kiranchandramohan This function was originally introduced in the patch for private clause https://reviews.llvm.org/D90210 .

Since it is not yet checked in and the same check is required for copyprivate clause, i have included this as part of this patch as well.

770

This check is now merged to master .

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

@kiranchandramohan Thanks for pointing this out . Added the checks for private and firstprivate.

1274

@kiranchandramohan Added the SINGLE construct check . Thanks

flang/test/Semantics/omp-firstprivate01.f90
2

@kiranchandramohan Combined all the similar test cases into a single file.

flang/test/Semantics/omp-lastprivate01.f90
5

@kiranchandramohan Added test cases for the module variable which is protected and subroutine argument which is intent(in).

flang/test/Semantics/omp-lastprivate02.f90
2

@kiranchandramohan Combined the test cases as suggested .
Lastprivate clause is not allowed on the single clause.