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

[sameeran joshi via Phabricator via llvm-commits]

sameeranjoshi added a comment.

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:



================
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 ->
----------------
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
```


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91839