[Flang][OpenMP 4.5] Add semantic check for OpenMP schedule clause

[yhegde]

Summary

Semantic check for OpenMP 4.5 - 2.7.1 schedule clause

Test cases : omp-do-schedule.f90

[kiranchandramohan]

Thanks for this patch.

If I understand correctly all of the cases cannot be caught since the standard does not require it to be a constant expression. Can you add a few more tests which

1. Catches constant expressions, for eg:

 integer, parameter :: xx = 1
 integer, parameter :: yy = 1
!$omp do schedule(STATIC, xx - yy)

2. Do not catch non-constant expressions

subroutine sb(xx, yy)
  integer :: xx
  integer :: yy
 !$omp do schedule(STATIC, xx - yy)

 integer :: xx
!$omp do schedule(STATIC, xx - xx)

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

613

Nit: integer expression?

[sameeranjoshi]

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

607–617

Can't this be replaced with RequiresPositiveParameter from check-directive-structure.h ?

[clementval]

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

607–617

+1

Would be good to use what is inside check-directive-structure.h

[yhegde]

1. Three more test cases are added.
2. RequiresPositiveParameter() function is not called, because with the custom implementation we can give specific error message pointing to invalid chunk size. If we use that function we will end up in throwing generic error message.

[clementval]

In D89546#2349857, @yhegde wrote:

2. RequiresPositiveParameter() function is not called, because with the custom implementation we can give specific error message pointing to invalid chunk size. If we use that function we will end up in throwing generic error message.

Fair enough. What about modifying RequiresPositiveParameter() to take optional custom error message so we can share the code?

[yhegde]

RequiresPositiveParameter() is modified to take optional custom error message.

[clementval]

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

609

The clause and directive name is usually in upper case in the error message.

[yhegde]

Clause name changed to upper case.

[kiranchandramohan]

LGTM. Have a nit comment inline about how to construct the message.

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

376–387

Nits:

1. Can msg be a String/StringRef with name paramName?
2. Can paramName be non-optional? Set it to "" at other places.
3. The error message can possibly be constructed like.

"The %s of the %s clause must be "
"a positive integer expression"_err_en_US,
paramName.str(),
parser::ToUpperCaseLetters(getClauseName(clause).str()))

[clementval]

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

7

I guess you have to make the clause uppercase here as well.

flang/test/Semantics/omp-do-schedule02.f90

9

Same here for the clause in uppercase.

[yhegde]

If RPP () function constructed like this
void RequiresPositiveParameter(

const C &clause, const parser::ScalarIntExpr &i, llvm::StringRef paramName="parameter");

It wont break the test cases. Otherwise suggested change requires fixes in ERROR messages of previous test cases also. Shall I go ahead with this ?

In D89546#2369707, @kiranchandramohan wrote:

LGTM. Have a nit comment inline about how to construct the message.

[yhegde]

1. Clause names are changed to uppercase in test cases.
2. An optional StringRef paramName is introduced. Non optional paramName would break the previous test cases.

[kiranchandramohan]

OK. LGTM.

Nit: Please reply inline and mark suggested changes as done.

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

protected:
   void CheckNotAllowedIfClause(
       C clause, common::EnumSet<C, ClauseEnumSize> set);
 
   std::string ContextDirectiveAsFortran();
 
   void RequiresConstantPositiveParameter(
       const C &clause, const parser::ScalarIntConstantExpr &i);
 
-  void RequiresPositiveParameter(
-      const C &clause, const parser::ScalarIntExpr &i);
+  void RequiresPositiveParameter(const C &clause,
+      const parser::ScalarIntExpr &i, parser::MessageFixedText msg = {"", 0});
 
   void OptionalConstantPositiveParameter(
       const C &clause, const std::optional<parser::ScalarIntConstantExpr> &o);
 
   virtual llvm::StringRef getClauseName(C clause) { return ""; };
 
   virtual llvm::StringRef getDirectiveName(D directive) { return ""; };
 
context_.Say(GetContext().directiveSource,
         ContextDirectiveAsFortran());
   }
 }
 
 // Check the value of the clause is a positive parameter.
 template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
 void DirectiveStructureChecker<D, C, PC,
     ClauseEnumSize>::RequiresPositiveParameter(const C &clause,
-    const parser::ScalarIntExpr &i) {
+    const parser::ScalarIntExpr &i, parser::MessageFixedText msg) {
   if (const auto v{GetIntValue(i)}) {
     if (*v <= 0) {
+      if (msg.text().empty()) {
         context_.Say(GetContext().clauseSource,
             "The parameter of the %s clause must be "
             "a positive integer expression"_err_en_US,
             parser::ToUpperCaseLetters(getClauseName(clause).str()));
+      } else {
+        context_.Say(GetContext().clauseSource, std::move(msg),
+            parser::ToUpperCaseLetters(getClauseName(clause).str()));
+      }
     }
   }
 }
 
 } // namespace Fortran::semantics
 
 #endif // FORTRAN_SEMANTICS_CHECK_DIRECTIVE_STRUCTURE_H_

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

