Page MenuHomePhabricator

[Flang][OpenMP-5.0] Semantic checks for flush construct.
AcceptedPublic

Authored by sameeranjoshi on Oct 21 2020, 6:27 AM.

Details

Summary

From OMP 5.0 [2.17.8]
Restriction:
If memory-order-clause is release,acquire, or acq_rel, list items must not be specified on the flush directive.

Diff Detail

Event Timeline

sameeranjoshi created this revision.Oct 21 2020, 6:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2020, 6:27 AM
sameeranjoshi requested review of this revision.Oct 21 2020, 6:27 AM
sameeranjoshi added a project: Restricted Project.Oct 21 2020, 6:29 AM

For a test as below

  TYPE somestr
    real :: rr
  end type
  TYPE(somestr) :: structObj

!$omp flush (structObj%rr)

I could see nag, intel and gfortran producing error when a list item is structObj%rr.
I didn't find inside standard though.
So should this be flagged as well an error?

Rebased on top of master to try to resolve buildbot failure.
https://reviews.llvm.org/harbormaster/build/99902/

clementval added inline comments.Oct 21 2020, 11:29 AM
llvm/include/llvm/Frontend/OpenMP/OMP.td
448

Did you check if clang test are still passing with this change?

I usually don't build clang.
I need to try.

sameeranjoshi added inline comments.Oct 22 2020, 7:29 AM
llvm/include/llvm/Frontend/OpenMP/OMP.td
448

With ninja check-clang check-flang all seems well.
I can't figure out why bots failed in applying the patch.
@Reviewers can someone conform if this patch applies and build and tests well?

clementval added inline comments.Oct 22 2020, 8:26 AM
llvm/include/llvm/Frontend/OpenMP/OMP.td
448

Which bot do you see failing?

sameeranjoshi added inline comments.Oct 27 2020, 7:51 AM
llvm/include/llvm/Frontend/OpenMP/OMP.td
448
clementval added inline comments.Oct 27 2020, 8:40 AM
llvm/include/llvm/Frontend/OpenMP/OMP.td
448

Looks like it a conflict problem. You should rebase one more time.

Rebase on master.

This commit tries to get a workaround for not chaning the parser as well as adding semantic checks.
The function FindClause was used earlier to get information from context.
Above function works on clauses which come under parser::OmpClause, currently flush doesn't fall under it.

sameeranjoshi retitled this revision from [Flang][OpenMP] Parsing and semantic changes for flush construct from OMP 5.0 specification. to [Flang][OpenMP-5.0] Semantic checks for flush construct..Nov 2 2020, 10:35 AM
sameeranjoshi edited the summary of this revision. (Show Details)
clementval requested changes to this revision.Nov 3 2020, 12:10 PM
clementval added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
552

It makes little sense to use the OmpClause infrastructure to check something enforced by the parser.

flang/test/Semantics/omp-flush.f90
36 ↗(On Diff #302333)

Any reason to have upper and lower cases?

Same questions for the others below

flang/test/Semantics/omp-flush01.f90
17

Any reason to have upper and lower cases?

Same questions for the others below

llvm/include/llvm/Frontend/OpenMP/OMP.td
448

Since you are not using the OmpClause this change is not needed here.

This revision now requires changes to proceed.Nov 3 2020, 12:10 PM
sameeranjoshi edited the summary of this revision. (Show Details)

Based on top of the parser changes which were handled in D91839.
Rebased after D91839.
Address review comments.

sameeranjoshi marked 7 inline comments as done.Sat, Nov 21, 7:10 PM

Some small comments

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

Isn't this line exceeding the character limit?

flang/test/Semantics/omp-flush01.f90
29

Why is the error reported twice here? Would it not be nicer to have a list of memory-order-clause and then the error being reported by the semantic check? I mean it would be more user friendly.

sameeranjoshi marked an inline comment as done.

Work on review comments.

sameeranjoshi marked an inline comment as done.Wed, Nov 25, 10:26 PM
sameeranjoshi added inline comments.
flang/test/Semantics/omp-flush01.f90
29

It's a known issue due the improper error recovery in openmp.
omp-atomic.f90 file too had to get a workaround using the same way.

Yes, I have forwarded the errors to semantic phase.

clementval added inline comments.Mon, Nov 30, 11:17 AM
flang/test/Semantics/omp-flush01.f90
29

If you have forwarded the errors to semantic then why is there still two syntax error lines?

sameeranjoshi marked an inline comment as done.Mon, Nov 30, 11:28 AM
sameeranjoshi added inline comments.
flang/test/Semantics/omp-flush01.f90
29

The errors forwarded to semantics are something like
At most one XYZ clause can appear on the FLUSH directive.
Generally the ones which don't have clauses apart from memory-order-clause.

The test cases over here have clauses which don't fall under memory-order-clause, hence the parser becomes very stricter over here and catches them in parsing stage.

clementval accepted this revision.Mon, Nov 30, 11:43 AM

LGTM. Please wait for the approval from someone from the OpenMP side.

flang/test/Semantics/omp-flush01.f90
29

Ok I see. It would be nice to fix the double reporting of error here but I guess it can be done in a separate patch. Can you add an entry in the progress document to not forget this issue.

This revision is now accepted and ready to land.Mon, Nov 30, 11:43 AM

Thank you!

flang/test/Semantics/omp-flush01.f90
29

Kiran Kumar from AMD was looking into improving the error messages for OMP parts.