Page MenuHomePhabricator

[Flang][OpenMP 4.5] Add semantic check for OpenMP Do Loop Constructs
ClosedPublic

Authored by yhegde on Dec 6 2020, 10:33 AM.

Details

Summary

Semantic check for OpenMP 4.5 - 2.7.1 Do Loop restrictions.
Implementation of Do loop iteration variable check , Do while loop check , Do loop threadprivate check,
Do loop cycle restrictions.

Files:
check-omp-structure.cpp
check-omp-structure.h
resolve-directives.cpp

TestCases:
omp-doloop02.f90
omp-doloop03.f90
omp-doloop04-pc.f90
omp-doloop04.f90

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
yhegde marked an inline comment as done.Jan 26 2021, 11:21 AM

Code that are related to ordered clause and cycle stmts are modified to address the review comments.

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

Is this variable used?

306

Will checks only happen for loops with collapse clause? What about those without collapse? Should it be handled elsewhere?

!$omp parallel
foo: do i = 0, 10
  !$omp do
  bar: do j = 0, 10
         cycle foo
       end do bar
  !$omp end do
  !$omp end parallel
end do foo
320–322

The concern with the approach is that you might have to call ompCycleBlockChecker for each individual construct. That is likely to be costly. Can parser::Walk happen on the entire block?

Do you have an estimate on how many additional visits will be there to a statement inside a Do loop due to this Walk? Will it be only 1 or more than that? If it is one additional visit it should be OK.

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

Please add a test for an openmp do loop with collapse nested inside an openmp do loop with collapse.

yhegde added inline comments.Jan 27 2021, 11:08 AM
flang/lib/Semantics/check-omp-structure.cpp
305

Will remove that. :)

306

Cases with OpenMP blocks in between are not handled . It requires different error message. But cases with out collapse - like the following case, works fine. Will add the test cases with out collapse. Thank you.
program test
!$omp parallel
foo: do i = 0, 10

!!$omp do
bar: do j = 0, 10
       cycle foo
end do bar
!!$omp end do

end do foo
!$omp end parallel
end program test

320–322

parser::Walk on entire block was throwing error twice in few cases. Hence I called it with in a construct. Will check that.

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

ok.

yhegde added inline comments.Fri, Jan 29, 10:41 AM
flang/test/Semantics/omp-do13.f90
2

Please add a test for an openmp do loop with collapse nested inside an openmp do loop with collapse.

Added a case like this . Hope this is what is suggested.

!$omp do collapse(2)

foo: do i = 0, 10
  foo1: do j = 0, 10
    !ERROR: CYCLE statement to non-innermost collapsed !$OMP DO loop
    cycle foo
    !ERROR: A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region
    !$omp do  collapse(1)
    foo2:  do k  = 0, 10
      print *, i, j, k
      end do foo2
   !$omp end do
   end do foo1
end do foo
!$omp end do
yhegde updated this revision to Diff 320173.Fri, Jan 29, 10:52 AM

The patch is updated with Cycle checks in IfConstruct and CaseConstruct with omp-do15.f90 ,omp-do16.f90 and omp-do17.f90 . The parser::Walk is out of the loop now and called only once. Cycles are mostly a concern when appear in do loops with collapse clause. Hence most of the test cases are with collapse.

kiranchandramohan requested changes to this revision.Sat, Jan 30, 3:46 AM

-> Threadprivate check is not working for the following case (when the declaration is in the parent scope).

program omp_do
  integer, save:: i, j, k
  !$omp threadprivate(i)
  !$omp threadprivate(j)
  call compute()
contains
  subroutine compute()
  !$omp  do collapse(2)
  do k = 1, 10
    do i = 1, 10
      print *, "Hello"
    end do
  end do
  !$omp end do
  end subroutine
end program omp_do

-> Do loop cycle restrictions should also include ordered besides collapse. I think this should be the same checks.

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

