Page MenuHomePhabricator

[flang][openmp] Separate memory-order-clause parser creating OmpClause node
ClosedPublic

Authored by clementval on Thu, Nov 19, 7:47 PM.

Details

Summary

This patch introduce the separate parser for the memory-order-clause from the general
OmpClauseList. This parser still creates OmpClause node and therefore can use all the feature
from TableGen and the OmpStructureChecker.
This is applied only for the Flush construct in this patch and it should be applied for
atomic as well.

This is the approach we disscussed several time during the weekly call.

Diff Detail

Event Timeline

clementval created this revision.Thu, Nov 19, 7:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Nov 19, 7:47 PM
clementval requested review of this revision.Thu, Nov 19, 7:47 PM

Remove commented line

Harbormaster completed remote builds in B79559: Diff 306587.
clementval added a project: Restricted Project.Fri, Nov 20, 4:45 AM

Thanks @clementval.
I was not able to come up with this approach and sorry that I couldn't understand this since many days and in spite of many pointers to the same time to time.
A few queries inline:

flang/lib/Parser/openmp-parsers.cpp
305–308

Possibly a typo in index reference from standard.
My copy of OMP 5.0 standard mentions

2.17.7 atomic construct
2.17.8 flush construct
312

Instead of defining OmpMemoryOrderClause and referencing it inside the OpenMPFlushConstruct, what if you change OmpMemoryOrderClause to OmpFlushMemoryOrderClause?

TYPE_PARSER(sourced(construct<OmpFlushMemoryOrderClause>(
    sourced("ACQ_REL" >> construct<OmpClause>(construct<OmpClause::AcqRel>()) ||
        "RELEASE" >> construct<OmpClause>(construct<OmpClause::Release>()) ||
        "ACQUIRE" >> construct<OmpClause>(construct<OmpClause::Acquire>())))))

IIUC, standard defines memory-order-clause and inside flush-construct uses only 3 clauses out of all 5 memory-order-clauses, so if we model it as OmpFlushMemoryOrderClause that's making the parser more stricter which would very strictly align with the standard.

Going ahead we can have only OmpFlushMemoryOrderClause and OmpAtomicMemoryOrderClause, do you see any issues with this going ahead, hence you might have used OmpMemoryOrderClause ?

clementval added inline comments.Fri, Nov 20, 10:14 AM
flang/lib/Parser/openmp-parsers.cpp
305–308

Right I'll update that

312

I think it would be nicer to have a single OmpMemoryOrderClause parser that is later reference in th atomic clause as well.

Fix spec references in comment

sameeranjoshi accepted this revision.Sat, Nov 21, 2:00 AM

Thank you for the great work.
With this patch we can preserve the representation from what standard says as well use the TableGen infrastructure and do the semantic checks without reinventing the wheels.
@kiranchandramohan does this seem ok from query which was raised in https://reviews.llvm.org/D89583#inline-836220 comment ?

flang/lib/Parser/openmp-parsers.cpp
312

Agreed.
That just creates too many parsers nodes and would become hard to maintain later.

I will reuse OmpMemoryOrderClause for OmpAtomicClause node.
Specifically below parser node

TYPE_PARSER(sourced(construct<OmpAtomicClause>(
    sourced(construct<OmpAtomicClause>(Parser<OmpMemoryOrderClause>{})) ||
    sourced(construct<OmpAtomicClause>("HINT" >> construct<OmpClause>(construct<OmpClause::Hint>(parenthesized(constantExpr)))))
)))
This revision is now accepted and ready to land.Sat, Nov 21, 2:00 AM

@kiranchandramohan does this seem ok from query which was raised in https://reviews.llvm.org/D89583#inline-836220 comment ?

Thanks @clementval, @sameeranjoshi LGTM.

flang/lib/Parser/openmp-parsers.cpp
305–308

Nit: Atomtic -> Atomic

This revision was landed with ongoing or failed builds.Sat, Nov 21, 11:31 AM
This revision was automatically updated to reflect the committed changes.