Page MenuHomePhabricator

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

Authored by yhegde on Oct 16 2020, 5:56 AM.

Details

Summary

Semantic check for OpenMP 4.5 - 2.7.1 schedule clause

Test cases : omp-do-schedule.f90

Diff Detail

Event Timeline

yhegde created this revision.Oct 16 2020, 5:56 AM
yhegde requested review of this revision.Oct 16 2020, 5:56 AM

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)
  1. 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
588

Nit: integer expression?

sameeranjoshi added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
582–592

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

clementval added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
582–592

+1

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

yhegde updated this revision to Diff 300255.Oct 23 2020, 5:58 AM
  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.
  1. 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 updated this revision to Diff 301025.Oct 27 2020, 9:08 AM

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

clementval added inline comments.Oct 28 2020, 7:37 AM
flang/lib/Semantics/check-omp-structure.cpp
584

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

yhegde updated this revision to Diff 301813.Oct 29 2020, 9:48 PM

Clause name changed to upper case.

kiranchandramohan accepted this revision.Mon, Nov 2, 2:30 PM

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

flang/lib/Semantics/check-directive-structure.h
465–473

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()))

This revision is now accepted and ready to land.Mon, Nov 2, 2:30 PM
clementval added inline comments.Tue, Nov 3, 8:20 AM
flang/test/Semantics/omp-do-schedule.f90
7 ↗(On Diff #301813)

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

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

Same here for the clause in uppercase.

yhegde added a comment.Wed, Nov 4, 6:20 AM

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 ?

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

yhegde updated this revision to Diff 303694.Sat, Nov 7, 9:04 PM
  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 accepted this revision.Sun, Nov 8, 4:00 PM

OK. LGTM.

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