[flang][OpenMP] Added semantic checks for atomic capture construct

[NimishMishra]

Summary

This patch adds semantic checks for atomic capture construct.

Depends on D127620

[NimishMishra]

There are still some semantic checks left. I will cover them in a separate patch

[shraiysh]

Two minor comments at the moment. Will review it later this week.

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

1413

Can we please start adding some documentation to the function right above it, like here

1633–1648

nit: wrap this loop in a lambda inside this function and use twice instead of duplication.

To avoid higher cyclomatic complexity, use early return in the lambda function when the if conditions become nullptr. What happens if none of the clauses in the list match the if conditions? Is it possible to have one clause match some conditions and not the rest? If not, then we should assert instead of if and if yes, should their else parts be ignored? (I think the innermost if must always be true if control reaches there, but please feel free to correct me).

[NimishMishra]

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

1413

This would certainly be nice. If others agree too, we can go ahead and document the functions.

[kiranchandramohan]

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

1413

+1 for more documentation, particularly for special support functions.

[peixin]

Can you split this patch? The small patch with single function is easier to review. If there are multiple checks with similar style, you can put them together.

1. check "capture statement is of the form v = x if atomic construct is read or capture"

check "write statement is of the form x = expr if atomic construct is write"

2. check "x must not have the ALLOCATABLE attribute."

check "operator must refer to the intrinsic operator and not to a user-defined operator"
check "hint-expression is a constant expression that evaluates to a scalar value with kind omp_sync_hint_kind and a value that is a valid synchronization hint."

Or even split point 2 into three small patches.

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

1413

+1

But I prefer not to add the comments like this The comments for op, converter, loc, eval are useless. My preference is to use function name and argument name comment themselves. If they cannot comment itself such as with some special support, it's better to add comments for them.

[NimishMishra]

In D126612#3550182, @peixin wrote:

Can you split this patch? The small patch with single function is easier to review. If there are multiple checks with similar style, you can put them together.

Sure. Working on splitting this patch's contents across multiple smaller patches

[NimishMishra]

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

3715 ↗

(On Diff #447287)

I have an idea here that could potentially ease lowering of atomic capture construct. To lower such constructs, we need to know what the two statements are before we can create operations for them.

My idea was if we could introduce attributes as these in the parse-tree node itself which shall be filled in during semantics. Then during lowering, we just look at Stmt1Type and Stmt2Type and create required operations for the two statements of capture construct.

The only caveat is that the node is const at the point where OmpStructureChecker is instantiated. Any ideas on how to go about this problem will greatly help.

[NimishMishra]

The design of this solution could be improved. Following whatever reviews come in https://reviews.llvm.org/D127272, we can use the same basic structural checks from that patch here to understand what the statements 1 and 2 of the atomic capture construct actually are (among read/write/update statements). That would remove dependence on global boolean flags, which don't look good design wise.

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 &);
-  void CheckAtomicUpdateAssignmentStmt(const parser::AssignmentStmt &);
-  void CheckAtomicConstructStructure(const parser::OpenMPAtomicConstruct &);
+      const parser::OmpAtomicClauseList *, const parser::OmpAtomicClauseList *);
+  void CheckAtomicHintClause(
+      const parser::OmpAtomicClauseList *, const parser::OmpAtomicClauseList *);
+  bool CheckAtomicCaptureStmt(const parser::AssignmentStmt &, bool);
+  bool CheckAtomicUpdateStmt(const parser::AssignmentStmt &, bool);
+  bool CheckAtomicWriteStmt(const parser::AssignmentStmt &, bool);
+  void CheckAtomicCaptureConstruct(const parser::OmpAtomicCapture &);
   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);
   bool CheckReductionOperators(const parser::OmpClause::Reduction &);
   bool CheckIntrinsicOperator(
void CheckArraySection(const parser::ArrayElement &arrayElement,
       const parser::Name &name, const llvm::omp::Clause clause);
   void CheckMultipleAppearanceAcrossContext(
       const parser::OmpObjectList &ompObjectList);
   const parser::OmpObjectList *GetOmpObjectList(const parser::OmpClause &);
   void CheckPredefinedAllocatorRestriction(const parser::CharBlock &source,
       const parser::OmpObjectList &ompObjectList);
   void CheckPredefinedAllocatorRestriction(
       const parser::CharBlock &source, const parser::Name &name);
+  bool CheckForAllocatableVariable(
+      const semantics::Symbol &, const parser::Variable &var, bool);
   bool isPredefinedAllocator{false};
   void EnterDirectiveNest(const int index) { directiveNest_[index]++; }
   void ExitDirectiveNest(const int index) { directiveNest_[index]--; }
   int GetDirectiveNest(const int index) { return directiveNest_[index]; }
 
   enum directiveNestType {
     SIMDNest,
     TargetBlockOnlyTeams,
     TargetNest,
     LastType
   };
   int directiveNest_[LastType + 1] = {0};
 };
 } // namespace Fortran::semantics
 #endif // FORTRAN_SEMANTICS_CHECK_OMP_STRUCTURE_H_

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

 // EndBlockDirective.
 void OmpStructureChecker::Leave(const parser::OmpEndBlockDirective &x) {
   if ((GetContext().directive == llvm::omp::Directive::OMPD_end_single) ||
       (GetContext().directive == llvm::omp::Directive::OMPD_end_workshare)) {
     dirContext_.pop_back();
   }
 }
 
