[Flang][OpenMP] Rework parser changes for OpenMP atomic construct.

[sameeranjoshi]

Summary

OmpStructureChecker is supposed to work only with parser::OmpClause
after tablegen changes for OpenMP and OpenACC were introduced.
Hence OmpMemoryOrderClause, OmpAtomicMemoryOrderClause and similar ones were failing
to catch semantic errors, inspite of having code for semantic checks.
This patch tries to change parser for OmpMemoryOrderClause and similar dependent ones
and use OmpClauseList which resides/comes from common tablegen for OpenMP/OpenACC eventually using parser::OmpClause.

This patch also tries to :

1. Change OmpCriticalDirective in openmp-parsers.cpp to support OmpClauseList.
2. Check-flang regresses when changes were introduced due to missing semantic checks in OmpCritical, patch implements them at the minimal level to pass the regression.
3. Change tablegen to support Hint clause.
4. Adds missing source locations CharBlock Source in each atomic construct.
5. Remove dead code realted to memory-order-clauses after moving to OmpClauseList.

[sameeranjoshi]

Rebased

[clementval]

Just to make things clear. The OmpStructureChecker can work with other specific clauses struct you introduce but of course if you want to leverage the current code that is in you have to use OmpClause. But in the Enter/Leave function you can perform any semantic check you want. See for example device_type Sema in OpenACC. It is specific so of course it doesn't fall into the TableGen automatic checks. So this is fine to make the change you did but you could have also implement these checks with specific code for OmpAtomicMemoryOrderClause. Anyway I think if the atomic construct fits with OmpClause this is a better approach since the implementation is used by all other clauses.

Just added small comment. The change seems fine to me but I didn't check if it is conform to the OpenMP spec.

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

3598

Can your remove the optional? Empty list should do the work ...

flang/lib/Parser/openmp-parsers.cpp

218

Remove new line.

[sameeranjoshi]

Review comments addressed.

[kiranchandramohan]

A few minor/nit comments. Could you add a few unparse tests in the test/Parser directory?

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

3598

Why is it an OmpClauseList and not OmpClause? It is just a hint clause right? So should this be std::optional<OmpClause>?

flang/lib/Parser/openmp-parsers.cpp

162

Nit: empty line?

214

Thanks for moving this to the correct place.

222

Nit: Empty Line

[sameeranjoshi]

1. Revert OmpClauseList back to std::optional<OmpClause>.

Sorry a copy paste typo.

2. Add unparse tests.

Why do we need unparser tests here, as unparsing would be removed at some point
in time? So should the tests be removed at that time.

[clementval]

2. Add unparse tests.

Why do we need unparser tests here, as unparsing would be removed at some point
in time? So should the tests be removed at that time.

Who said the unparse features will be removed?

[kiranchandramohan]

OmpStructureChecker is supposed to work only with parser::OmpClause
after tablegen changes for OpenMP and OpenACC were introduced.

I think this was not related to tablegen changes. The semantic checks
only worked with OmpClauses.

Why do we need unparser tests here, as unparsing would be removed at some point
in time? So should the tests be removed at that time.

Sorry, I was not aware that they are going to be removed. I guess we can keep it till then.
I would prefer keeping the unparsing since it provides a mechanism to 1) test the parser
and 2) do source to source translation.

BTW, are allowed clauses and all handled automatically or will they require a separate patch?

[clementval]

I would prefer keeping the unparsing since it provides a mechanism to 1) test the parser
and 2) do source to source translation.

+1 I thing the source-to-source translation will interest many people.

[sameeranjoshi]

Sorry, I was not aware that they are going to be removed. I guess we can keep it till then.
I would prefer keeping the unparsing since it provides a mechanism to 1) test the parser
and 2) do source to source translation.

Those are my personal understandings from how current f18 behaves by unparsing and passing it to an external compiler specified by F18_FC , I am too unaware if unparsing would be removed officially, but if projects which might interest people are possible it's ideal to retain that.

BTW, are allowed clauses and all handled automatically or will they require a separate patch?

I think semantic checks for atomic needs separate patch which might need to upgrade to OpenMP 5.0 changes.

[SouraVX]

flang/lib/Parser/unparse.cpp

2267

NIT: Can't we use the type name here ? get<0> does the job but makes the code less readable.

[sameeranjoshi]

flang/lib/Parser/unparse.cpp

2267

If you take a look at how parse-tree.h represents AtomicUpdate

// ATOMIC UPDATE
struct OmpAtomicUpdate {
  TUPLE_CLASS_BOILERPLATE(OmpAtomicUpdate);
  std::tuple<OmpClauseList, Verbatim,
      OmpClauseList, Statement<AssignmentStmt>,
      std::optional<OmpEndAtomic>>
      t;
};

When std::tuple contains more than 1 repeated types, there is ambiguity for compiler to choose the type.
This makes us use indices to make it explicit.

[kiranchandramohan]

BTW, are allowed clauses and all handled automatically or will they require a separate patch?

I think semantic checks for atomic needs separate patch which might need to upgrade to OpenMP 5.0 changes.

If semantic checks for 4.5 clauses don't automatically work then there is a regression since previously the parser correctly only allowed MemoryOrder Clauses. Now that we have generalized to OmpClause for Atomic, it might be better to add tests for semantic checks in this patch itself.

[SouraVX]

flang/lib/Parser/unparse.cpp

2267

Gotcha Thanks! Maybe get<0/*OmpClauseList*/>. No hard reservations, This may look not so good(Personal taste). Feel free to ignore otherwise :)

[sameeranjoshi]

In D88965#2321734, @kiranchandramohan wrote:

BTW, are allowed clauses and all handled automatically or will they require a separate patch?

I think semantic checks for atomic needs separate patch which might need to upgrade to OpenMP 5.0 changes.

If semantic checks for 4.5 clauses don't automatically work then there is a regression since previously the parser correctly only allowed MemoryOrder Clauses. Now that we have generalized to OmpClause for Atomic, it might be better to add tests for semantic checks in this patch itself.

