Page MenuHomePhabricator

[Flang][OpenMP 4.5] Add semantic check for OpenMP Schedule Clause - chunk size
AbandonedPublic

Authored by yhegde on Fri, Nov 20, 9:58 AM.

Details

Summary

Semantic check for OpenMP 4.5 - 2.7.1 Schedule clause .
Checks for the value of chunk size of the schedule clauses are the same for all threads

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

Test cases:
omp-schedule-teams.f90

Hopefully this is the right test case. I request the reviewers to provide some more inputs about the test cases.

Diff Detail

Event Timeline

yhegde created this revision.Fri, Nov 20, 9:58 AM
yhegde requested review of this revision.Fri, Nov 20, 9:58 AM

I will provide my understanding below. Can also check with @huntergr @Meinersbur.

Team is a set of threads executing a parallel region. This restriction is that the chunk size (which can be a variable but only needs to be positive) should not be different for different threads in the team.
So I think code like the following are disallowed. Not sure whether we can catch these during semantic checks.

subroutine fn(a,b,c,N)
  integer :: i
  integer :: a(:), b(:), c(:)
  integer :: chunk
  !$omp parallel
  chunk = 1 + omp_get_thread_num()
  !$omp do schedule(static, chunk)
  do i=1,N
    a(i) = b(i) + c(i)
  end do
  !$omp end do
  !$omp end parallel
end subroutine
yhegde updated this revision to Diff 306831.Sat, Nov 21, 2:11 AM

Thank you very much for the inputs @kiranchandramohan . If my understanding is correct, this test case requires a run time check and probably it is not possible to do a semantic check.

Based on your inputs I have updated the negative test case and added a positive test case too.
Thank you.

flang/test/Semantics/omp-schedule-teams01.f90
18

I was thinking it is OK to have different chunk_sizes for different worksharing loops. Also, Generally there are implicit barriers after worksharing loops.
Did you check with a different fortran compiler or a c/c++ compiler?

yhegde added inline comments.Tue, Dec 1, 12:34 AM
flang/test/Semantics/omp-schedule-teams01.f90
18

No error for this test case in gfortran and classic flang. No error to equivalent c++ test case in g++ and clang++ too. It looks like a runtime check. (https://www.openmp.org/wp-content/uploads/openmp-4.5.pdf , page 60)

I only saw the following one which puts some conditions for using the same assignment of logical iteration numbers to threads for two loops. If these conditions are not met it is just that the implementation can use a different assignment. Are you referring to something else?

"A compliant implementation of the static schedule must ensure that the same assignment of logical iteration numbers to threads will be used in two loop regions if the following conditions are satisfied: 1) both loop regions have the same number of loop iterations, 2) both loop regions have the same value of chunk_size specified, or both loop regions have no chunk_size specified, 3) both loop regions bind to the same parallel region, and 4) neither loop is associated with a SIMD construct. A data dependence between the same logical iterations in two such loops is guaranteed to be satisfied allowing safe use of the nowait clause."

yhegde added a comment.Tue, Dec 1, 4:12 AM

I only saw the following one which puts some conditions for using the same assignment of logical iteration numbers to threads for two loops. If these conditions are not met it is just that the implementation can use a different assignment. Are you referring to something else?

"A compliant implementation of the static schedule must ensure that the same assignment of logical iteration numbers to threads will be used in two loop regions if the following conditions are satisfied: 1) both loop regions have the same number of loop iterations, 2) both loop regions have the same value of chunk_size specified, or both loop regions have no chunk_size specified, 3) both loop regions bind to the same parallel region,

and 4) neither loop is associated with a SIMD construct. A data dependence between the same logical iterations in two such loops is guaranteed to be satisfied allowing safe use of the nowait clause."

Yes was referring to the 2), second one actually. So probably what check introduced in this patch is applicable as a semantic check. And I suppose the one which you were suggesting -
subroutine fn(a,b,c,N)

integer :: i
integer :: a(:), b(:), c(:)
integer :: chunk
!$omp parallel
chunk = 1 + omp_get_thread_num()
!$omp do schedule(static, chunk)
do i=1,N
  a(i) = b(i) + c(i)
end do
!$omp end do
!$omp end parallel

end subroutine

  • can be done at runtime. Also kindly suggest how I can go ahead with this patch.

Yes was referring to the 2), second one actually. So probably what check introduced in this patch is applicable as a semantic check. And I suppose the one which you were suggesting can be done at runtime. Also kindly suggest how I can go ahead with this patch.

I was saying that the check that you have introduced in this patch is a direction to the OpenMP runtime on how to assign iterations to threads. Having different chunk sizes on different loops in a parallel region is OK, it is just that the iteration to threads mapping will be different.

My recommendation is to drop this check unless you have a strong reason to not to.

yhegde added a comment.Tue, Dec 1, 6:46 AM

Yes was referring to the 2), second one actually. So probably what check introduced in this patch is applicable as a semantic check. And I suppose the one which you were suggesting can be done at runtime. Also kindly suggest how I can go ahead with this patch.

I was saying that the check that you have introduced in this patch is a direction to the OpenMP runtime on how to assign iterations to threads. Having different chunk sizes on different loops in a parallel region is OK, it is just that the iteration to threads mapping will be different.

My recommendation is to drop this check unless you have a strong reason to not to.

ok. Thank you. Shall I close this review ?

OK. please close.

yhegde abandoned this revision.Tue, Dec 1, 8:47 AM

As reviewers feel this semantic check is not needed, I am closing this review.