This is an archive of the discontinued LLVM Phabricator instance.

[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

yhegde created this revision.Dec 6 2020, 10:33 AM
yhegde requested review of this revision.Dec 6 2020, 10:33 AM
kiranchandramohan requested changes to this revision.Dec 6 2020, 3:18 PM
kiranchandramohan added a subscriber: sameeranjoshi.
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
120

CheckDoControl and CheckDoVariable in flang/lib/Semantics/check-do-forall.cpp does the same check but are probably OFF or set as warning by default. Can the code in check-do-forall be reused?

154

Can we avoid having these global variables?

205

Are you handling cycle with the name of a loop?

519

Nit: Is this new line needed?

flang/lib/Semantics/check-omp-structure.h
191–193

Can we keep the declarations at the top or at the bottom. Please refer to style elsewhere.

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

FYI @sameeranjoshi has a patch which does something similarhttps://reviews.llvm.org/D92638.

This revision now requires changes to proceed.Dec 6 2020, 3:18 PM
yhegde updated this revision to Diff 310171.Dec 8 2020, 6:36 AM

Test cases added for named do loops. Global variables removed.

TestCases:
omp-do04.f90
omp-do08.f90
omp-do09.f90
omp-do10.f90
omp-do11.f90
omp-do12.f90

yhegde marked 5 inline comments as done.Dec 8 2020, 6:39 AM
yhegde updated this revision to Diff 310898.Dec 10 2020, 8:23 AM

Implementation includes a check to "the ordered clause must be present on the loop construct if any ordered region ever binds to a loop region arising from the loop construct" - along with the previous Do loop iteration variable check , Do while loop check , Do loop threadprivate check, Do loop cycle restrictions.

Test case is omp-do06.f90.

yhegde updated this revision to Diff 311494.Dec 13 2020, 11:22 PM

Rebased. Requesting the review comments.

clementval added inline comments.Dec 14 2020, 5:37 PM
flang/test/Semantics/omp-do06.f90
14

Is this a tab?

flang/test/Semantics/omp-do08.f90
71

tabs?

clementval added inline comments.Dec 14 2020, 5:41 PM
flang/test/Semantics/omp-do11.f90
2

Shouldn't you use the test_symbols.sh script for these tests?

yhegde added inline comments.Dec 18 2020, 9:59 PM
flang/test/Semantics/omp-do11.f90
2

I guess so. I have committed a few positive test cases with test_errors.sh earlier. I suppose I need to rework on them !!.

yhegde updated this revision to Diff 313076.Dec 21 2020, 4:00 AM

This patch is updated with corrections to positive cases and indentations.

kiranchandramohan requested changes to this revision.Jan 14 2021, 6:31 AM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
109

If more than one loop is associated with the OpenMP do loop, should this check be performed on all the loops?

120

Is the answer here NO?

147–148

Why is collapse level used here but not in CheckLoopItrVariableIsInt?

205

I probably did not make myself clear here. What i was suggesting was that it is not enough to look at where the cycle exists but also have to consider where the cycle branches to. Consider the testcase below.

program test
  integer i, j, k
  !$omp do  collapse(2)
  foo: do i = 0, 10
         do j = 0, 10
           do k  = 0, 10
             if (k .lt. 1) cycle foo
             print *, i, j, k
           end do
         end do
  end do foo
  !$omp end do
end program
213

Is there an assumption here that if statement will be present when a cycle statement exists? Does the standard say that?

341

We do symbol related checks in resolve-directives right? Is there any particular reason this code should be here?

This revision now requires changes to proceed.Jan 14 2021, 6:31 AM
yhegde added inline comments.Jan 14 2021, 10:28 AM
flang/lib/Semantics/check-omp-structure.cpp
109

https://www.openmp.org/wp-content/uploads/openmp-4.5.pdf , page num 74, line num 5-6 says " If no collapse clause is present, the only loop that is associated with the loop construct is the one that immediately follows the directive."

So our understanding was if no collapse is present, only the iterations of outermost loop are distributed among threads.

gfrotran also throws error for only the outer loop.

120

I modified my code to suggested change. i.e I am doing similar kind of check like how it is done in check-do-forall.cpp

Will look at the rest of the review comments. Thank you.

DavidTruby resigned from this revision.Jan 15 2021, 6:13 AM

Please add @sameeranjoshi in all future patches.

yhegde marked an inline comment as done.Jan 21 2021, 6:17 AM
yhegde added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
147–148

I think with or without collapse clause, do iteration variables in all loops should be checked for integer type.

205

Done. Thanks.

213

Cycles with out ifstmts are also handled.

341

All do iteration variable related checks have been implemented in check-omp-structure.cpp

yhegde updated this revision to Diff 318187.Jan 21 2021, 7:08 AM
yhegde marked an inline comment as done.

Addressed review comments.

kiranchandramohan requested changes to this revision.Jan 22 2021, 5:06 PM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
213

I think it might not work with else if, nested if etc.

Anyway, my point was that cycle statements can appear inside other fortran constructs, for e.g in a case statement like in the example below. So this portion of the code has to be generic and not limited to if statements.

program test
  integer i, j, k
  !$omp do collapse(3)
  foo: do i = 0, 10
    bar: do j = 0, 10
           do k = 0, 10
             select case (k)
               case (1)
                 cycle foo
               case (2)
                 cycle bar
               case default
                 cycle
             end select
           end do
         end do bar
  end do foo
  !$omp end do
end program
flang/lib/Semantics/check-omp-structure.h
206

Are you resetting this to null anywhere? Stale values might cause issues.

Instead of this method can you travel up the stack, find the do construct if any and check its clauses for the ordered clause?

This revision now requires changes to proceed.Jan 22 2021, 5:06 PM
yhegde marked an inline comment as done.Jan 26 2021, 11:17 AM
yhegde added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
213

Thanks for pointing out these issues. Can I have a class (OmpCycleBlockChecker) and parser::Walk to identify the cycles.? I have tried that for existing test cases (0mp-do08.f90 and omp-do13.f90 negative cases. and omp-do12.f90 and omp-do14.f90 - positive cases). I can extend it to other cases if it is acceptable.

flang/lib/Semantics/check-omp-structure.h
206

Thanks for the review comments. CheckIfDoOrderedClause () is called before pushingthecontext and associated clauses of the OMPD_do context are searched for the ordered clause.

yhegde updated this revision to Diff 319361.Jan 26 2021, 11:21 AM
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
340

Is this variable used?

341

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
355–357

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
1 ↗(On Diff #319361)

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
340

Will remove that. :)

341

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

355–357

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
1 ↗(On Diff #319361)

ok.

yhegde added inline comments.Jan 29 2021, 10:41 AM
flang/test/Semantics/omp-do13.f90
1 ↗(On Diff #319361)

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.Jan 29 2021, 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.Jan 30 2021, 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
66

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

257

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

274

Use DO loop instead of do-loop.

341

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
341

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) {
flang/test/Semantics/omp-do13.f90
1 ↗(On Diff #319361)

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.Jan 30 2021, 3:46 AM
yhegde added inline comments.Jan 30 2021, 7:01 AM
flang/lib/Semantics/check-omp-structure.cpp
66

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
66

Would the following work?

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

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

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

341

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
66

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.

341

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.Feb 1 2021, 7:24 PM
yhegde added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
341

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.Feb 2 2021, 6:10 AM
flang/lib/Semantics/check-omp-structure.cpp
341

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
341

@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.Feb 2 2021, 6:49 AM
flang/lib/Semantics/check-omp-structure.cpp
341

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

@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.Feb 2 2021, 9:05 AM
flang/lib/Semantics/check-omp-structure.cpp
341

@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.Feb 2 2021, 9:35 AM
flang/lib/Semantics/check-omp-structure.cpp
341

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

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.Feb 3 2021, 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.Feb 3 2021, 10:46 AM
flang/lib/Semantics/check-omp-structure.cpp
341

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.Feb 3 2021, 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
312–313

An else to the previous if is better.

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

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

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

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
815

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.Feb 6 2021, 12:09 AM
flang/lib/Semantics/resolve-directives.cpp
815

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.Feb 8 2021, 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.Feb 9 2021, 7:53 AM
This revision now requires changes to proceed.Feb 9 2021, 7:53 AM
yhegde added a comment.Feb 9 2021, 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.Feb 10 2021, 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
62

Nit this can just be

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

Nit: cyclesource_ -> cycleSource_

257

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

281–283

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.

312

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.Feb 14 2021, 8:52 AM
yhegde added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
281–283

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.Feb 14 2021, 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
281–283

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.Feb 14 2021, 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.Feb 14 2021, 11:11 AM
flang/lib/Semantics/check-omp-structure.cpp
281–283

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
281–283

OK. Thanks. You can submit.