There are no semantic checks for even 4.5 as well nor any tests.
It needs more work which I think should be better to come with a new patch specific to semantic checks.
Until then is that ok to merge this, as there is a dependency for semantics with this patch.

[kiranchandramohan]

@sameeranjoshi Is it incorrect to assume that the allowed clauses will work because it is handled by tablegen? Like for e.g. will num_threads be accepted as a clause with atomic?
https://github.com/llvm/llvm-project/blob/1687a8d83b702332c4aae4f2a95d27c16688418d/llvm/include/llvm/Frontend/OpenMP/OMP.td#L444

[sameeranjoshi]

What I understand from your query is, you are basically trying to ask if we are using OmpClauseList in parser-tree.h so should all the clauses be allowed on atomic construct, which is not expected and you are trying to find where are the restrictions on the various clauses from OmpClauseList on Atomic construct.

In D88965#2326874, @kiranchandramohan wrote:

@sameeranjoshi Is it incorrect to assume that the allowed clauses will work because it is handled by tablegen?

IIUC, it's partially correct.
The allowedClauses which are currently present in OMP.td will work in catching the test cases which don't obey what OMP.td has defined.
e.g for test cases with num_threads clause, which is not found in allowedClauses of def OMP_Atomic is caught in parsing stage.

!!$omp atomic num_threads read
    i = j

Above test is a syntax error.

Now coming to what standard 5.0 says for one of the constraints:

At most one memory-order-clause may appear on the construct.

So a test like

!$omp atomic SEQ_CST SEQ_CST read
     i  = j

Is currently not flagged as semantic error.
The possible reason which Valentine explained me was that "there should be at least a Enter function for each clauses with a CheckAllowed inside check-omp-structure.cpp".
And those functions lack in check-omp-structure.cpp which was my next patch.

Like for e.g. will num_threads be accepted as a clause with atomic?

no.

https://github.com/llvm/llvm-project/blob/1687a8d83b702332c4aae4f2a95d27c16688418d/llvm/include/llvm/Frontend/OpenMP/OMP.td#L444

please correct me If I am still missing something.

[kiranchandramohan]

IIUC, it's partially correct.
The allowedClauses which are currently present in OMP.td will work in catching the test cases which don't obey what OMP.td has defined.
e.g for test cases with num_threads clause, which is not found in allowedClauses of def OMP_Atomic is caught in parsing stage.

A test for this might be good with this patch itself.

Thanks for the replies. No further questions from my side. I have already approved.

[clementval]

Now coming to what standard 5.0 says for one of the constraints:

At most one memory-order-clause may appear on the construct.

So a test like

!$omp atomic SEQ_CST SEQ_CST read
     i  = j

Is currently not flagged as semantic error.
The possible reason which Valentine explained me was that "there should be at least a Enter function for each clauses with a CheckAllowed inside check-omp-structure.cpp".
And those functions lack in check-omp-structure.cpp which was my next patch.

This should be done with the allowedOnceClauses list in OMP.td nothing should be done in the check-omp-structure.cpp

[sameeranjoshi]

A test for this might be good with this patch itself.

Thank you..
Test added with the commit into LLVM trunk.

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

#include "llvm/Frontend/OpenACC/ACC.cpp.inc"
   NODE(parser, NullInit)
   NODE(parser, ObjectDecl)
   NODE(parser, OldParameterStmt)
   NODE(parser, OmpAlignedClause)
   NODE(parser, OmpAtomic)
   NODE(parser, OmpAtomicCapture)
   NODE(OmpAtomicCapture, Stmt1)
   NODE(OmpAtomicCapture, Stmt2)
