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

[clementval]

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.

[clementval]

Remove commented line

[sameeranjoshi]

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

304

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

311

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]

flang/lib/Parser/openmp-parsers.cpp

304

Right I'll update that

311

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

[clementval]

Fix spec references in comment

[sameeranjoshi]

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

311

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)))))
)))

[kiranchandramohan]

@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

304–307

Nit: Atomtic -> Atomic

flang/include/flang/Parser/dump-parse-tree.h

#include "llvm/Frontend/OpenMP/OMP.cpp.inc"
   NODE(OpenMPCancelConstruct, If)
   NODE(parser, OpenMPCancellationPointConstruct)
   NODE(parser, OpenMPConstruct)
   NODE(parser, OpenMPCriticalConstruct)
   NODE(parser, OpenMPDeclarativeConstruct)
   NODE(parser, OpenMPDeclareReductionConstruct)
   NODE(parser, OpenMPDeclareSimdConstruct)
   NODE(parser, OpenMPDeclareTargetConstruct)
-  NODE(parser, OmpFlushMemoryClause)
+  NODE(parser, OmpMemoryOrderClause)
   NODE(parser, OpenMPFlushConstruct)
   NODE(parser, OpenMPLoopConstruct)
   NODE(parser, OpenMPSimpleStandaloneConstruct)
   NODE(parser, OpenMPStandaloneConstruct)
   NODE(parser, OpenMPSectionsConstruct)
   NODE(parser, OpenMPThreadprivate)
   NODE(parser, OpenStmt)
   NODE(parser, Optional)

flang/include/flang/Parser/parse-tree.h

struct OpenMPCancelConstruct {
   CharBlock source;
   std::tuple<Verbatim, OmpCancelType, std::optional<If>> t;
 };
 
 // 2.17.8 Flush Construct [OpenMP 5.0]
 // memory-order-clause -> acq_rel
 //                        release
 //                        acquire