if (chunk) {
       if (kind == parser::OmpScheduleClause::ScheduleType::Runtime ||
           kind == parser::OmpScheduleClause::ScheduleType::Auto) {
         context_.Say(GetContext().clauseSource,
             "When SCHEDULE clause has %s specified, "
             "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,
+            "The chunk size of the SCHEDULE clause must be"
+            " a positive integer expression."_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,
             "The NONMONOTONIC modifier can only be specified with "
             "SCHEDULE(DYNAMIC) or SCHEDULE(GUIDED)"_err_en_US);
       }
     }
   }
 }
 

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

+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
+! 2.7.1 Schedule Clause
+program omp_doSchedule
+  integer :: i,n
+  real ::  a(100), y(100), z(100)
+  !ERROR: The chunk size of the schedule clause must be a positive integer expression.
+  !$omp do schedule(static, -1)
+  do i=2,n+1
+    y(i) = z(i-1) + a(i)
+  end do
+  !$omp end do
+end program omp_doSchedule

flang/test/Semantics/omp-do-schedule02.f90

+!RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
+! 2.7.1 Schedule Clause
+program omp_doSchedule
+  integer :: i,n
+  real ::  a(100), y(100), z(100)
+  integer,parameter :: b = 10
+  integer,parameter :: c = 11
+  !ERROR: The chunk size of the schedule clause must be a positive integer expression.
+  !$omp do schedule(static,b-c)
+  do i=2,n+1
+    y(i) = z(i-1) + a(i)
+  end do
+  !$omp end do
+end program omp_doSchedule

flang/test/Semantics/omp-do-schedule03.f90

+! RUN: %S/test_symbols.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
+! 2.7.1 Schedule Clause
+! Test that does not catch non constant integer expressions like xx - xx.
+  !DEF: /ompdoschedule MainProgram
+ program ompdoschedule
+  !DEF: /ompdoschedule/a ObjectEntity REAL(4)
+  !DEF: /ompdoschedule/y ObjectEntity REAL(4)
+  !DEF: /ompdoschedule/z ObjectEntity REAL(4)
+  real  a(100),y(100),z(100)
+  !DEF: /ompdoschedule/b ObjectEntity INTEGER(4)
+  !DEF: /ompdoschedule/i ObjectEntity INTEGER(4)
+  !DEF: /ompdoschedule/n ObjectEntity INTEGER(4)
+  integer  b,i,n
+  !REF: /ompdoschedule/b
+  b = 10
+!$omp do  schedule(static,b-b)
+ !DEF: /ompdoschedule/Block1/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
+ !REF: /ompdoschedule/n
+  do i = 2,n+1
+  !REF: /ompdoschedule/y
+  !REF: /ompdoschedule/Block1/i
+  !REF: /ompdoschedule/z
+  !REF: /ompdoschedule/a
+    y(i) = z(i-1) + a(i)
+  end do
+ !$omp end do
+end program ompdoschedule

flang/test/Semantics/omp-do-schedule04.f90

+! RUN: %S/test_symbols.sh %s %t %f18 -fopenmp
+! OpenMP Version 4.5
+! 2.7.1 Schedule Clause
+! Test that does not catch non constant integer expressions like xx - yy.
+
+  !DEF: /omp_doschedule (Subroutine) Subprogram
+subroutine omp_doschedule
+  implicit none
+  !DEF: /omp_doschedule/a ObjectEntity REAL(4)
+  !DEF: /omp_doschedule/y ObjectEntity REAL(4)
+  !DEF: /omp_doschedule/z ObjectEntity REAL(4)
+  real a(100),y(100),z(100)
+  !DEF: /omp_doschedule/i ObjectEntity INTEGER(4)
+  !DEF: /omp_doschedule/j ObjectEntity INTEGER(4)
+  !DEF: /omp_doschedule/k ObjectEntity INTEGER(4)
+  integer i,j,k
+
+  !REF: /omp_doschedule/j
+  j = 11
+  !REF: /omp_doschedule/k
+  k = 12
+!$omp do  schedule(static,j-k)
+ !DEF: /omp_doschedule/Block1/i (OmpPrivate,OmpPreDetermined) HostAssoc INTEGER(4)
+  do i = 1,10
+  !REF: /omp_doschedule/y
+  !REF: /omp_doschedule/Block1/i
+  !REF: /omp_doschedule/z
+  !REF: /omp_doschedule/a
+    y(i) = z(i-1)+a(i)
+  end do
+ !$omp end do
+end subroutine omp_doschedule