Page MenuHomePhabricator

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

Authored by praveen on Nov 21 2020, 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

There are a very large number of changes, so older changes are hidden. Show Older Changes

Addressed review comments and combined the test cases

praveen marked 6 inline comments as done.Dec 2 2020, 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
1093

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

1093

This check is now merged to master .

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

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

1517

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

A few comments.

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

If this section is common code with firstprivate then sharing code would be good.

1064

"A list item that is private within a parallel region, or that appears in the reduction clause of a parallel construct, must not appear in a lastprivate clause on a worksharing construct if any of the corresponding worksharing regions ever binds to any of the corresponding parallel regions."

It seems you are checking for reduction only here. From the standard (quoted above) it appears that we have to check for all kind of privates and reduction.

1206

Can this function go to the base class?

kiranchandramohan requested changes to this revision.Dec 23 2020, 3:15 PM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
1206

Could evaluate::CollectSymbols have been used instead of this since a Designator can also be an expression?

This revision now requires changes to proceed.Dec 23 2020, 3:15 PM
praveen updated this revision to Diff 314084.Dec 30 2020, 12:07 AM
praveen marked 6 inline comments as done.

Handle all the private symbols in enclosing context for firstprivate and lastprivate clauses

praveen marked 3 inline comments as done.Dec 30 2020, 7:38 AM

@kiranchandramohan @sameeranjoshi Will the patch https://reviews.llvm.org/D93105 be committed to the master again?
If so , the function void GetSymbolsInDesignatorList(const std::list<parser::Designator> &, SymbolSourceMap &); would not be required.

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

Moved the common code to separate function. Thanks!

1064

@kiranchandramohan Handled all the privates and reduction for firstprivate and lastprivate clauses. Thanks!

1206

Moved this function to base class

1206

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

@kiranchandramohan @sameeranjoshi Will the patch https://reviews.llvm.org/D93105 be committed to the master again?
If so , the function void GetSymbolsInDesignatorList(const std::list<parser::Designator> &, SymbolSourceMap &); would not be required.

D93105 will be landed again. Waiting for @sameeranjoshi to do it.

praveen updated this revision to Diff 314543.Jan 5 2021, 2:32 AM
praveen marked 2 inline comments as done.

Update to latest syntax of Omp Reduction clause

kiranchandramohan requested changes to this revision.Jan 14 2021, 4:31 PM

I believe the checks have to be made more precise. Please have a look at the comments.

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

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
1151

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)
1152

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
flang/test/Semantics/omp-copyprivate03.f90
28

A threadprivate variable cannot appear in a private clause.

This revision now requires changes to proceed.Jan 14 2021, 4:31 PM
praveen marked an inline comment as done.Feb 1 2021, 10:50 AM
praveen added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
1144

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

1151

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

1152

@kiranchandramohan Thanks . Will handle this check

1206

Removed the function as OmpObjectList is being used for Reduction clause instead of a list of Designators.

praveen updated this revision to Diff 321809.Feb 5 2021, 10:21 AM

Address review comments and rebase

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

What did you finally do for this case? If it is not handled can you add an entry into the spreadsheet for this case?

1151
A list item that appears in a reduction clause of a parallel construct must not appear in a
firstprivate clause on a worksharing, task, or taskloop construct if any of the
worksharing or task regions arising from the worksharing, task, or taskloop construct ever
bind to any of the parallel regions arising from the parallel construct.

As per the above statement from the standard, yes it has to be flagged. If there are issues in handling this then please let me know.

praveen marked 4 inline comments as done.Feb 11 2021, 6:17 AM
praveen added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
1144

@kiranchandramohan I will add an entry to the spreadsheet for this case.
I guess we may have to explore to check if a separate parse-tree walk would help for such cases .

I am not sure if I am missing something or gfortran (gcc 9.3.0) does not catch the above mentioned error as expected .

firstprivate01.f90

