[PATCH] D91839: [flang][openmp] Separate memory-order-clause parser creating OmpClause node

[sameeran joshi via Phabricator via llvm-commits]

sameeranjoshi accepted this revision.
sameeranjoshi added a comment.
This revision is now accepted and ready to land.

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 ?



================
Comment at: flang/lib/Parser/openmp-parsers.cpp:311
+//                               relaxed
+TYPE_PARSER(sourced(construct<OmpMemoryOrderClause>(
+    sourced("SEQ_CST" >> construct<OmpClause>(construct<OmpClause::SeqCst>()) ||
----------------
clementval wrote:
> sameeranjoshi wrote:
> > 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` ?
> I think it would be nicer to have a single `OmpMemoryOrderClause` parser that is later reference in th atomic clause as well.  
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)))))
)))
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91839/new/

https://reviews.llvm.org/D91839