We usually do not add the "!$" in the error message.

222

Can the "do" be capitalized for the loop and the directive?

239

Use DO loop instead of do-loop.

306

My concern was because the cyclechecker is entered only if the collapse level is greater than 0. If there is no collapse then i guess it will return 0 and hence the cyclechecker might not be called.

std::int64_t collapseLevel{GetCollapseLevel(x)};
if (collapseLevel) {
397

This clear will cause programs with multiple declarations to not work properly. See example below.

program omp_do
  integer, save:: i, j, k
  !$omp threadprivate(i)
  !$omp threadprivate(j)
  !$omp do collapse(2)
  do k = 1, 10
    do i = 1, 10
      print *, "Hello"
    end do
  end do
  !$omp end do
end program omp_do
flang/test/Semantics/omp-do13.f90
2

I mean't the following where you have the same error in both the inner and outer collapsed set of loops. Also add another one with parallel do collapse(2).

!$omp parallel
!$omp do collapse(2)
foo: do i = 0, 10
  foo1: do j = 0, 10
    cycle foo
    !$omp parallel
    !$omp do collapse(2)
    foo2:  do k  = 0, 10
      foo3:  do l  = 0, 10
      print *, i, j, k, l
      cycle foo2
      end do foo3
    end do foo2
    !$omp end do
    !$omp end parallel
   end do foo1
end do foo
!$omp end do
!$omp end parallel
end program
This revision now requires changes to proceed.Sat, Jan 30, 3:46 AM
yhegde added inline comments.Sat, Jan 30, 7:01 AM
flang/lib/Semantics/check-omp-structure.cpp
113

That is gfortran error message. Kindly suggest a better one. Will look into the rest of the review comments. Thank you.

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

Would the following work?

"CYCLE statement to non-innermost associated loop of an OpenMP DO construct"

yhegde marked 4 inline comments as done.Mon, Feb 1, 11:45 AM
yhegde added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
113

can that be "CYCLE statement to non-innermost associated loop of an OpenMP collapsed DO construct" ?

306

All negative test cases including those cases which have been suggested with parallel /parallel do collapse are all positive without collapse. means they give no error. please let me know if it is required to check for cycles in do loops with out collapse clause.

kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
113

Associated loops can be due to both collapse as well as ordered clauses. Hence I would prefer that you not add "collapsed" to the message.

306

Is your point that it should be handled by https://reviews.llvm.org/D92735 and not this patch? If so, I am fine if @praveen can confirm.

yhegde marked an inline comment as done.Mon, Feb 1, 7:24 PM
yhegde added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
306

Ok. Fine. I was under the impression that the cycles are the cause of error only with the collapse clause. I can add test cases with ordered clause also.

praveen added inline comments.Tue, Feb 2, 6:10 AM
flang/lib/Semantics/check-omp-structure.cpp
306

Is your point that it should be handled by https://reviews.llvm.org/D92735 and not this patch? If so, I am fine if @praveen can confirm.

@kiranchandramohan The patch https://reviews.llvm.org/D92735 can be extended to check for cases such as the one below.

!$omp parallel
foo: do i = 0, 10
  !$omp do
  bar: do j = 0, 10
         cycle foo
       end do bar
  !$omp end do
  !$omp end parallel
end do foo

./omp-do-cycle.f90:15:18: error: invalid branch into an OpenMP structured block
           cycle foo
                 ^^^
./omp-do-cycle.f90:11:3: In the enclosing PARALLEL directive branched into
    foo: do i = 0, 10
    ^^^
./omp-do-cycle.f90:15:18: error: invalid branch leaving an OpenMP structured block
           cycle foo
                 ^^^
./omp-do-cycle.f90:11:3: Outside the enclosing DO directive
  foo: do i = 0, 10
  ^^^
flang/lib/Semantics/check-omp-structure.cpp
306

@praveen I was hopeful that CheckNoBranching will catch the following error. Do you know why it does not catch in the current state? Is it just about calling CheckNoBranching in a few more places?

./omp-do-cycle.f90:15:18: error: invalid branch leaving an OpenMP structured block
           cycle foo
                 ^^^
./omp-do-cycle.f90:11:3: Outside the enclosing DO directive
  foo: do i = 0, 10

Since the source and destination of the branch are in the parallel region there should be no error for the parallel directive.

praveen added inline comments.Tue, Feb 2, 6:49 AM
flang/lib/Semantics/check-omp-structure.cpp
306

@praveen I was hopeful that CheckNoBranching will catch the following error. Do you know why it does not catch in the current state? Is it just about calling CheckNoBranching in a few more places?

@kiranchandramohan Yes, as you suggested , CheckNoBranching should be able to catch this error , but since we are not handling "parser::CycleStmt" in the NoBranchingEnforce class it is not being checked.

Also CheckNoBranching is not being called for all the OpenMP structured blocks in the current state (It is checked as part of the patch for invalid branching checks)

The following is the output from handling the "parser::CycleStmt" in NoBranchingEnforce along with the changes in the patch https://reviews.llvm.org/D92735 for the below test case

!$omp parallel
foo: do i = 0, 10
  !$omp do
  bar: do j = 0, 10
         cycle foo
       end do bar
    !$omp end do
  !$omp end parallel
end do foo

error: CYCLE to construct 'foo' outside of DO construct is not allowed
           cycle foo
           ^^^^^^^^^
./d1.f90:13:9: Enclosing DO construct
  !$omp do
        ^^

For the cases such as this , two errors are identified for both the constructs

foo: do i = 0, 10
!$omp parallel
  !$omp do
  bar: do j = 0, 10
         cycle foo
       end do bar
       !$omp end do
  !$omp end parallel
end do foo

./omp-do-cycle.f90:15:12: error: CYCLE to construct 'foo' outside of PARALLEL construct is not allowed
           cycle foo
           ^^^^^^^^^
./omp-do-cycle.f90:11:9: Enclosing PARALLEL construct
  !$omp parallel
        ^^^^^^^^
./omp-do-cycle.f90:15:12: error: CYCLE to construct 'foo' outside of DO construct is not allowed
           cycle foo
           ^^^^^^^^^
./omp-do-cycle.f90:12:11: Enclosing DO construct
    !$omp do
          ^^

Please suggest if it is better to add the check as part of CheckNoBranching or in resolve-directives.cpp

Since the source and destination of the branch are in the parallel region there should be no error for the parallel directive.

Okay will add a check for the same

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

@praveen in my opinion handling "parser::CycleStmt" in NoBranchingEnforce and calling checkNoBranching is the right approach.

The above two errors match my expectation. If it matches yours also then please go ahead and make the change in https://reviews.llvm.org/D92735. If not then let me know the concern.

Thanks for your help here.

yhegde added inline comments.Tue, Feb 2, 9:05 AM
flang/lib/Semantics/check-omp-structure.cpp
397

@kiranchandramohan . Thanks for pointing this out. Can I have the ThreadPrivate related check in resolve-directives.cpp or is it required to do in check-omp-structure.cpp as it is openmp specific. ?

For the test case given (if implemented in resolve-directives.cpp)
program omp_do

integer, save:: i, j, k
!$omp threadprivate(i)
!$omp threadprivate(j)
!$omp do collapse(2)
do k = 1, 10
  do i = 1, 10
    print *, "Hello"
  end do
end do
!$omp end do

end program omp_do

This is the error thrown
/tp.f90:10:8: error: Loop iteration variable i is not allowed in THREADPRIVATE.

do i = 1, 10
   ^

and for omp-do04.f90

the errors are -
./omp-do04.f90:12:6: error: Loop iteration variable i is not allowed in THREADPRIVATE.

do i = 1, 10
   ^

./omp-do04.f90:14:8: error: Loop iteration variable j is not allowed in THREADPRIVATE.

do j = 1, 10
   ^

./omp-do04.f90:25:6: error: Loop iteration variable i is not allowed in THREADPRIVATE.

do i = 1, 10
   ^

./omp-do04.f90:26:8: error: Loop iteration variable j is not allowed in THREADPRIVATE.

do j = 1, 10
   ^
praveen added inline comments.Tue, Feb 2, 9:35 AM
flang/lib/Semantics/check-omp-structure.cpp
306

@kiranchandramohan @yhegde Thanks . I have updated the patch https://reviews.llvm.org/D92735 with the changes for parser::CycleStmt in NoBranchingEnforce to handle cycle statements leaving the OpenMP structured blocks

I think you are doing three patches worth of work in a single one. Ideally, we could have had, One patch for the cycle restrictions, One patch for threadprivate, and One patch for the rest.

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

Yes, you can add the check in resolve-directives.cpp.

Why is there an error in line 26 of test omp-do04.f90? Since there is no collapse/ordered, should there be an error?

yhegde added a comment.Wed, Feb 3, 7:39 AM

I think you are doing three patches worth of work in a single one. Ideally, we could have had, One patch for the cycle restrictions, One patch for threadprivate, and One patch for the rest.

Agreed. I suppose difficult to review also. but initially we thought it is good to have all do loop related checks committed together.

yhegde added inline comments.Wed, Feb 3, 10:46 AM
flang/lib/Semantics/check-omp-structure.cpp
397

Thank you @kiranchandramohan . I have updated the code and the test cases with ordered and collapsed clauses together. And the error for omp-do04.f90 is -

./omp-do04.f90:12:6: error: Loop iteration variable i is not allowed in THREADPRIVATE.

do i = 1, 10
   ^

./omp-do04.f90:14:8: error: Loop iteration variable j is not allowed in THREADPRIVATE.

do j = 1, 10
   ^

./omp-do04.f90:27:6: error: Loop iteration variable i is not allowed in THREADPRIVATE.

do i = 1, 10
   ^

Now the thread private related code is moved to resolve-directives.cpp

yhegde updated this revision to Diff 321161.Wed, Feb 3, 10:59 AM

This patch address the review comments.
The cycle related checks are in OmpCycleChecker class.
The test cases omp-do08.f90 and then omp-do12.f90 to omp-do17.f90 are the test cases for cycle checks.

The Threadprivate check is moved to resolve-directives.cpp and omp-do04.f90 is the test case for that check.

flang/lib/Semantics/check-omp-structure.cpp
277–278

An else to the previous if is better.

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

Will this work if there are two omp do loops having threadprivate iteration variables?

yhegde marked an inline comment as done.Wed, Feb 3, 9:55 PM
yhegde added inline comments.
flang/lib/Semantics/resolve-directives.cpp
1036

For the the following test case

program omp_do1

integer, save:: i, j, k, n
!$omp  threadprivate(i)
!$omp  do
!ERROR: Loop iteration variable i is not allowed in THREADPRIVATE.
do i = 1, 10
  !!$omp  threadprivate(j)
  !$omp  do
  do j = 1, 10
    print *, "Hello"
  end do
end do
!$omp end do

!$omp  do
!ERROR: Loop iteration variable i is not allowed in THREADPRIVATE.
do i = 1, 10
  !!$omp  threadprivate(j)
  !$omp  do
  do j = 1, 10
    print *, "Hello"
  end do
end do
!$omp end do

end program omp_do1

the error is

./kcmtp3.f90:6:6: error: Loop iteration variable i is not allowed in THREADPRIVATE.

do i = 1, 10
   ^

./kcmtp3.f90:8:12: error: A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region

!$omp  do
       ^^

./kcmtp3.f90:17:6: error: Loop iteration variable i is not allowed in THREADPRIVATE.

do i = 1, 10
   ^

./kcmtp3.f90:19:12: error: A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region

!$omp  do
       ^^

and if I uncomment !$omp threadprivate(j) , I get parse errors.

and the gfortran error is

kcmtp3.f90:6:14:

6 |   do i = 1, 10
  |              1

Error: !$OMP DO iteration variable must not be THREADPRIVATE at (1)
kcmtp3.f90:17:14:

17 |   do i = 1, 10
   |              1

Error: !$OMP DO iteration variable must not be THREADPRIVATE at (1)

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

I got that slightly wrong. What i meant is that if we clear the symbols after visiting a block construct then we will miss the error on a following loop construct. For e.g.

integer, save:: i, j, k, n
!$omp  threadprivate(i)

!$omp parallel
print *, "parallel"
!$omp end parallel

!$omp  do
do i = 1, 10
  do j = 1, 10
    print *, "Hello"
  end do
end do
!$omp end do

end program
yhegde added inline comments.Sat, Feb 6, 12:09 AM
flang/lib/Semantics/resolve-directives.cpp
1036

Probably I can call ClearThreadPrivateSymbols() in bool Pre(const parser::ModuleSubprogram &) ?

For the test case
program test
integer, save:: i, j, k, n
!$omp threadprivate(i)

!$omp parallel
print *, "parallel"
!$omp end parallel

!$omp do
do i = 1, 10

do j = 1, 10
  print *, "Hello"
end do

end do
!$omp end do

end program test

the error message is -
./tp4.f90:10:4: error: Loop iteration variable i is not allowed in THREADPRIVATE.

do i = 1, 10
   ^

and for the test case

program omp_do2

!$omp threadprivate(k,i)
!$omp threadprivate(j)
!$omp  threadprivate(i)
call compute()
contains
subroutine compute()
!$omp  do ordered(2) collapse(2)
!ERROR: Loop iteration variable k is not allowed in THREADPRIVATE.
foo: do k = 1, 10
  do i = 1, 10
    print *, "Hello"
  end do
end do foo
!$omp end do
end subroutine

end program omp_do2

program omp_do3

!$omp  threadprivate(j)
!$omp parallel
print *, "parallel"
!$omp end parallel
!$omp  do
!ERROR: Loop iteration variable i is not allowed in THREADPRIVATE.
do j = 1, 10
  do i = 1, 10
    print *, "Hello"
  end do
end do
!$omp end do

end program omp_do3

the error message is
./tp5.f90:10:11: error: Loop iteration variable k is not allowed in THREADPRIVATE.

foo: do k = 1, 10
        ^

./tp5.f90:11:8: error: Loop iteration variable i is not allowed in THREADPRIVATE.

do i = 1, 10
   ^

./tp5.f90:27:6: error: Loop iteration variable j is not allowed in THREADPRIVATE.

do j = 1, 10
   ^
yhegde updated this revision to Diff 322170.Mon, Feb 8, 11:00 AM

Addressed the review comments and rebased .

Thanks @yhegde for the changes. I believe this threadprivate check needs some more design and work. I don' t think we can clear the threadprivate symbols in module subprograms because it will fail to catch the error in test1 below. I think the threadprivate information should be captured in a symbol so that it is available in places where the module is used like in test2.

Would it be OK to move threadprivate checks to a separate patch? So that we can merge the other checks? We can even consider moving threadprivate out of the current statement of work since supporting this feature might be more effort than estimated. cc: @richard.barton.arm

test1

module md
integer :: i, j
!$omp  threadprivate(i)
contains
subroutine sb1
!$omp  do
do i = 1, 10
  do j = 1, 10
    print *, "Hello"
  end do
end do
!$omp end do
end subroutine
end module

test2

module md
integer :: i
!$omp  threadprivate(i)
end module

program mn
use md 
!$omp  do
do i = 1, 10 
  do j = 1, 10
    print *, "Hello"
  end do
end do
!$omp end do
end program
kiranchandramohan requested changes to this revision.Tue, Feb 9, 7:53 AM
This revision now requires changes to proceed.Tue, Feb 9, 7:53 AM
yhegde added a comment.Tue, Feb 9, 6:55 PM

Thanks @yhegde for the changes. I believe this threadprivate check needs some more design and work. I don' t think we can clear the threadprivate symbols in module subprograms because it will fail to catch the error in test1 below. I think the threadprivate information should be captured in a symbol so that it is available in places where the module is used like in test2.

Thank you for the review comments @kiranchandramohan . Can I split this patch one with threadprivate and cycle , dowhile etc with another patch ?

Would it be OK to move threadprivate checks to a separate patch? So that we can merge the other checks? We can even consider moving threadprivate out of the current statement of work since supporting this feature might be more effort than estimated. cc: @richard.barton.arm

test1

module md
integer :: i, j
!$omp  threadprivate(i)
contains
subroutine sb1
!$omp  do
do i = 1, 10
  do j = 1, 10
    print *, "Hello"
  end do
end do
!$omp end do
end subroutine
end module

test2

module md
integer :: i
!$omp  threadprivate(i)
end module

program mn
use md 
!$omp  do
do i = 1, 10 
  do j = 1, 10
    print *, "Hello"
  end do
end do
!$omp end do
end program

Thank you for the review comments @kiranchandramohan . Can I split this patch one with threadprivate and cycle , dowhile etc with another patch ?

Yes.
This patch can contain cycle, dowhile etc. Will try to approve it soon.
Another patch can contain the threadprivate checks.

yhegde updated this revision to Diff 322905.Wed, Feb 10, 9:55 PM

Based on Reviewer's comments , this patch is now updated with DoWhile , Cycles and other checks. Threadprivate related checks are removed.

One question about blocks and a few nits.

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

Nit this can just be

err = (it != labelNamesandLevels_.end() && it->second > 0);
125

Nit: cyclesource_ -> cycleSource_

222

Nit: Capitalize for the directive as well.
"The DO loop cannot be a DO WHILE with DO directive."

246–248

Is something missing here?
If "it" is block.begin() in the previous patch, how can it ever be block.end in this line unless the block is empty?

If nothing is missing, add a comment.

277

Nit: this condition is always true right? if so it can be reduced to an else statement (from else if).

yhegde marked 4 inline comments as done.Sun, Feb 14, 8:52 AM
yhegde added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
246–248

Is something missing here?
If "it" is block.begin() in the previous patch, how can it ever be block.end in this line unless the block is empty?

I think If the block is empty then Fortran::parser::ExecutableConstruct has "no contained value". If block is not empty then DoConstruct, if exists in ExecutableConstruct, is fetched. Please let me know otherwise.

If nothing is missing, add a comment.

yhegde updated this revision to Diff 323619.Sun, Feb 14, 9:03 AM

Patch updated and rebased after addressing the review comments.

LGTM. I have a Nit comment.

Did you create a separate PR for the threadprivate checks?

flang/lib/Semantics/check-omp-structure.cpp
246–248

Nit: Consider switching to the following.

loop = block.empty() ? nullptr : parser::Unwrap<parser::DoConstruct>(block.begin());
This revision is now accepted and ready to land.Sun, Feb 14, 9:18 AM

LGTM. I have a Nit comment.

Thank you @kiranchandramohan

Did you create a separate PR for the threadprivate checks?

Working on that.

yhegde added inline comments.Sun, Feb 14, 11:11 AM
flang/lib/Semantics/check-omp-structure.cpp
246–248

Tried this. Didn't get expected results (errors). Block is a list type. Similar kind of implementation can be found in resolve-directives:806 , 1201.

flang/lib/Semantics/check-omp-structure.cpp
246–248

OK. Thanks. You can submit.