+bool OmpStructureChecker::CheckForAllocatableVariable(
+    const semantics::Symbol &symbol, const parser::Variable &var,
+    bool shouldSuppressOutput) {
+  if (IsAllocatable(symbol)) {
+    const auto &designator =
+        std::get<common::Indirection<parser::Designator>>(var.u);
+    if (const auto *dataRef =
+            std::get_if<Fortran::parser::DataRef>(&designator.value().u)) {
+      if (const auto name = std::get_if<Fortran::parser::Name>(&dataRef->u)) {
+        if (!shouldSuppressOutput)
+          context_.Say(name->source,
+              "%s must not have ALLOCATABLE "
+              "attribute"_err_en_US,
+              name->ToString());
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
 template <typename T, typename D>
 bool OmpStructureChecker::IsOperatorValid(const T &node, const D &variable) {
   using AllowedBinaryOperators =
       std::variant<parser::Expr::Add, parser::Expr::Multiply,
           parser::Expr::Subtract, parser::Expr::Divide, parser::Expr::AND,
           parser::Expr::OR, parser::Expr::EQV, parser::Expr::NEQV>;
   using BinaryOperators = std::variant<parser::Expr::Add,
       parser::Expr::Multiply, parser::Expr::Subtract, parser::Expr::Divide,
       parser::Expr::AND, parser::Expr::OR, parser::Expr::EQV,
       parser::Expr::NEQV, parser::Expr::Power, parser::Expr::Concat,
       parser::Expr::LT, parser::Expr::LE, parser::Expr::EQ, parser::Expr::NE,
       parser::Expr::GE, parser::Expr::GT>;
 
   if constexpr (common::HasMember<T, BinaryOperators>) {
     const auto &variableName{variable.GetSource().ToString()};
     const auto &exprLeft{std::get<0>(node.t)};
     const auto &exprRight{std::get<1>(node.t)};
     if ((exprLeft.value().source.ToString() != variableName) &&
         (exprRight.value().source.ToString() != variableName)) {
       context_.Say(variable.GetSource(),
-          "Atomic update variable '%s' not found in the RHS of the "
-          "assignment statement in an ATOMIC (UPDATE) construct"_err_en_US,
-          variableName);
+          "Atomic update statement should "
+          "be of form `%s = %s operator expr` OR `%s = expr operator %s`"_err_en_US,
+          variableName, variableName, variableName, variableName);
     }
     return common::HasMember<T, AllowedBinaryOperators>;
   }
   return true;
 }
 
-void OmpStructureChecker::CheckAtomicUpdateAssignmentStmt(
-    const parser::AssignmentStmt &assignment) {
+void OmpStructureChecker::CheckAtomicCaptureConstruct(
+    const parser::OmpAtomicCapture &atomicCaptureConstruct) {
+  const parser::AssignmentStmt &stmt1 =
+      std::get<3>(atomicCaptureConstruct.t).v.statement;
+  const parser::AssignmentStmt &stmt2 =
+      std::get<4>(atomicCaptureConstruct.t).v.statement;
+
+  if (CheckAtomicCaptureStmt(stmt1, true)) {
+    // atomic-write statement is a proper superset of atomic-update. It is
+    // sufficient to thus check for it and report semantic errors (if any) in
+    // stmt2 from [capture-stmt, write-stmt] or [capture-stmt, update-stmt]
+    CheckAtomicWriteStmt(stmt2, false);
+  } else if (CheckAtomicWriteStmt(stmt2, true) ||
+      CheckAtomicUpdateStmt(stmt2, true)) {
+    // Report semantic errors (if any) in stmt1 in [capture-stmt, write-stmt] or
+    // [capture-stmt, update-stmt]
+    CheckAtomicCaptureStmt(stmt1, false);
+  } else if (CheckAtomicCaptureStmt(stmt2, true)) {
+    // Report semantic errors (if any) in stmt1 in [update-stmt, capture-stmt]
+    CheckAtomicUpdateStmt(stmt1, false);
+  } else if (CheckAtomicUpdateStmt(stmt1, true)) {
+    // Report semantic errors (if any) in stmt2 in [update-stmt, capture-stmt]
+    CheckAtomicCaptureStmt(stmt2, false);
+  } else {
+    context_.Say(atomicCaptureConstruct.source,
+        "Invalid ATOMIC (CAPTURE) "
+        "construct statements. Expect one of [update-stmt, "
+        "capture-stmt], [capture-stmt, update-stmt]"
+        "or [capture-stmt, write-stmt]."_err_en_US);
+  }
+}
+
+bool OmpStructureChecker::CheckAtomicWriteStmt(
+    const parser::AssignmentStmt &assignment, bool shouldSuppressOutput) {
+  const auto &expr{std::get<parser::Expr>(assignment.t)};
+  const auto &var{std::get<parser::Variable>(assignment.t)};
+  bool isValidAtomicWriteStmt{true};
+  if (const auto *e{GetExpr(context_, var)}) {
+    for (const Symbol &symbol : evaluate::CollectSymbols(*e)) {
+      if (CheckForAllocatableVariable(symbol, var, shouldSuppressOutput))
+        isValidAtomicWriteStmt = false;
+    }
+  }
+  return isValidAtomicWriteStmt;
+}
+
+bool OmpStructureChecker::CheckAtomicUpdateStmt(
+    const parser::AssignmentStmt &assignment, bool shouldSuppressOutput) {
   const auto &expr{std::get<parser::Expr>(assignment.t)};
   const auto &var{std::get<parser::Variable>(assignment.t)};
+  bool isValidAtomicUpdateStmt{true};
   common::visit(
       common::visitors{
           [&](const common::Indirection<parser::FunctionReference> &x) {
             const auto &procedureDesignator{
                 std::get<parser::ProcedureDesignator>(x.value().v.t)};
             const parser::Name *name{
                 std::get_if<parser::Name>(&procedureDesignator.u)};
             if (name &&
                 !(name->source == "max" || name->source == "min" ||
                     name->source == "iand" || name->source == "ior" ||
                     name->source == "ieor")) {
+              if (!shouldSuppressOutput)
                 context_.Say(expr.source,
                     "Invalid intrinsic procedure name in "
                     "OpenMP ATOMIC (UPDATE) statement"_err_en_US);
+              isValidAtomicUpdateStmt = false;
             } else if (name) {
               bool foundMatch{false};
               if (auto varDesignatorIndirection =
                       std::get_if<Fortran::common::Indirection<
                           Fortran::parser::Designator>>(&var.u)) {
                 const auto &varDesignator = varDesignatorIndirection->value();
                 if (const auto *dataRef = std::get_if<Fortran::parser::DataRef>(
                         &varDesignator.u)) {
                   if (const auto *name =
                           std::get_if<Fortran::parser::Name>(&dataRef->u)) {
                     const auto &varSymbol = *name->symbol;
                     if (const auto *e{GetExpr(context_, expr)}) {
                       for (const Symbol &symbol :
                           evaluate::CollectSymbols(*e)) {
                         if (symbol == varSymbol) {
                           foundMatch = true;
                           break;
                         }
                       }
                     }
                   }
                 }
               }
               if (!foundMatch) {
+                if (!shouldSuppressOutput)
                   context_.Say(expr.source,
                       "Atomic update variable '%s' not found in the "
                       "argument list of intrinsic procedure"_err_en_US,
                       var.GetSource().ToString());
+                isValidAtomicUpdateStmt = false;
               }
             }
           },
+          [&](const parser::Expr::DefinedUnary &) {
+            if (!shouldSuppressOutput)
+              context_.Say(expr.source,
+                  "User defined operator not allowed in"
+                  " OpenMP ATOMIC (UPDATE) statement"_err_en_US);
+            isValidAtomicUpdateStmt = false;
+          },
+          [&](const parser::Expr::DefinedBinary &) {
+            if (!shouldSuppressOutput)
+              context_.Say(expr.source,
+                  "User defined operator not allowed in"
+                  " OpenMP ATOMIC (UPDATE) statement"_err_en_US);
+            isValidAtomicUpdateStmt = false;
+          },
           [&](const auto &x) {
             if (!IsOperatorValid(x, var)) {
+              if (!shouldSuppressOutput)
                 context_.Say(expr.source,
                     "Invalid operator in OpenMP ATOMIC (UPDATE) statement"_err_en_US);
+              isValidAtomicUpdateStmt = false;
             }
           },
       },
-      expr.u);
-}
 
-void OmpStructureChecker::CheckAtomicMemoryOrderClause(
-    const parser::OmpAtomicClauseList &clauseList) {
-  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;
-      }
+      expr.u);
+  if (const auto *e{GetExpr(context_, var)}) {
+    for (const Symbol &symbol : evaluate::CollectSymbols(*e)) {
+      if (CheckForAllocatableVariable(symbol, var, shouldSuppressOutput))
+        isValidAtomicUpdateStmt = false;
     }
   }
+  return isValidAtomicUpdateStmt;
 }
 
 void OmpStructureChecker::CheckAtomicMemoryOrderClause(
-    const parser::OmpAtomicClauseList &leftHandClauseList,
-    const parser::OmpAtomicClauseList &rightHandClauseList) {
+    const parser::OmpAtomicClauseList *leftOmpAtomicClauseList,
+    const parser::OmpAtomicClauseList *rightOmpAtomicClauseList) {
   int numMemoryOrderClause = 0;
-  for (const auto &clause : leftHandClauseList.v) {
+  if (leftOmpAtomicClauseList) {
+    for (const auto &clause : leftOmpAtomicClauseList->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 (rightOmpAtomicClauseList) {
+    for (const auto &clause : rightOmpAtomicClauseList->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::CheckAtomicHintClause(
+    const parser::OmpAtomicClauseList *leftOmpAtomicClauseList,
+    const parser::OmpAtomicClauseList *rightOmpAtomicClauseList) {
+  if (leftOmpAtomicClauseList) {
+    for (const auto &clause : leftOmpAtomicClauseList->v) {
+      if (const auto ompClause =
+              std::get_if<Fortran::parser::OmpClause>(&clause.u)) {
+        if (auto hintClause =
+                std::get_if<Fortran::parser::OmpClause::Hint>(&ompClause->u)) {
+          if (auto evaluatedValue{GetIntValue(hintClause->v)}) {
+            int hintValue = evaluatedValue.value();
+            if (hintValue != 0x0 && hintValue != 0x1 && hintValue != 0x2 &&
+                hintValue != 0x4 && hintValue != 0x8)
+              context_.Say(clause.source,
+                  "Hint clause value "
+                  "is not a valid OpenMP synchronization value"_err_en_US);
+          }
+        }
+      }
+    }
+  }
+  if (rightOmpAtomicClauseList) {
+    for (const auto &clause : rightOmpAtomicClauseList->v) {
+      if (const auto ompClause =
+              std::get_if<Fortran::parser::OmpClause>(&clause.u)) {
+        if (auto hintClause =
+                std::get_if<Fortran::parser::OmpClause::Hint>(&ompClause->u)) {
+          if (auto evaluatedValue{GetIntValue(hintClause->v)}) {
+            int hintValue = evaluatedValue.value();
+            if (hintValue != 0x0 && hintValue != 0x1 && hintValue != 0x2 &&
+                hintValue != 0x4 && hintValue != 0x8)
+              context_.Say(clause.source,
+                  "Hint clause value "
+                  "is not a valid OpenMP synchronization value"_err_en_US);
+          }
+        }
+      }
+    }
+  }
+}
+
+bool OmpStructureChecker::CheckAtomicCaptureStmt(
+    const parser::AssignmentStmt &assignmentStmt, bool shouldSuppressOutput) {
+  const auto &expr{std::get<parser::Expr>(assignmentStmt.t)};
+  const auto &var{std::get<parser::Variable>(assignmentStmt.t)};
+  bool isVariable{false};
+  bool isValidAtomicCaptureStmt{true};
+  common::visit(
+      common::visitors{
+          [&](const common::Indirection<parser::Designator> &designator) {
+            if (const auto *dataRef = std::get_if<Fortran::parser::DataRef>(
+                    &designator.value().u)) {
+              if (const auto *name =
+                      std::get_if<Fortran::parser::Name>(&dataRef->u)) {
+                if (IsVariableName(*name->symbol)) {
+                  isVariable = true;
+                  if (IsAllocatable(*name->symbol)) {
+                    if (!shouldSuppressOutput)
+                      context_.Say(name->source,
+                          "%s must not have ALLOCATABLE "
+                          "attribute"_err_en_US,
+                          name->ToString());
+                    isValidAtomicCaptureStmt = false;
+                  }
+                }
+              }
+            }
+          },
+          [&](const auto &) {},
+      },
+      expr.u);
+  if (!isVariable) {
+    if (!shouldSuppressOutput)
+      context_.Say(expr.source,
+          "Expected scalar variable "
+          "of intrinsic type on RHS of OPENMP Atomic "
+          " (CAPTURE) assignment statement"_err_en_US);
+    isValidAtomicCaptureStmt = false;
+  }
+  return isValidAtomicCaptureStmt;
+}
 
 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(
+            CheckAtomicUpdateStmt(
                 std::get<parser::Statement<parser::AssignmentStmt>>(
                     atomicConstruct.t)
-                    .statement);
+                    .statement,
+                false);
             CheckAtomicMemoryOrderClause(
-                std::get<parser::OmpAtomicClauseList>(atomicConstruct.t));
+                &std::get<parser::OmpAtomicClauseList>(atomicConstruct.t),
+                nullptr);
+            CheckAtomicHintClause(
+                &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(
+            CheckAtomicUpdateStmt(
                 std::get<parser::Statement<parser::AssignmentStmt>>(
-                    atomicConstruct.t)
-                    .statement);
+                    atomicUpdate.t)
+                    .statement,
+                false);
             CheckAtomicMemoryOrderClause(
-                std::get<0>(atomicConstruct.t), std::get<2>(atomicConstruct.t));
+                &std::get<0>(atomicUpdate.t), &std::get<2>(atomicUpdate.t));
+            CheckAtomicHintClause(
+                &std::get<0>(atomicUpdate.t), &std::get<2>(atomicUpdate.t));
           },
-          [&](const auto &atomicConstruct) {
-            const auto &dir{std::get<parser::Verbatim>(atomicConstruct.t)};
+          [&](const parser::OmpAtomicRead &atomicRead) {
+            const auto &dir{std::get<parser::Verbatim>(atomicRead.t)};
+            PushContextAndClauseSets(
+                dir.source, llvm::omp::Directive::OMPD_atomic);
+            CheckAtomicMemoryOrderClause(
+                &std::get<0>(atomicRead.t), &std::get<2>(atomicRead.t));
+            CheckAtomicHintClause(
+                &std::get<0>(atomicRead.t), &std::get<2>(atomicRead.t));
+            CheckAtomicCaptureStmt(
+                std::get<parser::Statement<parser::AssignmentStmt>>(
+                    atomicRead.t)
+                    .statement,
+                false);
+          },
+          [&](const parser::OmpAtomicWrite &atomicWrite) {
+            const auto &dir{std::get<parser::Verbatim>(atomicWrite.t)};
+            PushContextAndClauseSets(
+                dir.source, llvm::omp::Directive::OMPD_atomic);
+            CheckAtomicMemoryOrderClause(
+                &std::get<0>(atomicWrite.t), &std::get<2>(atomicWrite.t));
+            CheckAtomicHintClause(
+                &std::get<0>(atomicWrite.t), &std::get<2>(atomicWrite.t));
+            CheckAtomicWriteStmt(
+                std::get<parser::Statement<parser::AssignmentStmt>>(
+                    atomicWrite.t)
+                    .statement,
+                false);
+          },
+          [&](const parser::OmpAtomicCapture &atomicCapture) {
+            const auto &dir{std::get<parser::Verbatim>(atomicCapture.t)};
             PushContextAndClauseSets(
                 dir.source, llvm::omp::Directive::OMPD_atomic);
             CheckAtomicMemoryOrderClause(
-                std::get<0>(atomicConstruct.t), std::get<2>(atomicConstruct.t));
+                &std::get<0>(atomicCapture.t), &std::get<2>(atomicCapture.t));
+            CheckAtomicHintClause(
+                &std::get<0>(atomicCapture.t), &std::get<2>(atomicCapture.t));
+            CheckAtomicCaptureConstruct(atomicCapture);
           },
       },
       x.u);
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPAtomicConstruct &) {
   dirContext_.pop_back();
 }

flang/test/Semantics/omp-atomic-assignment-stmt.f90

+! RUN: %python %S/test_errors.py %s %flang_fc1 -fopenmp
+! Semantic checks for various assignments related to atomic constructs
+
+program sample
+    use omp_lib
+    integer :: x, v
+    integer :: y(10)
+    integer, allocatable :: k
+
+    !$omp atomic read
+        v = x
+
+    !$omp atomic read
+    !ERROR: No intrinsic or user-defined ASSIGNMENT(=) matches scalar INTEGER(4) and rank 1 array of INTEGER(4)
+    !ERROR: Expected scalar variable of intrinsic type on RHS of OPENMP Atomic  (CAPTURE) assignment statement
+        v = y(1:3)
+
+    !$omp atomic read
+    !ERROR: Expected scalar variable of intrinsic type on RHS of OPENMP Atomic  (CAPTURE) assignment statement
+        v = x * (10 + x)
+
+    !$omp atomic read
+    !ERROR: Expected scalar variable of intrinsic type on RHS of OPENMP Atomic  (CAPTURE) assignment statement
+        v = 4
+
+    !$omp atomic read
+    !ERROR: k must not have ALLOCATABLE attribute
+        v = k
+
+    !$omp atomic write
+    !ERROR: k must not have ALLOCATABLE attribute
+        k = x
+
+    !$omp atomic update 
+    !ERROR: Atomic update statement should be of form `k = k operator expr` OR `k = expr operator k`
+    !ERROR: k must not have ALLOCATABLE attribute
+        k = v + k * (v * k)
+
+    !$omp atomic
+    !ERROR: k must not have ALLOCATABLE attribute
+        k = v * k  
+end program
+
+subroutine omp_atomic_capture
+    integer :: x, v, y
+    integer, allocatable :: k
+    ! [capture-stmt, write-stmt]
+    !$omp atomic capture
+        x = v
+        v = max(x, v)
+    !$omp end atomic
+
+    !$omp atomic capture
+        !ERROR: k must not have ALLOCATABLE attribute
+        x = k
+        v = max(x, y)
+    !$omp end atomic
+
+    !$omp atomic capture
+    !ERROR: Expected scalar variable of intrinsic type on RHS of OPENMP Atomic  (CAPTURE) assignment statement
+        x = 10
+        x = 4 + x * 10
+    !$omp end atomic
+end subroutine

flang/test/Semantics/omp-atomic-hint-clause.f90

+! RUN: %python %S/test_errors.py %s %flang_fc1 -fopenmp
+! Semantic checks on hint clauses, as they appear on atomic constructs
+
+program sample
+    use omp_lib
+    integer :: x, y
+    !$omp atomic hint(1) write
+        y = 2
+    
+    !$omp atomic read hint(2)
+        y = x    
+    
+    !ERROR: Hint clause value is not a valid OpenMP synchronization value
+    !$omp atomic hint(3)
+        y = y + 10
+    
+    !ERROR: Hint clause value is not a valid OpenMP synchronization value
+    !$omp atomic update hint(5)
+        y = x
+    
+    !ERROR: Hint clause value is not a valid OpenMP synchronization value
+    !$omp atomic hint(7) capture
+        y = x
+        x = y
+    !$omp end atomic
+    
+    !ERROR: Must be a constant value
+    !$omp atomic update hint(x)
+        y = y * 1
+    
+    !$omp atomic read hint(4)
+        y = x
+
+    !$omp atomic hint(8)
+        x = x * y
+
+    !$omp atomic write hint(omp_sync_hint_uncontended)
+        x = 10 * y
+
+    !$omp atomic hint(omp_lock_hint_speculative)
+        x = y + x
+
+    !$omp atomic hint(omp_sync_hint_uncontended) read
+        y = x 
+
+    !$omp atomic hint(omp_sync_hint_nonspeculative)
+        y = y * 9
+end program

flang/test/Semantics/omp-atomic-user-defined-operators.f90

+! RUN: %python %S/test_errors.py %s %flang_fc1 -fopenmp
+! Semantics checks on user defined operators on atomic update statement
+! TODO: Add error reporting for overloaded intrinsic operators
+
+module overload_asterisk
+    implicit none
+    private
+    public operator (*)
+
+    interface operator(*)
+        module procedure logical_and
+    end interface
+
+contains
+    pure logical function logical_and(log1, log2)
+        logical, intent(in) :: log1, log2
+
+        logical_and = (log1 .and. log2)
+    end function
+end module
+
+module new_operator
+    implicit none
+    private
+    public operator(.MYOPERATOR.)
+
+    interface operator(.MYOPERATOR.)
+       module procedure myprocedure
+    end interface
+contains
+    pure integer function myprocedure(param1, param2)
+        integer, intent(in) :: param1, param2
+        myprocedure = param1 + param2
+    end function
+end module
+
+program sample
+    use omp_lib
+    use overload_asterisk
+    use new_operator
+    implicit none
+
+    logical  :: T = .true., F = .false.
+    integer :: x, y 
+    !$omp atomic
+        T = T*T
+    
+    !$omp atomic update
+        x = x / y
+     
+    !$omp atomic update
+    !ERROR: User defined operator not allowed in OpenMP ATOMIC (UPDATE) statement
+        x = x .MYOPERATOR. y
+
+    !$omp atomic
+    !ERROR: User defined operator not allowed in OpenMP ATOMIC (UPDATE) statement
+        x = x .MYOPERATOR. y
+end program

flang/test/Semantics/omp-atomic02.f90

program OmpAtomic
    a = a/(b + 1)
    !$omp atomic
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    a = a**4
    !$omp atomic
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    c = c//d
    !$omp atomic
-   !ERROR: Atomic update variable 'l' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    l = a .LT. b
    !$omp atomic
-   !ERROR: Atomic update variable 'l' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    l = a .LE. b
    !$omp atomic
-   !ERROR: Atomic update variable 'l' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    l = a .EQ. b
    !$omp atomic
-   !ERROR: Atomic update variable 'l' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    l = a .NE. b
    !$omp atomic
-   !ERROR: Atomic update variable 'l' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    l = a .GE. b
    !$omp atomic
-   !ERROR: Atomic update variable 'l' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    l = a .GT. b
    !$omp atomic
    m = m .AND. n
    !$omp atomic
    m = m .OR. n
    !$omp atomic
    m = m .EQV. n
program OmpAtomic
    a = a/(b + 1)
    !$omp atomic update
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    a = a**4
    !$omp atomic update
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    c = c//d
    !$omp atomic update
-   !ERROR: Atomic update variable 'l' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    l = a .LT. b
    !$omp atomic update
-   !ERROR: Atomic update variable 'l' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    l = a .LE. b
    !$omp atomic update
-   !ERROR: Atomic update variable 'l' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    l = a .EQ. b
    !$omp atomic update
-   !ERROR: Atomic update variable 'l' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    l = a .GE. b
    !$omp atomic update
-   !ERROR: Atomic update variable 'l' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `l = l operator expr` OR `l = expr operator l`
    !ERROR: Invalid operator in OpenMP ATOMIC (UPDATE) statement
    l = a .GT. b
    !$omp atomic update
    m = m .AND. n
    !$omp atomic update
    m = m .OR. n
    !$omp atomic update
    m = m .EQV. n
    !$omp atomic update
    m = m .NEQV. n
    !$omp end parallel
 end program OmpAtomic

flang/test/Semantics/omp-atomic04.f90

program OmpAtomic
    y = 3
    m = .TRUE.
    n = .FALSE.
 !$omp atomic
    x = x + 1
 !$omp atomic
    x = 1 + x
 !$omp atomic
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
    x = y + 1
 !$omp atomic
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
-   x = 1 + y
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
+   x = 1 + (y + x)
 
 !$omp atomic
-   x = x - 1
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
+   x = 1 - (10 * (y + x))
 !$omp atomic
    x = 1 - x
 !$omp atomic
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
    x = y - 1
 !$omp atomic
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
    x = 1 - y
 
 !$omp atomic
    x = x*1
 !$omp atomic
    x = 1*x
 !$omp atomic
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
-   x = y*1
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
+   x = y*(10 + x)
 !$omp atomic
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
-   x = 1*y
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
+   x = (44 * x) * y
 
 !$omp atomic
    x = x/1
 !$omp atomic
    x = 1/x
 !$omp atomic
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
    x = y/1
 !$omp atomic
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
    x = 1/y
 
 !$omp atomic
    m = m .AND. n
 !$omp atomic
    m = n .AND. m
 !$omp atomic
-   !ERROR: Atomic update variable 'm' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `m = m operator expr` OR `m = expr operator m`
    m = n .AND. l
 
 !$omp atomic
    m = m .OR. n
 !$omp atomic
    m = n .OR. m
 !$omp atomic
-   !ERROR: Atomic update variable 'm' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `m = m operator expr` OR `m = expr operator m`
    m = n .OR. l
 
 !$omp atomic
    m = m .EQV. n
 !$omp atomic
    m = n .EQV. m
 !$omp atomic
-   !ERROR: Atomic update variable 'm' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `m = m operator expr` OR `m = expr operator m`
    m = n .EQV. l
 
 !$omp atomic
    m = m .NEQV. n
 !$omp atomic
    m = n .NEQV. m
 !$omp atomic
-   !ERROR: Atomic update variable 'm' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `m = m operator expr` OR `m = expr operator m`
    m = n .NEQV. l
 
 !$omp atomic update
    x = x + 1
 !$omp atomic update
    x = 1 + x
 !$omp atomic update
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
    x = y + 1
 !$omp atomic update
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
    x = 1 + y
 
 !$omp atomic update
    x = x - 1
 !$omp atomic update
    x = 1 - x
 !$omp atomic update
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
    x = y - 1
 !$omp atomic update
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
    x = 1 - y
 
 !$omp atomic update
    x = x*1
 !$omp atomic update
    x = 1*x
 !$omp atomic update
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
    x = y*1
 !$omp atomic update
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
    x = 1*y
 
 !$omp atomic update
    x = x/1
 !$omp atomic update
    x = 1/x
 !$omp atomic update
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
-   x = y/1
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
+   x = max(x, y) + 10
 !$omp atomic update
-   !ERROR: Atomic update variable 'x' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
-   x = 1/y
+   !ERROR: Atomic update statement should be of form `x = x operator expr` OR `x = expr operator x`
+   x = y * min(x, y)
 
 !$omp atomic update
    m = m .AND. n
 !$omp atomic update
    m = n .AND. m
 !$omp atomic update
-   !ERROR: Atomic update variable 'm' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `m = m operator expr` OR `m = expr operator m`
    m = n .AND. l
 
 !$omp atomic update
    m = m .OR. n
 !$omp atomic update
    m = n .OR. m
 !$omp atomic update
-   !ERROR: Atomic update variable 'm' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `m = m operator expr` OR `m = expr operator m`
    m = n .OR. l
 
 !$omp atomic update
    m = m .EQV. n
 !$omp atomic update
    m = n .EQV. m
 !$omp atomic update
-   !ERROR: Atomic update variable 'm' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `m = m operator expr` OR `m = expr operator m`
    m = n .EQV. l
 
 !$omp atomic update
    m = m .NEQV. n
 !$omp atomic update
    m = n .NEQV. m
 !$omp atomic update
-   !ERROR: Atomic update variable 'm' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct
+   !ERROR: Atomic update statement should be of form `m = m operator expr` OR `m = expr operator m`
    m = n .NEQV. l
 
 end program OmpAtomic