[flang][OpenMP][NFC] Refactor code related to OpenMP atomic memory order clause semantics

[NimishMishra]

Summary

Refactor functions handling memory order clauses. Related tests are already available in omp-atomic01.f90, so I do not add more tests.

[peixin]

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

1594

How about not changing CheckAtomicMemoryOrderClause and call it twice here? And also remove OmpStructureChecker::CheckAtomicMemoryOrderClause(const parser::OmpAtomicClauseList &leftHandClauseList, const parser::OmpAtomicClauseList &rightHandClauseList). Then you will escape from passing the pointer by functions and using the obscure variable names leftHandClauseList an rightHandClauseList.

CheckAtomicMemoryOrderClause(std::get<0>(atomicUpdate.t));
CheckAtomicMemoryOrderClause(std::get<2>(atomicUpdate.t));

[NimishMishra]

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

1594

That could be done. However, then some additional handling would be required for numMemoryOrderClause.

For example, !$omp atomic <memory_order_clause> read <memory_order_clause> is invalid since there are > 1 memory order clauses here. Thus I need the information- that we found 1 memory order clause while processing the left hand clause list- while I am working on the right hand clause list.

However, I can not make numMemoryOrderClause a static variable, because then

!$omp atomic  read <memory_order_clause>
   x = y
!$omp atomic  write <memory_order_clause>
   y = 1

will start giving errors, as the variable will persist across several different atomic constructs.

I do not have a strong opinion here however. I will do it as you suggest.

[peixin]

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

1594

!$omp atomic <memory_order_clause> read <memory_order_clause>

Sorry, I didn't notice this case. Your current approach is fine to me. I noticed the memory order check are all for atomic constructs. Maybe put the lambda in the front of void OmpStructureChecker::Enter(const parser::OpenMPAtomicConstruct &x)? This is not one hard comment.

[peixin]

You can wait for one day before landing if no one has further comments so that we can proceed the review for other patches depending on this.

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

private:
   bool CheckTargetBlockOnlyTeams(const parser::Block &);
   void CheckWorkshareBlockStmts(const parser::Block &, parser::CharBlock);
 
   void CheckLoopItrVariableIsInt(const parser::OpenMPLoopConstruct &x);
   void CheckDoWhile(const parser::OpenMPLoopConstruct &x);
   void CheckCycleConstraints(const parser::OpenMPLoopConstruct &x);
   template <typename T, typename D> bool IsOperatorValid(const T &, const D &);
   void CheckAtomicMemoryOrderClause(
-      const parser::OmpAtomicClauseList &, const parser::OmpAtomicClauseList &);
-  void CheckAtomicMemoryOrderClause(const parser::OmpAtomicClauseList &);
+      const parser::OmpAtomicClauseList *, const parser::OmpAtomicClauseList *);
   void CheckAtomicUpdateAssignmentStmt(const parser::AssignmentStmt &);
   void CheckAtomicConstructStructure(const parser::OpenMPAtomicConstruct &);
   void CheckDistLinear(const parser::OpenMPLoopConstruct &x);
   void CheckSIMDNest(const parser::OpenMPConstruct &x);
   void CheckTargetNest(const parser::OpenMPConstruct &x);
   void CheckCancellationNest(
       const parser::CharBlock &source, const parser::OmpCancelType::Type &type);
   std::int64_t GetOrdCollapseLevel(const parser::OpenMPLoopConstruct &x);

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