-struct OmpFlushMemoryClause {
-  WRAPPER_CLASS_BOILERPLATE(OmpFlushMemoryClause, llvm::omp::Clause);
+struct OmpMemoryOrderClause {
+  WRAPPER_CLASS_BOILERPLATE(OmpMemoryOrderClause, OmpClause);
   CharBlock source;
 };
 
 // 2.17.8 flush -> FLUSH [memory-order-clause] [(variable-name-list)]
 struct OpenMPFlushConstruct {
   TUPLE_CLASS_BOILERPLATE(OpenMPFlushConstruct);
   CharBlock source;
-  std::tuple<Verbatim, std::optional<OmpFlushMemoryClause>,
+  std::tuple<Verbatim, std::optional<OmpMemoryOrderClause>,
       std::optional<OmpObjectList>>
       t;
 };
 
 struct OmpSimpleStandaloneDirective {
   WRAPPER_CLASS_BOILERPLATE(OmpSimpleStandaloneDirective, llvm::omp::Directive);
   CharBlock source;
 };

flang/lib/Lower/OpenMP.cpp

std::visit(
             genOMP(converter, eval, simpleStandaloneConstruct);
           },
           [&](const Fortran::parser::OpenMPFlushConstruct &flushConstruct) {
             SmallVector<Value, 4> operandRange;
             if (const auto &ompObjectList =
                     std::get<std::optional<Fortran::parser::OmpObjectList>>(
                         flushConstruct.t))
               genObjectList(*ompObjectList, converter, operandRange);
-            if (std::get<std::optional<Fortran::parser::OmpFlushMemoryClause>>(
+            if (std::get<std::optional<Fortran::parser::OmpMemoryOrderClause>>(
                     flushConstruct.t))
-              TODO("Handle OmpFlushMemoryClause");
+              TODO("Handle OmpMemoryOrderClause");
             converter.getFirOpBuilder().create<mlir::omp::FlushOp>(
                 converter.getCurrentLocation(), operandRange);
           },
           [&](const Fortran::parser::OpenMPCancelConstruct &cancelConstruct) {
             TODO("");
           },
           [&](const Fortran::parser::OpenMPCancellationPointConstruct
                   &cancellationPointConstruct) { TODO(""); },

flang/lib/Parser/openmp-parsers.cpp

 // 2.14.2 Cancellation Point construct
 TYPE_PARSER(sourced(construct<OpenMPCancellationPointConstruct>(
     verbatim("CANCELLATION POINT"_tok), Parser<OmpCancelType>{})))
 
 // 2.14.1 Cancel construct
 TYPE_PARSER(sourced(construct<OpenMPCancelConstruct>(verbatim("CANCEL"_tok),
     Parser<OmpCancelType>{}, maybe("IF" >> parenthesized(scalarLogicalExpr)))))
 
-// 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 ->
+//                               seq_cst
+//                               acq_rel
 //                               release
 //                               acquire
-TYPE_PARSER(sourced(construct<OmpFlushMemoryClause>(
-    "ACQ_REL" >> pure(llvm::omp::Clause::OMPC_acq_rel) ||
-    "RELEASE" >> pure(llvm::omp::Clause::OMPC_release) ||
-    "ACQUIRE" >> pure(llvm::omp::Clause::OMPC_acquire))))
+//                               relaxed
+TYPE_PARSER(sourced(construct<OmpMemoryOrderClause>(
+    sourced("SEQ_CST" >> construct<OmpClause>(construct<OmpClause::SeqCst>()) ||
+        "ACQ_REL" >> construct<OmpClause>(construct<OmpClause::AcqRel>()) ||
+        "RELEASE" >> construct<OmpClause>(construct<OmpClause::Release>()) ||
+        "ACQUIRE" >> construct<OmpClause>(construct<OmpClause::Acquire>()) ||
+        "RELAXED" >> construct<OmpClause>(construct<OmpClause::Relaxed>())))))
 
 TYPE_PARSER(sourced(construct<OpenMPFlushConstruct>(verbatim("FLUSH"_tok),
-    maybe(Parser<OmpFlushMemoryClause>{}),
+    maybe(Parser<OmpMemoryOrderClause>{}),
     maybe(parenthesized(Parser<OmpObjectList>{})))))
 
 // Simple Standalone Directives
 TYPE_PARSER(sourced(construct<OmpSimpleStandaloneDirective>(first(
     "BARRIER" >> pure(llvm::omp::Directive::OMPD_barrier),
     "ORDERED" >> pure(llvm::omp::Directive::OMPD_ordered),
     "TARGET ENTER DATA" >> pure(llvm::omp::Directive::OMPD_target_enter_data),
     "TARGET EXIT DATA" >> pure(llvm::omp::Directive::OMPD_target_exit_data),

flang/lib/Parser/unparse.cpp

#include "llvm/Frontend/OpenMP/OMP.cpp.inc"
   void Unparse(const OpenMPCancelConstruct &x) {
     BeginOpenMP();
     Word("!$OMP CANCEL ");
     Walk(std::get<OmpCancelType>(x.t));
     Walk(std::get<std::optional<OpenMPCancelConstruct::If>>(x.t));
     Put("\n");
     EndOpenMP();
   }
-  void Unparse(const OmpFlushMemoryClause &x) {
-    switch (x.v) {
-    case llvm::omp::Clause::OMPC_acq_rel:
-      Word("ACQ_REL ");
-      break;
-    case llvm::omp::Clause::OMPC_release:
-      Word("RELEASE ");
-      break;
-    case llvm::omp::Clause::OMPC_acquire:
-      Word("ACQUIRE ");
-      break;
-    default:
-      break;
-    }
-  }
+  void Unparse(const OmpMemoryOrderClause &x) { Walk(x.v); }
   void Unparse(const OpenMPFlushConstruct &x) {
     BeginOpenMP();
     Word("!$OMP FLUSH ");
-    Walk(std::get<std::optional<OmpFlushMemoryClause>>(x.t));
+    Walk(std::get<std::optional<OmpMemoryOrderClause>>(x.t));
     Walk(" (", std::get<std::optional<OmpObjectList>>(x.t), ")");
     Put("\n");
     EndOpenMP();
   }
   void Unparse(const OmpEndLoopDirective &x) {
     BeginOpenMP();
     Word("!$OMP END ");
     Walk(std::get<OmpLoopDirective>(x.t));

flang/lib/Semantics/check-omp-structure.h

#include "llvm/Frontend/OpenMP/OMP.cpp.inc"
   void Enter(const parser::OmpClause::Shared &);
   void Enter(const parser::OmpClause::Simdlen &);
   void Enter(const parser::OmpClause::ThreadLimit &);
   void Enter(const parser::OmpClause::To &);
   void Enter(const parser::OmpClause::Link &);
   void Enter(const parser::OmpClause::Uniform &);
   void Enter(const parser::OmpClause::UseDevicePtr &);
   void Enter(const parser::OmpClause::IsDevicePtr &);
+  // Memory-order-clause
+  void Enter(const parser::OmpClause::SeqCst &);
+  void Enter(const parser::OmpClause::AcqRel &);
+  void Enter(const parser::OmpClause::Release &);
+  void Enter(const parser::OmpClause::Acquire &);
+  void Enter(const parser::OmpClause::Relaxed &);
 
   void Enter(const parser::OmpAlignedClause &);
   void Enter(const parser::OmpAllocateClause &);
   void Enter(const parser::OmpDefaultClause &);
   void Enter(const parser::OmpDefaultmapClause &);
   void Enter(const parser::OmpDependClause &);
   void Enter(const parser::OmpDistScheduleClause &);
   void Enter(const parser::OmpIfClause &);

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

 CHECK_SIMPLE_CLAUSE(Nogroup, OMPC_nogroup)
 CHECK_SIMPLE_CLAUSE(Notinbranch, OMPC_notinbranch)
 CHECK_SIMPLE_CLAUSE(Private, OMPC_private)
 CHECK_SIMPLE_CLAUSE(Shared, OMPC_shared)
 CHECK_SIMPLE_CLAUSE(To, OMPC_to)
 CHECK_SIMPLE_CLAUSE(Uniform, OMPC_uniform)
 CHECK_SIMPLE_CLAUSE(Untied, OMPC_untied)
 CHECK_SIMPLE_CLAUSE(UseDevicePtr, OMPC_use_device_ptr)
+CHECK_SIMPLE_CLAUSE(AcqRel, OMPC_acq_rel)
+CHECK_SIMPLE_CLAUSE(Acquire, OMPC_acquire)
+CHECK_SIMPLE_CLAUSE(SeqCst, OMPC_seq_cst)
+CHECK_SIMPLE_CLAUSE(Release, OMPC_release)
+CHECK_SIMPLE_CLAUSE(Relaxed, OMPC_relaxed)
 
 CHECK_REQ_SCALAR_INT_CLAUSE(Grainsize, OMPC_grainsize)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumTasks, OMPC_num_tasks)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumTeams, OMPC_num_teams)
 CHECK_REQ_SCALAR_INT_CLAUSE(NumThreads, OMPC_num_threads)
 CHECK_REQ_SCALAR_INT_CLAUSE(Priority, OMPC_priority)
 CHECK_REQ_SCALAR_INT_CLAUSE(ThreadLimit, OMPC_thread_limit)
 

flang/test/Semantics/omp-clause-validity01.f90

!                        allocate-clause
   enddo
   !$omp end parallel
 
   !$omp parallel allocate(xy_alloc :b) private(b)
   do i = 1, N
      a = 3.14
   enddo
   !$omp end parallel
-  
   !$omp task private(b) allocate(b)
   do i = 1, N
      z = 2
   end do
   !$omp end task
 
   !$omp teams private(b) allocate(b)
   do i = 1, N
      z = 2
   end do
   !$omp end teams
 
   !$omp target private(b) allocate(b)
   do i = 1, N
      z = 2
   end do
   !$omp end target
- 
   !ERROR: ALLOCATE clause is not allowed on the TARGET DATA directive
   !$omp target data map(from: b) allocate(b)
   do i = 1, N
      z = 2
   enddo
    !$omp end target data
 
   !ERROR: SCHEDULE clause is not allowed on the PARALLEL directive
! Standalone Directives (basic)
   !$omp ordered depend(source)
   !ERROR: Internal: no symbol found for 'i'
   !$omp ordered depend(sink:i-1)
   !$omp flush (c)
   !$omp flush acq_rel
   !$omp flush release
   !$omp flush acquire
   !$omp flush release (c)
+  !ERROR: SEQ_CST clause is not allowed on the FLUSH directive
+  !$omp flush seq_cst
+  !ERROR: RELAXED clause is not allowed on the FLUSH directive
+  !$omp flush relaxed
+
   !$omp cancel DO
   !$omp cancellation point parallel
 
 ! 2.13.2 critical Construct
 
   !ERROR: Internal: no symbol found for 'first'
   !$omp critical (first)
   a = 3.14