-  NODE(parser, OmpAtomicMemoryOrderClause)
-  NODE(parser, OmpAtomicMemoryOrderClauseList)
-  NODE(parser, OmpAtomicMemoryOrderClausePostList)
   NODE(parser, OmpAtomicRead)
   NODE(parser, OmpAtomicUpdate)
   NODE(parser, OmpAtomicWrite)
   NODE(parser, OmpBeginBlockDirective)
   NODE(parser, OmpBeginLoopDirective)
   NODE(parser, OmpBeginSectionsDirective)
   NODE(parser, OmpBlockDirective)
   static std::string GetNodeName(const llvm::omp::Directive &x) {
#include "llvm/Frontend/OpenMP/OMP.cpp.inc"
   NODE(parser, OmpDependSinkVec)
   NODE(parser, OmpDependSinkVecLength)
   NODE(parser, OmpDistScheduleClause)
   NODE(parser, OmpEndAtomic)
   NODE(parser, OmpEndBlockDirective)
   NODE(parser, OmpEndCriticalDirective)
   NODE(parser, OmpEndLoopDirective)
   NODE(parser, OmpEndSectionsDirective)
-  NODE(parser, OmpHintExpr)
   NODE(parser, OmpIfClause)
   NODE_ENUM(OmpIfClause, DirectiveNameModifier)
   NODE(parser, OmpLinearClause)
   NODE(OmpLinearClause, WithModifier)
   NODE(OmpLinearClause, WithoutModifier)
   NODE(parser, OmpLinearModifier)
   NODE_ENUM(OmpLinearModifier, Type)
   NODE(parser, OmpLoopDirective)
   NODE(parser, OmpMapClause)
   NODE(parser, OmpMapType)
   NODE(OmpMapType, Always)
   NODE_ENUM(OmpMapType, Type)
-  NODE(parser, OmpMemoryOrderClause)
   static std::string GetNodeName(const llvm::omp::Clause &x) {
     return llvm::Twine(
         "llvm::omp::Clause = ", llvm::omp::getOpenMPClauseName(x))
         .str();
   }
   NODE(parser, OmpNowait)
   NODE(parser, OmpObject)
   NODE(parser, OmpObjectList)

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

 struct OpenMPDeclarativeConstruct {
   UNION_CLASS_BOILERPLATE(OpenMPDeclarativeConstruct);
   CharBlock source;
   std::variant<OpenMPDeclareReductionConstruct, OpenMPDeclareSimdConstruct,
       OpenMPDeclareTargetConstruct, OpenMPThreadprivate>
       u;
 };
 
-// HINT(hint-expression)
-WRAPPER_CLASS(OmpHintExpr, ConstantExpr);
-
 // 2.13.2 CRITICAL [Name] <block> END CRITICAL [Name]
 struct OmpCriticalDirective {
   TUPLE_CLASS_BOILERPLATE(OmpCriticalDirective);
   CharBlock source;
-  std::tuple<Verbatim, std::optional<Name>, std::optional<OmpHintExpr>> t;
+  std::tuple<Verbatim, std::optional<Name>, std::optional<OmpClause>> t;
 };
 struct OmpEndCriticalDirective {
   TUPLE_CLASS_BOILERPLATE(OmpEndCriticalDirective);
   CharBlock source;
   std::tuple<Verbatim, std::optional<Name>> t;
 };
 struct OpenMPCriticalConstruct {
   TUPLE_CLASS_BOILERPLATE(OpenMPCriticalConstruct);
   std::tuple<OmpCriticalDirective, Block, OmpEndCriticalDirective> t;
 };
 
 // 2.17.7 atomic -> ATOMIC [clause[,]] atomic-clause [[,]clause] |
 //                  ATOMIC [clause]
 //        clause -> memory-order-clause | HINT(hint-expression)
 //        memory-order-clause -> SEQ_CST | ACQ_REL | RELEASE | ACQUIRE | RELAXED
 //        atomic-clause -> READ | WRITE | UPDATE | CAPTURE
 
 // END ATOMIC
 EMPTY_CLASS(OmpEndAtomic);
 
-// Memory order clause
-struct OmpMemoryOrderClause {
-  WRAPPER_CLASS_BOILERPLATE(OmpMemoryOrderClause, llvm::omp::Clause);
-  CharBlock source;
-};
-
-// ATOMIC Memory order clause or hint expression
-struct OmpAtomicMemoryOrderClause {
-  UNION_CLASS_BOILERPLATE(OmpAtomicMemoryOrderClause);
-  std::variant<OmpMemoryOrderClause, OmpHintExpr> u;
-};
-
-WRAPPER_CLASS(
-    OmpAtomicMemoryOrderClauseList, std::list<OmpAtomicMemoryOrderClause>);
-WRAPPER_CLASS(
-    OmpAtomicMemoryOrderClausePostList, std::list<OmpAtomicMemoryOrderClause>);
-
 // ATOMIC READ
 struct OmpAtomicRead {
   TUPLE_CLASS_BOILERPLATE(OmpAtomicRead);
-  std::tuple<OmpAtomicMemoryOrderClauseList, Verbatim,
-      OmpAtomicMemoryOrderClausePostList, Statement<AssignmentStmt>,
+  CharBlock source;
+  std::tuple<OmpClauseList, Verbatim, OmpClauseList, Statement<AssignmentStmt>,
       std::optional<OmpEndAtomic>>
       t;
 };
 
 // ATOMIC WRITE
 struct OmpAtomicWrite {
   TUPLE_CLASS_BOILERPLATE(OmpAtomicWrite);
-  std::tuple<OmpAtomicMemoryOrderClauseList, Verbatim,
-      OmpAtomicMemoryOrderClausePostList, Statement<AssignmentStmt>,
+  CharBlock source;
+  std::tuple<OmpClauseList, Verbatim, OmpClauseList, Statement<AssignmentStmt>,
       std::optional<OmpEndAtomic>>
       t;
 };
 
 // ATOMIC UPDATE
 struct OmpAtomicUpdate {
   TUPLE_CLASS_BOILERPLATE(OmpAtomicUpdate);
-  std::tuple<OmpAtomicMemoryOrderClauseList, Verbatim,
-      OmpAtomicMemoryOrderClausePostList, Statement<AssignmentStmt>,
+  CharBlock source;
+  std::tuple<OmpClauseList, Verbatim, OmpClauseList, Statement<AssignmentStmt>,
       std::optional<OmpEndAtomic>>
       t;
 };
 
 // ATOMIC CAPTURE
 struct OmpAtomicCapture {
   TUPLE_CLASS_BOILERPLATE(OmpAtomicCapture);
+  CharBlock source;
   WRAPPER_CLASS(Stmt1, Statement<AssignmentStmt>);
   WRAPPER_CLASS(Stmt2, Statement<AssignmentStmt>);
-  std::tuple<OmpAtomicMemoryOrderClauseList, Verbatim,
-      OmpAtomicMemoryOrderClausePostList, Stmt1, Stmt2, OmpEndAtomic>
+  std::tuple<OmpClauseList, Verbatim, OmpClauseList, Stmt1, Stmt2, OmpEndAtomic>
       t;
 };
 
 // ATOMIC
 struct OmpAtomic {
   TUPLE_CLASS_BOILERPLATE(OmpAtomic);
-  std::tuple<Verbatim, OmpAtomicMemoryOrderClauseList,
-      Statement<AssignmentStmt>, std::optional<OmpEndAtomic>>
+  CharBlock source;
+  std::tuple<Verbatim, OmpClauseList, Statement<AssignmentStmt>,
+      std::optional<OmpEndAtomic>>
       t;
 };
 
 struct OpenMPAtomicConstruct {
   UNION_CLASS_BOILERPLATE(OpenMPAtomicConstruct);
   std::variant<OmpAtomicRead, OmpAtomicWrite, OmpAtomicCapture, OmpAtomicUpdate,
       OmpAtomic>
       u;

flang/lib/Parser/openmp-parsers.cpp

 
 // 2.8.1 ALIGNED (list: alignment)
 TYPE_PARSER(construct<OmpAlignedClause>(
     nonemptyList(name), maybe(":" >> scalarIntConstantExpr)))
 
 TYPE_PARSER(
     construct<OmpObject>(designator) || construct<OmpObject>("/" >> name / "/"))
 
-TYPE_PARSER("ALIGNED" >>
+TYPE_PARSER(
+    "ACQUIRE" >> construct<OmpClause>(construct<OmpClause::Acquire>()) ||
+    "ACQ_REL" >> construct<OmpClause>(construct<OmpClause::AcqRel>()) ||
+    "ALIGNED" >>
         construct<OmpClause>(parenthesized(Parser<OmpAlignedClause>{})) ||
+    "ALLOCATE" >>
+        construct<OmpClause>(parenthesized(Parser<OmpAllocateClause>{})) ||
     "COLLAPSE" >> construct<OmpClause>(construct<OmpClause::Collapse>(
                       parenthesized(scalarIntConstantExpr))) ||
     "COPYIN" >> construct<OmpClause>(construct<OmpClause::Copyin>(
                     parenthesized(Parser<OmpObjectList>{}))) ||
     "COPYPRIVATE" >> construct<OmpClause>(construct<OmpClause::Copyprivate>(
                          (parenthesized(Parser<OmpObjectList>{})))) ||
     "DEFAULT"_id >>
         construct<OmpClause>(parenthesized(Parser<OmpDefaultClause>{})) ||
     "DEFAULTMAP" >>
         construct<OmpClause>(parenthesized(Parser<OmpDefaultmapClause>{})) ||
     "DEPEND" >>
         construct<OmpClause>(parenthesized(Parser<OmpDependClause>{})) ||
     "DEVICE" >> construct<OmpClause>(construct<OmpClause::Device>(
                     parenthesized(scalarIntExpr))) ||
     "DIST_SCHEDULE" >>
         construct<OmpClause>(construct<OmpDistScheduleClause>(
             parenthesized("STATIC" >> maybe("," >> scalarIntExpr)))) ||
     "FINAL" >> construct<OmpClause>(construct<OmpClause::Final>(
                    parenthesized(scalarLogicalExpr))) ||
     "FIRSTPRIVATE" >> construct<OmpClause>(construct<OmpClause::Firstprivate>(
                           parenthesized(Parser<OmpObjectList>{}))) ||
     "FROM" >> construct<OmpClause>(construct<OmpClause::From>(
                   parenthesized(Parser<OmpObjectList>{}))) ||
     "GRAINSIZE" >> construct<OmpClause>(construct<OmpClause::Grainsize>(
                        parenthesized(scalarIntExpr))) ||
+    "HINT" >> construct<OmpClause>(parenthesized(constantExpr)) ||
     "IF" >> construct<OmpClause>(parenthesized(Parser<OmpIfClause>{})) ||
     "INBRANCH" >> construct<OmpClause>(construct<OmpClause::Inbranch>()) ||
     "IS_DEVICE_PTR" >> construct<OmpClause>(construct<OmpClause::IsDevicePtr>(
                            parenthesized(nonemptyList(name)))) ||
     "LASTPRIVATE" >> construct<OmpClause>(construct<OmpClause::Lastprivate>(
                          parenthesized(Parser<OmpObjectList>{}))) ||
     "LINEAR" >>
         construct<OmpClause>(parenthesized(Parser<OmpLinearClause>{})) ||
TYPE_PARSER(
     "ORDERED" >> construct<OmpClause>(construct<OmpClause::Ordered>(
                      maybe(parenthesized(scalarIntConstantExpr)))) ||
     "PRIORITY" >> construct<OmpClause>(construct<OmpClause::Priority>(
                       parenthesized(scalarIntExpr))) ||
     "PRIVATE" >> construct<OmpClause>(construct<OmpClause::Private>(
                      parenthesized(Parser<OmpObjectList>{}))) ||
     "PROC_BIND" >>
         construct<OmpClause>(parenthesized(Parser<OmpProcBindClause>{})) ||
     "REDUCTION" >>
         construct<OmpClause>(parenthesized(Parser<OmpReductionClause>{})) ||
-    "ALLOCATE" >>
-        construct<OmpClause>(parenthesized(Parser<OmpAllocateClause>{})) ||
+    "RELAXED" >> construct<OmpClause>(construct<OmpClause::Relaxed>()) ||
+    "RELEASE" >> construct<OmpClause>(construct<OmpClause::Release>()) ||
     "SAFELEN" >> construct<OmpClause>(construct<OmpClause::Safelen>(
                      parenthesized(scalarIntConstantExpr))) ||
     "SCHEDULE" >>
         construct<OmpClause>(parenthesized(Parser<OmpScheduleClause>{})) ||
+    "SEQ_CST" >> construct<OmpClause>(construct<OmpClause::SeqCst>()) ||
     "SHARED" >> construct<OmpClause>(construct<OmpClause::Shared>(
                     parenthesized(Parser<OmpObjectList>{}))) ||
     "SIMD"_id >> construct<OmpClause>(construct<OmpClause::Simd>()) ||
     "SIMDLEN" >> construct<OmpClause>(construct<OmpClause::Simdlen>(
                      parenthesized(scalarIntConstantExpr))) ||
     "THREADS" >> construct<OmpClause>(construct<OmpClause::Threads>()) ||
     "THREAD_LIMIT" >> construct<OmpClause>(construct<OmpClause::ThreadLimit>(
                           parenthesized(scalarIntExpr))) ||
TYPE_PARSER(sourced(construct<OpenMPDeclareTargetConstruct>(
     verbatim("DECLARE TARGET"_tok), Parser<OmpDeclareTargetSpecifier>{})))
 
 TYPE_PARSER(construct<OmpReductionCombiner>(Parser<AssignmentStmt>{}) ||
     construct<OmpReductionCombiner>(
         construct<OmpReductionCombiner::FunctionCombiner>(
             construct<Call>(Parser<ProcedureDesignator>{},
                 parenthesized(optionalList(actualArgSpec))))))
 
-// Hint Expression => HINT(hint-expression)
-TYPE_PARSER("HINT" >> construct<OmpHintExpr>(parenthesized(constantExpr)))
-
 // 2.17.7 atomic -> ATOMIC [clause [,]] atomic-clause [[,] clause] |
 //                  ATOMIC [clause]
 //       clause -> memory-order-clause | HINT(hint-expression)
 //       memory-order-clause -> SEQ_CST | ACQ_REL | RELEASE | ACQUIRE | RELAXED
 //       atomic-clause -> READ | WRITE | UPDATE | CAPTURE
 
 // OMP END ATOMIC
 TYPE_PARSER(construct<OmpEndAtomic>(startOmpLine >> "END ATOMIC"_tok))
 
-// Memory order clause
-TYPE_PARSER(sourced(construct<OmpMemoryOrderClause>(
-    "SEQ_CST" >> pure(llvm::omp::Clause::OMPC_seq_cst) ||
-    "ACQ_REL" >> pure(llvm::omp::Clause::OMPC_acq_rel) ||
-    "RELEASE" >> pure(llvm::omp::Clause::OMPC_release) ||
-    "ACQUIRE" >> pure(llvm::omp::Clause::OMPC_acquire) ||
-    "RELAXED" >> pure(llvm::omp::Clause::OMPC_relaxed))))
-
-// ATOMIC Memory order clause or Hint expression
-TYPE_PARSER(
-    construct<OmpAtomicMemoryOrderClause>(Parser<OmpMemoryOrderClause>{}) ||
-    construct<OmpAtomicMemoryOrderClause>(Parser<OmpHintExpr>{}))
-
-// ATOMIC Memory order Clause List
-TYPE_PARSER(construct<OmpAtomicMemoryOrderClauseList>(
-    many(maybe(","_tok) >> Parser<OmpAtomicMemoryOrderClause>{})))
-
-TYPE_PARSER(construct<OmpAtomicMemoryOrderClausePostList>(
-    many(maybe(","_tok) >> Parser<OmpAtomicMemoryOrderClause>{})))
-
 // OMP ATOMIC [MEMORY-ORDER-CLAUSE-LIST] READ [MEMORY-ORDER-CLAUSE-LIST]
 TYPE_PARSER("ATOMIC" >>
-    construct<OmpAtomicRead>(
-        Parser<OmpAtomicMemoryOrderClauseList>{} / maybe(","_tok),
-        verbatim("READ"_tok),
-        Parser<OmpAtomicMemoryOrderClausePostList>{} / endOmpLine,
+    construct<OmpAtomicRead>(Parser<OmpClauseList>{} / maybe(","_tok),
+        verbatim("READ"_tok), Parser<OmpClauseList>{} / endOmpLine,
         statement(assignmentStmt), maybe(Parser<OmpEndAtomic>{} / endOmpLine)))
 
 // OMP ATOMIC [MEMORY-ORDER-CLAUSE-LIST] CAPTURE [MEMORY-ORDER-CLAUSE-LIST]
-TYPE_PARSER(
-    "ATOMIC" >> construct<OmpAtomicCapture>(
-                    Parser<OmpAtomicMemoryOrderClauseList>{} / maybe(","_tok),
-                    verbatim("CAPTURE"_tok),
-                    Parser<OmpAtomicMemoryOrderClausePostList>{} / endOmpLine,
-                    statement(assignmentStmt), statement(assignmentStmt),
-                    Parser<OmpEndAtomic>{} / endOmpLine))
+TYPE_PARSER("ATOMIC" >>
+    construct<OmpAtomicCapture>(Parser<OmpClauseList>{} / maybe(","_tok),
+        verbatim("CAPTURE"_tok), Parser<OmpClauseList>{} / endOmpLine,
 
 // OMP ATOMIC [MEMORY-ORDER-CLAUSE-LIST] UPDATE [MEMORY-ORDER-CLAUSE-LIST]
 TYPE_PARSER("ATOMIC" >>
-    construct<OmpAtomicUpdate>(
-        Parser<OmpAtomicMemoryOrderClauseList>{} / maybe(","_tok),
-        verbatim("UPDATE"_tok),
-        Parser<OmpAtomicMemoryOrderClausePostList>{} / endOmpLine,
+    construct<OmpAtomicUpdate>(Parser<OmpClauseList>{} / maybe(","_tok),
+        verbatim("UPDATE"_tok), Parser<OmpClauseList>{} / endOmpLine,
         statement(assignmentStmt), maybe(Parser<OmpEndAtomic>{} / endOmpLine)))
 
 // OMP ATOMIC [MEMORY-ORDER-CLAUSE-LIST]
 TYPE_PARSER(construct<OmpAtomic>(verbatim("ATOMIC"_tok),
-    Parser<OmpAtomicMemoryOrderClauseList>{} / endOmpLine,
-    statement(assignmentStmt), maybe(Parser<OmpEndAtomic>{} / endOmpLine)))
+    Parser<OmpClauseList>{} / endOmpLine, statement(assignmentStmt),
+    maybe(Parser<OmpEndAtomic>{} / endOmpLine)))
 
 // OMP ATOMIC [MEMORY-ORDER-CLAUSE-LIST] WRITE [MEMORY-ORDER-CLAUSE-LIST]
 TYPE_PARSER("ATOMIC" >>
-    construct<OmpAtomicWrite>(
-        Parser<OmpAtomicMemoryOrderClauseList>{} / maybe(","_tok),
-        verbatim("WRITE"_tok),
-        Parser<OmpAtomicMemoryOrderClausePostList>{} / endOmpLine,
+    construct<OmpAtomicWrite>(Parser<OmpClauseList>{} / maybe(","_tok),
+        verbatim("WRITE"_tok), Parser<OmpClauseList>{} / endOmpLine,
         statement(assignmentStmt), maybe(Parser<OmpEndAtomic>{} / endOmpLine)))
 
 // Atomic Construct
 TYPE_PARSER(construct<OpenMPAtomicConstruct>(Parser<OmpAtomicRead>{}) ||
     construct<OpenMPAtomicConstruct>(Parser<OmpAtomicCapture>{}) ||
     construct<OpenMPAtomicConstruct>(Parser<OmpAtomicWrite>{}) ||
     construct<OpenMPAtomicConstruct>(Parser<OmpAtomicUpdate>{}) ||
     construct<OpenMPAtomicConstruct>(Parser<OmpAtomic>{}))
 
 // 2.13.2 OMP CRITICAL
 TYPE_PARSER(startOmpLine >>
     sourced(construct<OmpEndCriticalDirective>(
         verbatim("END CRITICAL"_tok), maybe(parenthesized(name)))) /
         endOmpLine)
 TYPE_PARSER(sourced(construct<OmpCriticalDirective>(verbatim("CRITICAL"_tok),
-                maybe(parenthesized(name)), maybe(Parser<OmpHintExpr>{}))) /
+                maybe(parenthesized(name)), maybe(Parser<OmpClause>{}))) /
     endOmpLine)
 
 TYPE_PARSER(construct<OpenMPCriticalConstruct>(
     Parser<OmpCriticalDirective>{}, block, Parser<OmpEndCriticalDirective>{}))
 
 // 2.8.2 Declare Simd construct
 TYPE_PARSER(
     sourced(construct<OpenMPDeclareSimdConstruct>(verbatim("DECLARE SIMD"_tok),

flang/lib/Parser/unparse.cpp

void Unparse(const OmpBlockDirective &x) {
     case llvm::omp::Directive::OMPD_workshare:
       Word("WORKSHARE ");
       break;
     default:
       // Nothing to be done
       break;
     }
   }
-  void Unparse(const OmpHintExpr &x) { Word("HINT("), Walk(x.v), Put(')'); }
-  void Unparse(const OmpMemoryOrderClause &x) {
-    switch (x.v) {
-    case llvm::omp::Clause::OMPC_seq_cst:
-      Word("SEQ_CST");
-      break;
-    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;
-    case llvm::omp::Clause::OMPC_relaxed:
-      Word("RELAXED");
-      break;
-    default:
-      break;
-    }
-  }
-  void Unparse(const OmpAtomicMemoryOrderClauseList &x) { Walk(" ", x.v, " "); }
-  void Unparse(const OmpAtomicMemoryOrderClausePostList &x) {
-    Walk(" ", x.v, " ");
-  }
   void Unparse(const OmpAtomic &x) {
     BeginOpenMP();
     Word("!$OMP ATOMIC");
-    Walk(std::get<OmpAtomicMemoryOrderClauseList>(x.t));
+    Walk(std::get<OmpClauseList>(x.t));
     Put("\n");
     EndOpenMP();
     Walk(std::get<Statement<AssignmentStmt>>(x.t));
     BeginOpenMP();
     Walk(std::get<std::optional<OmpEndAtomic>>(x.t), "!$OMP END ATOMIC\n");
     EndOpenMP();
   }
   void Unparse(const OmpAtomicCapture &x) {
     BeginOpenMP();
     Word("!$OMP ATOMIC");
-    Walk(std::get<OmpAtomicMemoryOrderClauseList>(x.t));
+    Walk(std::get<0>(x.t));
     Word(" CAPTURE");
-    Walk(std::get<OmpAtomicMemoryOrderClausePostList>(x.t));
+    Walk(std::get<2>(x.t));
     Put("\n");
     EndOpenMP();
     Walk(std::get<OmpAtomicCapture::Stmt1>(x.t));
     Put("\n");
     Walk(std::get<OmpAtomicCapture::Stmt2>(x.t));
     BeginOpenMP();
     Word("!$OMP END ATOMIC\n");
     EndOpenMP();
   }
   void Unparse(const OmpAtomicRead &x) {
     BeginOpenMP();
     Word("!$OMP ATOMIC");
-    Walk(std::get<OmpAtomicMemoryOrderClauseList>(x.t));
+    Walk(std::get<0>(x.t));
     Word(" READ");
-    Walk(std::get<OmpAtomicMemoryOrderClausePostList>(x.t));
+    Walk(std::get<2>(x.t));
     Put("\n");
     EndOpenMP();
     Walk(std::get<Statement<AssignmentStmt>>(x.t));
     BeginOpenMP();
     Walk(std::get<std::optional<OmpEndAtomic>>(x.t), "!$OMP END ATOMIC\n");
     EndOpenMP();
   }
   void Unparse(const OmpAtomicUpdate &x) {
     BeginOpenMP();
     Word("!$OMP ATOMIC");
-    Walk(std::get<OmpAtomicMemoryOrderClauseList>(x.t));
+    Walk(std::get<0>(x.t));
     Word(" UPDATE");
-    Walk(std::get<OmpAtomicMemoryOrderClausePostList>(x.t));
+    Walk(std::get<2>(x.t));
     Put("\n");
     EndOpenMP();
     Walk(std::get<Statement<AssignmentStmt>>(x.t));
     BeginOpenMP();
     Walk(std::get<std::optional<OmpEndAtomic>>(x.t), "!$OMP END ATOMIC\n");
     EndOpenMP();
   }
   void Unparse(const OmpAtomicWrite &x) {
     BeginOpenMP();
     Word("!$OMP ATOMIC");
-    Walk(std::get<OmpAtomicMemoryOrderClauseList>(x.t));
+    Walk(std::get<0>(x.t));
     Word(" WRITE");
-    Walk(std::get<OmpAtomicMemoryOrderClausePostList>(x.t));
+    Walk(std::get<2>(x.t));
     Put("\n");
     EndOpenMP();
     Walk(std::get<Statement<AssignmentStmt>>(x.t));
     BeginOpenMP();
     Walk(std::get<std::optional<OmpEndAtomic>>(x.t), "!$OMP END ATOMIC\n");
     EndOpenMP();
   }
   void Unparse(const OmpCriticalDirective &x) {
     BeginOpenMP();
     Word("!$OMP CRITICAL");
     Walk(" (", std::get<std::optional<Name>>(x.t), ")");
-    Walk(std::get<std::optional<OmpHintExpr>>(x.t));
+    Walk(std::get<std::optional<OmpClause>>(x.t));
     Put("\n");
     EndOpenMP();
   }
   void Unparse(const OmpEndCriticalDirective &x) {
     BeginOpenMP();
     Word("!$OMP END CRITICAL");
     Walk(" (", std::get<std::optional<Name>>(x.t), ")");
     Put("\n");

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

#include "llvm/Frontend/OpenMP/OMP.cpp.inc"
   void Enter(const parser::OpenMPSimpleStandaloneConstruct &);
   void Leave(const parser::OpenMPSimpleStandaloneConstruct &);
   void Enter(const parser::OpenMPFlushConstruct &);
   void Leave(const parser::OpenMPFlushConstruct &);
   void Enter(const parser::OpenMPCancelConstruct &);
   void Leave(const parser::OpenMPCancelConstruct &);
   void Enter(const parser::OpenMPCancellationPointConstruct &);
   void Leave(const parser::OpenMPCancellationPointConstruct &);
+  void Enter(const parser::OpenMPCriticalConstruct &);
+  void Leave(const parser::OpenMPCriticalConstruct &);
 
   void Leave(const parser::OmpClauseList &);
   void Enter(const parser::OmpClause &);
   void Enter(const parser::OmpNowait &);
   void Enter(const parser::OmpClause::Inbranch &);
   void Enter(const parser::OmpClause::Mergeable &);
   void Enter(const parser::OmpClause::Nogroup &);
   void Enter(const parser::OmpClause::Notinbranch &);

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

void OmpStructureChecker::Enter(const parser::OpenMPCancelConstruct &x) {
   const auto &dir{std::get<parser::Verbatim>(x.t)};
   PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_cancel);
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPCancelConstruct &) {
   dirContext_.pop_back();
 }
 
+void OmpStructureChecker::Enter(const parser::OpenMPCriticalConstruct &x) {
+  const auto &dir{std::get<parser::OmpCriticalDirective>(x.t)};
+  PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_critical);
+}
+
+void OmpStructureChecker::Leave(const parser::OpenMPCriticalConstruct &) {
+  dirContext_.pop_back();
+}
+
 void OmpStructureChecker::Enter(
     const parser::OpenMPCancellationPointConstruct &x) {
   const auto &dir{std::get<parser::Verbatim>(x.t)};
   PushContextAndClauseSets(
       dir.source, llvm::omp::Directive::OMPD_cancellation_point);
 }
 
 void OmpStructureChecker::Leave(

flang/test/Parser/omp-atomic-unparse.f90

+! RUN: %f18 -fdebug-no-semantics -funparse -fopenmp %s | FileCheck %s
+
+program main
+   implicit none
+   integer :: i, j = 10
+!READ
+!$omp atomic read
+   i = j
+!$omp atomic seq_cst read
+   i = j
+!$omp atomic read seq_cst
+   i = j
+!$omp atomic release read
+   i = j
+!$omp atomic read release
+   i = j
+!$omp atomic acq_rel read
+   i = j
+!$omp atomic read acq_rel
+   i = j
+!$omp atomic acquire read
+   i = j
+!$omp atomic read acquire
+   i = j
+!$omp atomic relaxed read
+   i = j
+!$omp atomic read relaxed
+   i = j
+
+! WRITE
+!$omp atomic write
+   i = j
+!$omp atomic seq_cst write
+   i = j
+!$omp atomic write seq_cst
+   i = j
+!$omp atomic release write
+   i = j
+!$omp atomic write release
+   i = j
+!$omp atomic acq_rel write
+   i = j
+!$omp atomic write acq_rel
+   i = j
+!$omp atomic acquire write
+   i = j
+!$omp atomic write acquire
+   i = j
+!$omp atomic relaxed write
+   i = j
+!$omp atomic write relaxed
+   i = j
+
+!UPDATE
+!$omp atomic update
+   i = j
+!$omp atomic seq_cst update
+   i = j
+!$omp atomic update seq_cst
+   i = j
+!$omp atomic release update
+   i = j
+!$omp atomic update release
+   i = j
+!$omp atomic acq_rel update
+   i = j
+!$omp atomic update acq_rel
+   i = j
+!$omp atomic acquire update
+   i = j
+!$omp atomic update acquire
+   i = j
+!$omp atomic relaxed update
+   i = j
+!$omp atomic update relaxed
+   i = j
+
+!CAPTURE
+!$omp atomic capture
+   i = j
+   i = j
+!$omp end atomic
+!$omp atomic seq_cst capture
+   i = j
+   i = j
+!$omp end atomic
+!$omp atomic capture seq_cst
+   i = j
+   i = j
+!$omp end atomic
+!$omp atomic release capture
+   i = j
+   i = j
+!$omp end atomic
+!$omp atomic capture release
+   i = j
+   i = j
+!$omp end atomic
+!$omp atomic acq_rel capture
+   i = j
+   i = j
+!$omp end atomic
+!$omp atomic capture acq_rel
+   i = j
+   i = j
+!$omp end atomic
+!$omp atomic acquire capture
+   i = j
+   i = j
+!$omp end atomic
+!$omp atomic capture acquire
+   i = j
+   i = j
+!$omp end atomic
+!$omp atomic relaxed capture
+   i = j
+   i = j
+!$omp end atomic
+!$omp atomic capture relaxed
+   i = j
+   i = j
+!$omp end atomic
+
+!ATOMIC
+!$omp atomic
+   i = j
+!$omp atomic seq_cst
+   i = j
+!$omp atomic release
+   i = j
+!$omp atomic acq_rel
+   i = j
+!$omp atomic acquire
+   i = j
+!$omp atomic relaxed
+   i = j
+
+end program main
+!CHECK-LABEL: PROGRAM main
+
+!READ
+
+!CHECK: !$OMP ATOMIC READ
+!CHECK: !$OMP ATOMIC SEQ_CST READ
+!CHECK: !$OMP ATOMIC READ SEQ_CST
+!CHECK: !$OMP ATOMIC RELEASE READ
+!CHECK: !$OMP ATOMIC READ RELEASE
+!CHECK: !$OMP ATOMIC ACQ_REL READ
+!CHECK: !$OMP ATOMIC READ ACQ_REL
+!CHECK: !$OMP ATOMIC ACQUIRE READ
+!CHECK: !$OMP ATOMIC READ ACQUIRE
+!CHECK: !$OMP ATOMIC RELAXED READ
+!CHECK: !$OMP ATOMIC READ RELAXED
+
+!WRITE
+
+!CHECK: !$OMP ATOMIC WRITE
+!CHECK: !$OMP ATOMIC SEQ_CST WRITE
+!CHECK: !$OMP ATOMIC WRITE SEQ_CST
+!CHECK: !$OMP ATOMIC RELEASE WRITE
+!CHECK: !$OMP ATOMIC WRITE RELEASE
+!CHECK: !$OMP ATOMIC ACQ_REL WRITE
+!CHECK: !$OMP ATOMIC WRITE ACQ_REL
+!CHECK: !$OMP ATOMIC ACQUIRE WRITE
+!CHECK: !$OMP ATOMIC WRITE ACQUIRE
+!CHECK: !$OMP ATOMIC RELAXED WRITE
+!CHECK: !$OMP ATOMIC WRITE RELAXED
+
+!UPDATE
+
+!CHECK: !$OMP ATOMIC UPDATE
+!CHECK: !$OMP ATOMIC SEQ_CST UPDATE
+!CHECK: !$OMP ATOMIC UPDATE SEQ_CST
+!CHECK: !$OMP ATOMIC RELEASE UPDATE
+!CHECK: !$OMP ATOMIC UPDATE RELEASE
+!CHECK: !$OMP ATOMIC ACQ_REL UPDATE
+!CHECK: !$OMP ATOMIC UPDATE ACQ_REL
+!CHECK: !$OMP ATOMIC ACQUIRE UPDATE
+!CHECK: !$OMP ATOMIC UPDATE ACQUIRE
+!CHECK: !$OMP ATOMIC RELAXED UPDATE
+!CHECK: !$OMP ATOMIC UPDATE RELAXED
+
+!CAPTURE
+
+!CHECK: !$OMP ATOMIC CAPTURE
+!CHECK: !$OMP END ATOMIC
+!CHECK: !$OMP ATOMIC SEQ_CST CAPTURE
+!CHECK: !$OMP END ATOMIC
+!CHECK: !$OMP ATOMIC CAPTURE SEQ_CST
+!CHECK: !$OMP END ATOMIC
+!CHECK: !$OMP ATOMIC RELEASE CAPTURE
+!CHECK: !$OMP END ATOMIC
+!CHECK: !$OMP ATOMIC CAPTURE RELEASE
+!CHECK: !$OMP END ATOMIC
+!CHECK: !$OMP ATOMIC ACQ_REL CAPTURE
+!CHECK: !$OMP END ATOMIC
+!CHECK: !$OMP ATOMIC CAPTURE ACQ_REL
+!CHECK: !$OMP END ATOMIC
+!CHECK: !$OMP ATOMIC ACQUIRE CAPTURE
+!CHECK: !$OMP END ATOMIC
+!CHECK: !$OMP ATOMIC CAPTURE ACQUIRE
+!CHECK: !$OMP END ATOMIC
+!CHECK: !$OMP ATOMIC RELAXED CAPTURE
+!CHECK: !$OMP END ATOMIC
+!CHECK: !$OMP ATOMIC CAPTURE RELAXED
+!CHECK: !$OMP END ATOMIC
+
+!ATOMIC
+!CHECK: !$OMP ATOMIC
+!CHECK: !$OMP ATOMIC SEQ_CST
+!CHECK: !$OMP ATOMIC RELEASE
+!CHECK: !$OMP ATOMIC ACQ_REL
+!CHECK: !$OMP ATOMIC ACQUIRE
+!CHECK: !$OMP ATOMIC RELAXED

flang/test/Semantics/omp-atomic.f90

! Check OpenMP 2.13.6 atomic Construct
   a = a + 1
 
   !$omp atomic
   a = a + 1
 
   !$omp atomic relaxed
   a = a + 1
 
+  !ERROR: expected 'UPDATE'
+  !ERROR: expected 'WRITE'
+  !ERROR: expected 'CAPTURE'
+  !ERROR: expected 'READ'
+  !$omp atomic num_threads write
+  a = a + 1
+
   !$omp end parallel
 end

llvm/include/llvm/Frontend/OpenMP/OMP.td

def OMPC_NoGroup : Clause<"nogroup"> {
   let clangClass = "OMPNogroupClause";
 }
 def OMPC_NumTasks : Clause<"num_tasks"> {
   let clangClass = "OMPNumTasksClause";
   let flangClassValue = "ScalarIntExpr";
 }
 def OMPC_Hint : Clause<"hint"> {
   let clangClass = "OMPHintClause";
+  let flangClass = "ConstantExpr";
 }
 def OMPC_DistSchedule : Clause<"dist_schedule"> {
   let clangClass = "OMPDistScheduleClause";
   let flangClass = "OmpDistScheduleClause";
   let flangClassValue = "ScalarIntExpr";
   let isValueOptional = 1;
 }
 def OMPC_DefaultMap : Clause<"defaultmap"> {