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

[yhegde]

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.

[kiranchandramohan]

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]

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.

[kiranchandramohan]

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]

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)

[kiranchandramohan]

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]

In D91879#2424968, @kiranchandramohan wrote:

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.

[kiranchandramohan]

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]

In D91879#2425553, @kiranchandramohan wrote:

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 ?

[kiranchandramohan]

OK. please close.

[yhegde]

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

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

bool HasInvalidWorksharingNesting(
       const parser::CharBlock &, const OmpDirectiveSet &);
 
   // specific clause related
   bool ScheduleModifierHasType(const parser::OmpScheduleClause &,
       const parser::OmpScheduleModifierType::ModType &);
 
   llvm::StringRef getClauseName(llvm::omp::Clause clause) override;
   llvm::StringRef getDirectiveName(llvm::omp::Directive directive) override;
+
+  std::optional<long int> chunkSize{0};
+  std::optional<long int> GetChunkSize() { return chunkSize; }
+  void SetChunkSize(const parser::ScalarIntExpr &chunkExpr) {
+    if (std::optional<long int> v{GetIntValue(chunkExpr)}) {
+      chunkSize.emplace(*v);
+    }
+  }
 };
 } // namespace Fortran::semantics
 #endif // FORTRAN_SEMANTICS_CHECK_OMP_STRUCTURE_H_

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

if (chunk) {
             "it must not have chunk size specified"_err_en_US,
             parser::ToUpperCaseLetters(
                 parser::OmpScheduleClause::EnumToString(kind)));
       }
       if (const auto &chunkExpr{
               std::get<std::optional<parser::ScalarIntExpr>>(x.t)}) {
         RequiresPositiveParameter(
             llvm::omp::Clause::OMPC_schedule, *chunkExpr, "chunk size");
+
+        if (kind == parser::OmpScheduleClause::ScheduleType::Static) {
+          if (const auto cv{GetIntValue(chunkExpr)}) {
+            const auto cs{GetChunkSize()};
+            if (!*cs) {
+              SetChunkSize(*chunkExpr);
+            } else if (cs != cv) {
+              context_.Say(GetContext().clauseSource,
+                  "The value of chunk size in SCHEDULE clause must"
+                  " be the same for all threads."_err_en_US);
+            }
+          }
+        }
       }
     }
 
     if (ScheduleModifierHasType(
             x, parser::OmpScheduleModifierType::ModType::Nonmonotonic)) {
       if (kind != parser::OmpScheduleClause::ScheduleType::Dynamic &&
           kind != parser::OmpScheduleClause::ScheduleType::Guided) {
         context_.Say(GetContext().clauseSource,

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

+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
+! 2.7.1 Thread Chunk Size - Schedule Clause
+
+program omp_doschedule
+  integer,parameter :: N = 14
+  integer :: i
+  integer :: sumx
+  real :: B(N), C(N)
+
+  !$omp parallel
+  !$omp  do schedule(static, 4)
+  do i = 1,N
+     sumx = sumx + B(i) * C(i)
+  end do
+
+  !ERROR: The value of chunk size in SCHEDULE clause must be the same for all threads.
+  !$omp  do schedule(static, 8)
+  do i = 1,N
+      sumx = sumx + B(i) * C(i)
+  end do
+
+  !$omp end do
+  !$omp end parallel
+
+end program omp_doschedule

flang/test/Semantics/omp-schedule-teams02.f90

+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
+! 2.7.1 Thread Chunk Size - Schedule Clause , a positive case
+
+program omp_doschedule
+
+  !DEF: /omp_doschedule/n PARAMETER ObjectEntity INTEGER(4)
+  integer, parameter :: n = 14
+  !DEF: /omp_doschedule/i ObjectEntity INTEGER(4)
+  integer i
+  !DEF: /omp_doschedule/sumx ObjectEntity INTEGER(4)
+  integer sumx
+  !DEF: /omp_doschedule/b ObjectEntity REAL(4)
+  !REF: /omp_doschedule/n
+  !DEF: /omp_doschedule/c ObjectEntity REAL(4)
+  real b(n), c(n)
+  !$omp parallel
+  !$omp do  schedule(static,4)
+  !DEF: /omp_doschedule/Block1/Block1/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+  !REF: /omp_doschedule/n
+  do i=1,n
+    !REF: /omp_doschedule/sumx
+    !REF: /omp_doschedule/b
+    !REF: /omp_doschedule/Block1/Block1/i
+    !REF: /omp_doschedule/c
+    sumx = sumx+b(i)*c(i)
+  end do
+  !$omp do  schedule(static,4)
+  !DEF: /omp_doschedule/Block1/Block2/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+  !REF: /omp_doschedule/n
+  do i=1,n
+    !REF: /omp_doschedule/sumx
+    !REF: /omp_doschedule/b
+    !REF: /omp_doschedule/Block1/Block2/i
+    !REF: /omp_doschedule/c
+    sumx = sumx+b(i)*c(i)
+  end do
+  !$omp end do
+  !$omp end parallel
+
+end program omp_doschedule