common::visit(
                   "Invalid operator in OpenMP ATOMIC (UPDATE) statement"_err_en_US);
             }
           },
       },
       expr.u);
 }
 
 void OmpStructureChecker::CheckAtomicMemoryOrderClause(
-    const parser::OmpAtomicClauseList &clauseList) {
+    const parser::OmpAtomicClauseList *leftHandClauseList,
+    const parser::OmpAtomicClauseList *rightHandClauseList) {
   int numMemoryOrderClause = 0;
-  for (const auto &clause : clauseList.v) {
-    if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
-      numMemoryOrderClause++;
-      if (numMemoryOrderClause > 1) {
-        context_.Say(clause.source,
-            "More than one memory order clause not allowed on OpenMP "
-            "Atomic construct"_err_en_US);
-        return;
-      }
-    }
-  }
-}
-
-void OmpStructureChecker::CheckAtomicMemoryOrderClause(
-    const parser::OmpAtomicClauseList &leftHandClauseList,
-    const parser::OmpAtomicClauseList &rightHandClauseList) {
-  int numMemoryOrderClause = 0;
-  for (const auto &clause : leftHandClauseList.v) {
+  auto checkForValidMemoryOrderClause =
+      [&](const parser::OmpAtomicClauseList *clauseList) {
+        for (const auto &clause : clauseList->v) {
           if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
             numMemoryOrderClause++;
             if (numMemoryOrderClause > 1) {
               context_.Say(clause.source,
                   "More than one memory order clause not allowed on "
                   "OpenMP Atomic construct"_err_en_US);
               return;
             }
           }
         }
-  for (const auto &clause : rightHandClauseList.v) {
-    if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
-      numMemoryOrderClause++;
-      if (numMemoryOrderClause > 1) {
-        context_.Say(clause.source,
-            "More than one memory order clause not "
-            "allowed on OpenMP Atomic construct"_err_en_US);
-        return;
-      }
-    }
+      };
+  if (leftHandClauseList) {
+    checkForValidMemoryOrderClause(leftHandClauseList);
+  if (rightHandClauseList) {
+    checkForValidMemoryOrderClause(rightHandClauseList);
   }
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPAtomicConstruct &x) {
   common::visit(
       common::visitors{
           [&](const parser::OmpAtomic &atomicConstruct) {
             const auto &dir{std::get<parser::Verbatim>(atomicConstruct.t)};
             PushContextAndClauseSets(
                 dir.source, llvm::omp::Directive::OMPD_atomic);
             CheckAtomicUpdateAssignmentStmt(
                 std::get<parser::Statement<parser::AssignmentStmt>>(
                     atomicConstruct.t)
                     .statement);
             CheckAtomicMemoryOrderClause(
-                std::get<parser::OmpAtomicClauseList>(atomicConstruct.t));
+                &std::get<parser::OmpAtomicClauseList>(atomicConstruct.t),
+                nullptr);
           },
-          [&](const parser::OmpAtomicUpdate &atomicConstruct) {
-            const auto &dir{std::get<parser::Verbatim>(atomicConstruct.t)};
+          [&](const parser::OmpAtomicUpdate &atomicUpdate) {
+            const auto &dir{std::get<parser::Verbatim>(atomicUpdate.t)};
             PushContextAndClauseSets(
                 dir.source, llvm::omp::Directive::OMPD_atomic);
             CheckAtomicUpdateAssignmentStmt(
                 std::get<parser::Statement<parser::AssignmentStmt>>(
-                    atomicConstruct.t)
+                    atomicUpdate.t)
                     .statement);
             CheckAtomicMemoryOrderClause(
-                std::get<0>(atomicConstruct.t), std::get<2>(atomicConstruct.t));
+                &std::get<0>(atomicUpdate.t), &std::get<2>(atomicUpdate.t));
           },
           [&](const auto &atomicConstruct) {
             const auto &dir{std::get<parser::Verbatim>(atomicConstruct.t)};
             PushContextAndClauseSets(
                 dir.source, llvm::omp::Directive::OMPD_atomic);
-            CheckAtomicMemoryOrderClause(
-                std::get<0>(atomicConstruct.t), std::get<2>(atomicConstruct.t));
+            CheckAtomicMemoryOrderClause(&std::get<0>(atomicConstruct.t),
+                &std::get<2>(atomicConstruct.t));
           },
       },
       x.u);
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPAtomicConstruct &) {
   dirContext_.pop_back();
 }