Page MenuHomePhabricator

[Flang] [OpenMP] Add semantic checks for OpenMP Workshare Construct
ClosedPublic

Authored by praveen on Dec 11 2020, 12:04 AM.

Details

Summary

Add Semantic checks for OpenMP 4.5 - 2.7.4 Workshare Construct

  • The structured block in a worksahre construct may consist of only scalar or array assignments, forall or where statements, forall, where, atomic, critical or parallel constructs
  • All array assignments, scalar assignments, and masked array assignments must be intrinsic assignments.
  • The construct must not contain any user defined function calls unless the function is ELEMENTAL.

Resolve related test cases marked XFAIL

Diff Detail

Event Timeline

praveen created this revision.Dec 11 2020, 12:04 AM
praveen requested review of this revision.Dec 11 2020, 12:04 AM
Herald added a project: Restricted Project. · View Herald Transcript
praveen updated this revision to Diff 311129.Dec 11 2020, 12:27 AM

Rebase and apply clang-format

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

I think use of non-elemental function can be outside an assignment statement like in the where statement below.

program mn
  integer :: a(10) = (/1,2,3,4,5,6,7,8,9,10/)
  !$omp workshare
  where ( a .lt. f()) a = a + 5
  !$omp end workshare
contains
  integer function f()
    f = 5
  end function
end program
931–934

Is it better to use OmpStructureChecker::HasInvalidWorksharingNesting for handling this?

kiranchandramohan requested changes to this revision.Dec 23 2020, 3:16 PM
This revision now requires changes to proceed.Dec 23 2020, 3:16 PM
praveen updated this revision to Diff 315593.Jan 9 2021, 5:13 AM
  1. Handle uses of non-elemental functions in the workshare construct
  2. Handle the nested where/forall constructs enclosed in the workshare construct
  3. Apply the restrictions on workshare to OpenMP critical construct enclosed in workshare construct
praveen marked an inline comment as done.Jan 9 2021, 5:21 AM

@kiranchandramohan Updated the patch to handle all the checks on workshare construct . Since the WhereConstruct and ForallConstructs have nested WhereStmt/WhereConstructs and ForAllStmt/ForallConstructs , separate functions are added to handle the same recursively.

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

Thanks @kiranchandramohan Will handle the same.

907

@kiranchandramohan Modified the code to handle the same . Thanks!

931–934

@kiranchandramohan We would have to check for workshare directive and call OmpStructureChecker::HasInvalidWorksharingNesting for all the OpenMP Constructs.

Since the critical and atomic constructs are being checked here , thought it would be simpler to handle here.

Please let me know if we can have the check for other OpenMP constructs here or make use of HasInvalidWorksharingNesting in all the OpenMP Constructs

kiranchandramohan requested changes to this revision.Jan 13 2021, 1:11 AM

Add a few tests to show that most restrictions do not apply inside a parallel construct nested inside a workshare construct.

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

There are probably more places (see example below) where user-defined are not allowed. I wonder whether a parse-tree walk would have been better for catching issues like these.

program mn
integer, parameter ::N = 100
real :: A(N,N), B(N,N)
!$omp workshare
FORALL(I = f():N, J = f():N, A(I, J) .NE. f()) B(I, J) = 1.0 / A(I, J)
!$omp end workshare
contains
 pure integer function f()
    f = 5
  end function
end program
950

Nit: Remove empty line.

995–997

Nit: Capitalize constructs like elsewhere.

This revision now requires changes to proceed.Jan 13 2021, 1:11 AM
praveen marked an inline comment as done.Jan 14 2021, 11:36 AM

@kiranchandramohan To make use of parse-tree walk to check all the assignments and expressions in OpenMP Workshare constructs , is it preferred to add a class in check-omp-structure.cpp specific for workshare or add a generic class in Semantics/tools.cpp to collect all the assignments and expressions and then iterate over all the assignments / expressions identified in the walk?

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

@kiranchandramohan Thanks for pointing this out . I had missed this check in forall statements and construct.

I do feel parse-tree walk would be better to catch all the assignments and Expressions instead of multiple functions for each statement / construct.

praveen updated this revision to Diff 316931.Jan 15 2021, 6:37 AM

Use parse-tree walk to check the validity of assignments and expressions in workshare construct .

praveen marked 2 inline comments as done.Jan 15 2021, 6:40 AM

Add a few tests to show that most restrictions do not apply inside a parallel construct nested inside a workshare construct.

@kiranchandramohan Added a class to make use of parse-tree walk for identifying all the assignments and expressions in workshare construct . Added tests with parallel constructs inside a workshare construct.

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

Have added a new class to use parse-tree walk to catch all the assignments and Expressions.

950

Done

995–997

Done

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

Should this be set to some invalid value to avoid a stale value matching, since only block, loop and sections construct is handled for setting the currentDir?

967

Nit: Spelling ATOMIC

praveen marked 3 inline comments as done.Jan 18 2021, 9:45 AM
praveen added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
923

@kiranchandramohan Can the currentDir be set to llvm::omp::Directive::OMPD_unknown or is it used for a specific purpose ?

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

I think it is OK to use.

praveen updated this revision to Diff 317404.Jan 18 2021, 12:12 PM

Address review comments

This revision is now accepted and ready to land.Jan 18 2021, 2:41 PM
This revision was automatically updated to reflect the committed changes.
praveen marked 3 inline comments as done.