This is an archive of the discontinued LLVM Phabricator instance.

[Flang][OpenMP 4.5] Add semantic check for OpenMP ordered and collapse clause
ClosedPublic

Authored by yhegde on Oct 21 2020, 2:22 AM.

Details

Summary

Semantic check for OpenMP 4.5 - 2.7.1 ordered and collapse clause

File:

  1. resolve-directives.cpp CHECK(level == 0); is commented ; because semantic checks are invoked after this function is called, so the check will fail before checking the semantic errors.

Test cases:

  1. omp-do-ordered.f90

2.omp-do-collapse.f90

Diff Detail

Event Timeline

yhegde created this revision.Oct 21 2020, 2:22 AM
yhegde requested review of this revision.Oct 21 2020, 2:22 AM

Thanks for this patch. A few comments inline.

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

OmpClause should ideally be inside the OmpAttributeVistor. Is there any specific reason it should be here?

flang/test/Semantics/omp-do-collapse.f90
5

Can you add the following tests?

  1. A positive test.
  2. A positive test with more nested loops than the collapse value.
  3. A negative test where there are loops enclosing the omp do loop with collapse.
clementval requested changes to this revision.Oct 21 2020, 11:22 AM
clementval added a subscriber: clementval.
clementval added inline comments.
flang/lib/Semantics/resolve-directives.cpp
45

OmpClause should not be here. If this is intended to be shared with OpenACC it should be templated.

76

Same here for OmpClause in generic part of DirectiveAttributeVisitor

79

Same here for OmpClause in generic part of DirectiveAttributeVisitor

This revision now requires changes to proceed.Oct 21 2020, 11:22 AM
yhegde updated this revision to Diff 300747.Oct 26 2020, 11:29 AM
  1. More positive and negative test cases are added.
  2. Code moved to OmpAttributeVisitor.

A few questions. Otherwise LGTM.

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

Can this function return a pair of std::int64_t parser::OmpClause *?

858

Nit: Can this function return a pair? Question because the name of the function is Get*.

904–913

You can remove this commented code.

905–912

Nit: This function's name (PrivatizeAssociatedLoopIndex) does not suggest that this kind of check happens here. Is it possible to pull this out or do it at a different place? Any strong reasons why it should be here?

yhegde updated this revision to Diff 303427.Nov 6 2020, 6:26 AM
  1. From PrivatizeAssociatedLoopIndex function, the loop level checking code for ordered and collapse clause  is moved to CheckAssocLoopLevel function.

"Is it possible to pull this out or do it at a different place?"

Since CHECK(level == 0) is removed, the semantic checks can be done in check-omp-structure.cpp. Once again we can go thro the DoConstruct to get the loop level and can have respective checks in OMPC_collapse and OMPC_ordered. Is that what you are suggesting ?!! @kiranchandramohan

  1. Clause names are changed to uppercase.
  1. "Nit: Can this function return a pair? Question because the name of the function is Get*."

Not done.
It can return a pair of level and OmpClause (probably to avoid Set and GetAssociatedClause,) but to invoke this function from elsewhere, we require OmpClauseList as an input parameter , if my understanding is correct. @kiranchandramohan

kiranchandramohan accepted this revision.Nov 8 2020, 4:02 PM

OK. LGTM.

flang/lib/Semantics/resolve-directives.cpp
874–875

Nit: remove this todo.

875

Nit: rename to PrivatizeLoopIndexAndCheckLoopLevel or a better name.

clementval accepted this revision.Nov 9 2020, 12:40 PM
This revision is now accepted and ready to land.Nov 9 2020, 12:40 PM

@yhegde can this be submitted? If you require inputs from me please let me know.

yhegde updated this revision to Diff 305026.Nov 12 2020, 9:53 PM

Thanks for reviewing.
Function name PrivatizeAssociatedLoopIndex is changed to PrivatizeAssociatedLoopIndexAndCheckLoopLevel.

yhegde marked an inline comment as done.Nov 21 2020, 10:45 AM
This comment was removed by yhegde.
flang/lib/Semantics/resolve-directives.cpp
875

Nit: rename to PrivatizeLoopIndexAndCheckLoopLevel or a better name.

Thank you very much @kiranchandramohan and @clementval for all the review comments.
Shall I change the name of the function to CheckClauseAssocLoopIndexandLoopLevel ?

Thank you once again.

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

This is fine. Please go ahead and submit.

Fine for me too