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

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
Comment at: flang/lib/Parser/openmp-parsers.cpp:304
 
-// 2.17.8 Flush construct [OpenMP 5.0]
-//        flush -> FLUSH [memory-order-clause] [(variable-name-list)]
-//        memory-order-clause -> acq_rel
+// 2.17.7 Flush construct/2.17.8 Atmoic construct [OpenMP 5.0]
+//        memory-order-clause ->
----------------
sameeranjoshi wrote:
> 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
> ```
Right I'll update that


================
Comment at: flang/lib/Parser/openmp-parsers.cpp:311
+//                               relaxed
+TYPE_PARSER(sourced(construct<OmpMemoryOrderClause>(
+    sourced("SEQ_CST" >> construct<OmpClause>(construct<OmpClause::SeqCst>()) ||
----------------
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.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91839