program mn
integer :: a(10), b(10), c(10)
!$omp parallel shared(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

firstprivate02.f90

program mn
integer :: a(10), b(10), c(10)
!$omp parallel
call tmp()
!$omp end parallel
contains
  subroutine tmp()
  !$omp do firstprivate(a)
  do i = 1, 10
    c(i) = a(i) + b(i) + i
  end do
  !$omp end do
  end subroutine
end program

firstprivate03.f90

program mn
integer :: a(10), b(10), c(10), i
!$omp do firstprivate(a)
do i = 1, 10
  c(i) = a(i) + b(i) + i
end do
!$omp end do
end program

In the above test cases firstprivate01.f90, firstprivate02.f90 and firstprivate03.f90 , the firstprivate symbol is not added as private in the enclosing context.

But gfortran identifies the above cases as errors. In gfortran , it seems that if the variable is not explicitly specified in any of the clauses , then it is assumed as private implicity.

Please let me know if it is the expected behaviour?

nesting.f90

program mn
integer :: a(10), b(10), c(10)
!$omp parallel
!$omp single
call tmp()
!$omp end single
!$omp end parallel
contains
  subroutine tmp()
  !$omp do
  do i = 1, 10
    c(i) = a(i) + b(i) + i
  end do
  !$omp end do
  end subroutine
end program

In the above program nesting.f90, should there be an "invalid nesting of worksharing regions" error ?

copyprivate.f90

program mn
integer :: a(10), b(10), c(10)
!$omp parallel
call tmp()
!$omp end parallel
contains
  subroutine tmp()
  !$omp single
   c = a + b
  !$omp end single copyprivate(a)
  end subroutine
end program

For the above case copyprivate.f90, we are identifying this as an error since "copyprivate variable is not private or thread-private in outer context" , but gfortran does not throw the error similar to the above cases.

1151

@kiranchandramohan Have updated the code to identify the enclosing constructs that are not immediate enclosing directives and binds to the current directive.

1151

@kiranchandramohan I have updated the code to handle cases such as the above example where the binding region may not be the immediate enclosing directive.

1152

Updated the logic to check based on the triple.

flang/test/Semantics/omp-copyprivate03.f90
28

A threadprivate variable cannot appear in a private clause.

28

Done

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

Thanks @praveen for your questions.
I believe we are talking about the following restrictions for,

  1. firstprivate: "A list item that is private within a parallel region must not appear in a firstprivate clause on a worksharing construct if any of the worksharing regions arising from the worksharing construct ever bind to any of the parallel regions arising from the parallel construct."
  1. nesting: "A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region"
  1. copyprivate: "All list items that appear in the copyprivate clause must be either threadprivate or private in the enclosing context"

I think gfortran is giving an incorrect messages here since for (1), (2), (3).
But for (3) we must be careful since it talks about "enclosing context" and not regions. Enclosing context in Fortran is defined as the innermost scoping unit enclosing an OpenMP directive.

praveen updated this revision to Diff 323381.Feb 12 2021, 10:19 AM
praveen marked 2 inline comments as done.

Check threadprivate variables in symbols with HostAssocDetails

praveen marked 3 inline comments as done.Feb 12 2021, 10:22 AM
praveen added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
1144

@kiranchandramohan Thanks for the explanation . I have added an entry in the spreadsheet for the above cases as part of Nesting of Regions part.

For the restrictions on the copyprivate variables , we are checking in the enclosing scope only.

Quick comments/questions before I have a final look.

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

Nit: Add an empty line above.

1174–1181

Will this clash with the reduction work that @yhegde is doing in https://reviews.llvm.org/D90697?

flang/lib/Semantics/check-omp-structure.h
85–87

Can you add comments on why these Data Structures are necessary either here or where they are used?

A few more questions.

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

Could you point me to where and how this was handled?

1156

Having to hard-code this looks unfortunate. Can you add a TODO to replace this hardcoding? Try one of the following,

  1. autogen checks
  2. or an autogenerated function which maps parser::OmpClause::<name> to llvm::omp::Clause::OMPC_<name>
flang/lib/Semantics/resolve-directives.cpp
1511

Nit: would it be better to add a check for Symbol::Flag::OmpCopyPrivate here and then do the remaining checks inside?

Otherwise add a check or assert that it is CopyPrivate.

praveen marked 2 inline comments as done.Feb 24 2021, 10:02 AM
praveen added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
1151

@kiranchandramohan The function GetEnclosingContextWithDir returns the matching directive in the dirContext and it is called in check-omp-structure.cpp:1011

DirectiveContext *GetEnclosingContextWithDir(D dir) {
  CHECK(!dirContext_.empty());
  auto it{dirContext_.rbegin()};
  while (++it != dirContext_.rend()) {
    if (it->directive == dir) {
      return &(*it);
    }
  }
  return nullptr;
}
1156

@kiranchandramohan Will try to add autogenerated function / autogen checks for the same

1174–1181

@kiranchandramohan In this patch , only the variables in the firstprivate and lastprivate clauses on the worksharing constructs are being checked while in https://reviews.llvm.org/D90697 added by @yhegde , variables in the reduction clause is checked . So , I dont think there would be a clash regarding the checks.

But the checks for privates and reduction in the enclosing region is common for both.

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

@clementval Is it currently possible to get the llvm::omp::Clause corresponding to a parser::OmpClause in a function?
From the top of your head is there a different way to write this std::visit?

clementval added inline comments.Feb 25 2021, 9:00 AM
flang/lib/Semantics/check-omp-structure.cpp
1156

We do not have such a function generated by TableGen right now. That would be a nice addition and useful in many places.

If you can add the autogen portion that will be great. It can be added either as part of this patch or a separate patch. But it need not be a blocker for this. I am approving this patch.

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

OK.

1156

Thanks valentine.

1156

OK @praveen, see whether you can add them but that is not a blocker for this patch.

This revision is now accepted and ready to land.Feb 28 2021, 5:37 AM
praveen updated this revision to Diff 326973.Feb 28 2021, 10:41 AM
praveen marked 11 inline comments as done.

Add TODO and comments for the data structures

@kiranchandramohan The test case flang/test/Semantics/resolve102.f90 is failing with the error messages possibly due to the commit

https://github.com/llvm/llvm-project/commit/07de0846a5055015b55dc2b8faa2143f9902e549#diff-bc82e30d81709b26f2c8030300784e95003f1b968e1ed7842fa717858e497336

actual at 13: Procedure 'p' is recursively defined. Procedures in the cycle: 'p2', 'sub', 'p'
expect at 13: Procedure 'p' is recursively defined. Procedures in the cycle: 'p2', 'p', 'sub'
actual at 25: Procedure 'p' is recursively defined. Procedures in the cycle: 'p2', 'sub', 'p'
expect at 25: Procedure 'p' is recursively defined. Procedures in the cycle: 'p2', 'p', 'sub'

Can I push this patch now or only after the above failed case is fixed ?

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

@kiranchandramohan @clementval Thanks . I have added a TODO to replace the hard-coded names as part of this patch.

Will create a separate patch if it is possible to add an auto generated function to maintain the map.

flang/lib/Semantics/check-omp-structure.h
85–87

Added the comments.

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

Added a check for Symbol::Flag::OmpCopyPrivate

@kiranchandramohan The test case flang/test/Semantics/resolve102.f90 is failing with the error messages possibly due to the commit

https://github.com/llvm/llvm-project/commit/07de0846a5055015b55dc2b8faa2143f9902e549#diff-bc82e30d81709b26f2c8030300784e95003f1b968e1ed7842fa717858e497336

actual at 13: Procedure 'p' is recursively defined. Procedures in the cycle: 'p2', 'sub', 'p'
expect at 13: Procedure 'p' is recursively defined. Procedures in the cycle: 'p2', 'p', 'sub'
actual at 25: Procedure 'p' is recursively defined. Procedures in the cycle: 'p2', 'sub', 'p'
expect at 25: Procedure 'p' is recursively defined. Procedures in the cycle: 'p2', 'p', 'sub'

Can I push this patch now or only after the above failed case is fixed ?

You can push it now since the failure is unrelated to your patch.

This revision was landed with ongoing or failed builds.Mar 1 2021, 6:25 AM
This revision was automatically updated to reflect the committed changes.