From e39900727ba16b78a4434876cb21103078821feb Mon Sep 17 00:00:00 2001
From: Andre Kuhlenschmidt <akuhlenschmi@nvidia.com>
Date: Fri, 18 Jul 2025 12:56:11 -0700
Subject: [PATCH 1/8] early feedback commit

---
 flang/lib/Semantics/check-acc-structure.cpp   | 153 ++++++++++++++++--
 flang/lib/Semantics/check-acc-structure.h     |  17 ++
 .../Semantics/OpenACC/acc-atomic-validity.f90 |   1 +
 3 files changed, 161 insertions(+), 10 deletions(-)

diff --git a/flang/lib/Semantics/check-acc-structure.cpp b/flang/lib/Semantics/check-acc-structure.cpp
index 9cbea9742a6a2..a1b877c64b968 100644
--- a/flang/lib/Semantics/check-acc-structure.cpp
+++ b/flang/lib/Semantics/check-acc-structure.cpp
@@ -7,8 +7,13 @@
 //===----------------------------------------------------------------------===//
 #include "check-acc-structure.h"
 #include "flang/Common/enum-set.h"
+#include "flang/Evaluate/tools.h"
 #include "flang/Parser/parse-tree.h"
+#include "flang/Semantics/symbol.h"
 #include "flang/Semantics/tools.h"
+#include "flang/Support/Fortran.h"
+#include "llvm/Support/raw_ostream.h"
+#include <optional>
 
 #define CHECK_SIMPLE_CLAUSE(X, Y) \
   void AccStructureChecker::Enter(const parser::AccClause::X &) { \
@@ -342,21 +347,149 @@ void AccStructureChecker::Leave(const parser::OpenACCAtomicConstruct &x) {
   dirContext_.pop_back();
 }
 
-void AccStructureChecker::Enter(const parser::AccAtomicUpdate &x) {
-  const parser::AssignmentStmt &assignment{
-      std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement};
-  const auto &var{std::get<parser::Variable>(assignment.t)};
-  const auto &expr{std::get<parser::Expr>(assignment.t)};
+void AccStructureChecker::CheckAtomicStmt(
+    const parser::AssignmentStmt &assign, std::string &construct) {
+  const auto &var{std::get<parser::Variable>(assign.t)};
+  const auto &expr{std::get<parser::Expr>(assign.t)};
   const auto *rhs{GetExpr(context_, expr)};
   const auto *lhs{GetExpr(context_, var)};
-  if (lhs && rhs) {
-    if (lhs->Rank() != 0)
+
+  if (lhs) {
+    if (lhs->Rank() != 0) {
       context_.Say(expr.source,
-          "LHS of atomic update statement must be scalar"_err_en_US);
-    if (rhs->Rank() != 0)
+          "LHS of atomic %s statement must be scalar"_err_en_US, construct);
+    }
+    // TODO: Check if lhs is intrinsic type
+  }
+  if (rhs) {
+    if (rhs->Rank() != 0) {
       context_.Say(var.GetSource(),
-          "RHS of atomic update statement must be scalar"_err_en_US);
+          "RHS of atomic %s statement must be scalar"_err_en_US, construct);
+    }
+    // TODO: Check if lhs is intrinsic type
+  }
+}
+
+void AccStructureChecker::CheckAtomicUpdateStmt(
+    const parser::AssignmentStmt &assign) {
+  static std::string construct{"update"};
+  CheckAtomicStmt(assign, construct);
+}
+
+void AccStructureChecker::CheckAtomicWriteStmt(
+    const parser::AssignmentStmt &assign) {
+  static std::string construct{"write"};
+  CheckAtomicStmt(assign, construct);
+}
+
+void AccStructureChecker::CheckAtomicCaptureStmt(
+    const parser::AssignmentStmt &assign) {
+  static std::string construct{"capture"};
+  CheckAtomicStmt(assign, construct);
+}
+
+const parser::Variable *AccStructureChecker::GetIfAtomicUpdateVar(
+    const parser::AssignmentStmt &assign) const {
+  // OpenACC 3.4, 2893-2898
+  const auto &updatedVar{std::get<parser::Variable>(assign.t)};
+  const auto &rhs{std::get<parser::Expr>(assign.t)};
+
+  // Is the rhs something a valid operations that references the updatedVar?
+  const auto expr{GetExpr(context_, rhs)};
+  if (!expr) {
+    llvm::errs() << "IsAtomicUpdateStmt: no expr\n";
+    return nullptr;
+  }
+  const auto [op, args] = evaluate::GetTopLevelOperation(*expr);
+  switch (op) {
+  // 2909-2910 operator is one of +, *, -, /, .and., .or., .eqv., or .neqv..
+  case evaluate::operation::Operator::Add:
+  case evaluate::operation::Operator::Mul:
+  case evaluate::operation::Operator::Sub:
+  case evaluate::operation::Operator::Div:
+  case evaluate::operation::Operator::And:
+  case evaluate::operation::Operator::Or:
+  case evaluate::operation::Operator::Eqv:
+  case evaluate::operation::Operator::Neqv:
+  // 2909 intrinsic-procedure name is one of max, min, iand, ior, or ieor.
+  case evaluate::operation::Operator::Max:
+  case evaluate::operation::Operator::Min:
+    // case evaluate::operation::Operator::Iand:
+    // case evaluate::operation::Operator::Ior:
+    // case evaluate::operation::Operator::Ieor:
+    break;
+  // x = (x) is not allowed.
+  case evaluate::operation::Operator::Identity:
+  default:
+    return nullptr;
+  }
+  const auto &updatedExpr{GetExpr(context_, updatedVar)};
+  if (!updatedExpr) {
+    return nullptr;
+  }
+  for (const auto &arg : args) {
+    if (*updatedExpr == arg) {
+      return &updatedVar;
+    }
   }
+  return nullptr;
+}
+
+bool AccStructureChecker::IsAtomicCaptureStmt(
+    const parser::AssignmentStmt &assign,
+    const parser::Variable &updatedVar) const {
+  const auto &var{std::get<parser::Variable>(assign.t)};
+  const auto &expr{std::get<parser::Expr>(assign.t)};
+  const auto *varExpr{GetExpr(context_, var)};
+  const auto *updatedVarExpr{GetExpr(context_, updatedVar)};
+  const auto *exprExpr{GetExpr(context_, expr)};
+  if (!varExpr || !updatedVarExpr || !exprExpr) {
+    return false;
+  }
+  return updatedVarExpr == exprExpr && !(updatedVarExpr == varExpr);
+}
+
+void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
+  const Fortran::parser::AssignmentStmt &stmt1 =
+      std::get<Fortran::parser::AccAtomicCapture::Stmt1>(capture.t).v.statement;
+  const Fortran::parser::AssignmentStmt &stmt2 =
+      std::get<Fortran::parser::AccAtomicCapture::Stmt2>(capture.t).v.statement;
+  if (const parser::Variable *updatedVar{GetIfAtomicUpdateVar(stmt1)};
+      updatedVar && IsAtomicCaptureStmt(stmt2, *updatedVar)) {
+    CheckAtomicUpdateStmt(stmt1);
+    CheckAtomicCaptureStmt(stmt2);
+  } else if (const parser::Variable *updatedVar{GetIfAtomicUpdateVar(stmt2)};
+      updatedVar && IsAtomicCaptureStmt(stmt1, *updatedVar)) {
+    CheckAtomicCaptureStmt(stmt1);
+    CheckAtomicUpdateStmt(stmt2);
+  } else {
+    // Note, the write-statement when unified with is just an assignment
+    // statement x is unconstrained. This is moral equivalent of `if (...
+    // updatedVar{IsAtomicWriteStmt(stmt2)}; updatedVar && ...)`
+    const auto &updatedVarV{std::get<parser::Variable>(stmt2.t)};
+    if (IsAtomicCaptureStmt(stmt1, updatedVarV)) {
+      CheckAtomicCaptureStmt(stmt1);
+      CheckAtomicWriteStmt(stmt2);
+    } else {
+      context_.Say(std::get<parser::Verbatim>(capture.t).source,
+          "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
+    }
+  }
+}
+
+void AccStructureChecker::Enter(const parser::AccAtomicUpdate &x) {
+  CheckAtomicUpdateStmt(
+      std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement);
+}
+
+void AccStructureChecker::Enter(const parser::AccAtomicWrite &x) {
+  CheckAtomicWriteStmt(
+      std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement);
+}
+
+void AccStructureChecker::Enter(const parser::AccAtomicRead &x) {
+  CheckAtomicCaptureStmt(
+      std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement);
 }
 
 void AccStructureChecker::Enter(const parser::OpenACCCacheConstruct &x) {
diff --git a/flang/lib/Semantics/check-acc-structure.h b/flang/lib/Semantics/check-acc-structure.h
index 6a9aa01a9bb13..207311be85973 100644
--- a/flang/lib/Semantics/check-acc-structure.h
+++ b/flang/lib/Semantics/check-acc-structure.h
@@ -63,6 +63,9 @@ class AccStructureChecker
   void Enter(const parser::OpenACCCacheConstruct &);
   void Leave(const parser::OpenACCCacheConstruct &);
   void Enter(const parser::AccAtomicUpdate &);
+  void Enter(const parser::AccAtomicCapture &);
+  void Enter(const parser::AccAtomicWrite &);
+  void Enter(const parser::AccAtomicRead &);
   void Enter(const parser::OpenACCEndConstruct &);
 
   // Clauses
@@ -80,6 +83,20 @@ class AccStructureChecker
 #include "llvm/Frontend/OpenACC/ACC.inc"
 
 private:
+  bool IsAtomicUpdateOperator(
+      const parser::Expr &expr, const parser::Variable &updatedVar) const;
+  bool IsAtomicUpdateIntrinsic(
+      const parser::Expr &expr, const parser::Variable &updatedVar) const;
+  const parser::Variable *GetIfAtomicUpdateVar(
+      const parser::AssignmentStmt &assign) const;
+  bool IsAtomicCaptureStmt(const parser::AssignmentStmt &assign,
+      const parser::Variable &updatedVar) const;
+  void CheckAtomicStmt(
+      const parser::AssignmentStmt &assign, std::string &construct);
+  void CheckAtomicUpdateStmt(const parser::AssignmentStmt &assign);
+  void CheckAtomicCaptureStmt(const parser::AssignmentStmt &assign);
+  void CheckAtomicWriteStmt(const parser::AssignmentStmt &assign);
+
   bool CheckAllowedModifier(llvm::acc::Clause clause);
   bool IsComputeConstruct(llvm::acc::Directive directive) const;
   bool IsInsideComputeConstruct() const;
diff --git a/flang/test/Semantics/OpenACC/acc-atomic-validity.f90 b/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
index 07fb864695737..6e3cab41749db 100644
--- a/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
+++ b/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
@@ -54,6 +54,7 @@ program openacc_atomic_validity
   i = c(i)
   !$acc end atomic
 
+  !TODO: Should error because c(i) references i which is the atomic update variable.
   !$acc atomic capture
   c(i) = i
   i = i + 1

From f5dcc86342a30ba62e86954191835829f3f9df36 Mon Sep 17 00:00:00 2001
From: Andre Kuhlenschmidt <akuhlenschmi@nvidia.com>
Date: Mon, 28 Jul 2025 08:25:21 -0700
Subject: [PATCH 2/8] Still not ready but passes all tests

---
 flang/lib/Semantics/check-acc-structure.cpp   | 277 ++++++++++++++----
 flang/lib/Semantics/check-acc-structure.h     |  22 +-
 .../test/Lower/OpenACC/acc-atomic-capture.f90 |  28 +-
 .../test/Lower/OpenACC/acc-atomic-update.f90  |  89 ------
 .../Semantics/OpenACC/acc-atomic-validity.f90 |  42 +++
 5 files changed, 287 insertions(+), 171 deletions(-)
 delete mode 100644 flang/test/Lower/OpenACC/acc-atomic-update.f90

diff --git a/flang/lib/Semantics/check-acc-structure.cpp b/flang/lib/Semantics/check-acc-structure.cpp
index a1b877c64b968..66fbf50ef52c4 100644
--- a/flang/lib/Semantics/check-acc-structure.cpp
+++ b/flang/lib/Semantics/check-acc-structure.cpp
@@ -11,10 +11,16 @@
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/symbol.h"
 #include "flang/Semantics/tools.h"
+#include "flang/Semantics/type.h"
 #include "flang/Support/Fortran.h"
+#include "llvm/Support/AtomicOrdering.h"
 #include "llvm/Support/raw_ostream.h"
 #include <optional>
 
+// TODO: Move the parts that I am reusing from openmp-utils.h to
+// evaluate/tools.h
+#include "openmp-utils.h"
+
 #define CHECK_SIMPLE_CLAUSE(X, Y) \
   void AccStructureChecker::Enter(const parser::AccClause::X &) { \
     CheckAllowed(llvm::acc::Clause::Y); \
@@ -370,83 +376,162 @@ void AccStructureChecker::CheckAtomicStmt(
   }
 }
 
+static bool IsValidAtomicUpdateOperation(
+    const evaluate::operation::Operator &op) {
+  switch (op) {
+  case evaluate::operation::Operator::Add:
+  case evaluate::operation::Operator::Mul:
+  case evaluate::operation::Operator::Sub:
+  case evaluate::operation::Operator::Div:
+  case evaluate::operation::Operator::And:
+  case evaluate::operation::Operator::Or:
+  case evaluate::operation::Operator::Eqv:
+  case evaluate::operation::Operator::Neqv:
+  // 2909 intrinsic-procedure name is one of max, min, iand, ior, or ieor.
+  case evaluate::operation::Operator::Max:
+  case evaluate::operation::Operator::Min:
+    // Currently all get mapped to And, Or, and Neqv
+    // case evaluate::operation::Operator::Iand:
+    // case evaluate::operation::Operator::Ior:
+    // case evaluate::operation::Operator::Ieor:
+    return true;
+  case evaluate::operation::Operator::Convert:
+  case evaluate::operation::Operator::Identity:
+  default:
+    return false;
+  }
+}
+
+static bool IsConvertOperation(const evaluate::operation::Operator op) {
+  switch (op) {
+  case evaluate::operation::Operator::Convert:
+    return true;
+  default:
+    return false;
+  }
+}
+
+static SomeExpr GetExprModuloConversion(const SomeExpr &expr) {
+  const auto [op, args]{evaluate::GetTopLevelOperation(expr)};
+  // Check: if it is a conversion then it must have at least one argument.
+  CHECK((op != evaluate::operation::Operator::Convert || args.size() >= 1) &&
+      "Invalid conversion operation");
+  if (op == evaluate::operation::Operator::Convert && args.size() == 1) {
+    return args[0];
+  }
+  return expr;
+}
+
 void AccStructureChecker::CheckAtomicUpdateStmt(
-    const parser::AssignmentStmt &assign) {
+    const parser::AssignmentStmt &assign, const SomeExpr &updateVar,
+    const SomeExpr *captureVar) {
   static std::string construct{"update"};
   CheckAtomicStmt(assign, construct);
+  const auto &expr{std::get<parser::Expr>(assign.t)};
+  const auto *rhs{GetExpr(context_, expr)};
+  if (rhs) {
+    const auto [op, args]{
+        evaluate::GetTopLevelOperation(GetExprModuloConversion(*rhs))};
+    if (!IsValidAtomicUpdateOperation(op)) {
+      context_.Say(expr.source,
+          "Invalid atomic update operation, can only use: *, +, -, *, /, and, or, eqv, neqv, max, min, iand, ior, ieor"_err_en_US,
+          construct);
+      // TODO: Check that the updateVar is referenced in the args.
+      // TODO: Check that the captureVar is not referenced in the args.
+    }
+  }
 }
 
 void AccStructureChecker::CheckAtomicWriteStmt(
-    const parser::AssignmentStmt &assign) {
+    const parser::AssignmentStmt &assign, const SomeExpr &updateVar,
+    const SomeExpr *captureVar) {
   static std::string construct{"write"};
   CheckAtomicStmt(assign, construct);
+  const auto &expr{std::get<parser::Expr>(assign.t)};
+  const auto *rhs{GetExpr(context_, expr)};
+  if (rhs) {
+    if (omp::IsSubexpressionOf(updateVar, *rhs)) {
+      context_.Say(expr.source,
+          "The RHS of this atomic write statement cannot reference the updated variable: %s"_err_en_US,
+          updateVar.AsFortran());
+    }
+    // TODO: Check w/ Valintin about semantic there are a lot of lowering tests
+    // that seem to violate the spec.
+    // if (captureVar && omp::IsSubexpressionOf(*captureVar, *rhs)) {
+    //  context_.Say(expr.source,
+    //      "The RHS of this atomic write statement cannot reference the capture
+    //      variable: %s"_err_en_US, captureVar->AsFortran());
+    //}
+  }
 }
 
+// Todo, with an appropriate extractor this would be pretty easy to check
+// and provide updateVar in a context sensitive way. I would probably need to
+// to switch the arguments to be evaluate::Exprs.
 void AccStructureChecker::CheckAtomicCaptureStmt(
-    const parser::AssignmentStmt &assign) {
+    const parser::AssignmentStmt &assign, const SomeExpr *updateVar,
+    const SomeExpr &captureVar) {
   static std::string construct{"capture"};
   CheckAtomicStmt(assign, construct);
 }
 
-const parser::Variable *AccStructureChecker::GetIfAtomicUpdateVar(
-    const parser::AssignmentStmt &assign) const {
+// If the updatedVar is null, then we don't bother checking for updates to it.
+// Just that the op is a valid atomic update operator.
+bool AccStructureChecker::IsAtomicUpdateExpr(
+    const evaluate::Expr<evaluate::SomeType> &expr,
+    const evaluate::Expr<evaluate::SomeType> *updatedVar,
+    bool allowConvert) const {
+  const auto [op, args]{evaluate::GetTopLevelOperation(expr)};
+  if (!IsValidAtomicUpdateOperation(op)) {
+    if (IsConvertOperation(op) && args.size() == 1) {
+      return IsAtomicUpdateExpr(args[0], updatedVar, /*allowConvert=*/false);
+    }
+    return false;
+  }
+  if (!updatedVar) {
+    return true;
+  }
+  for (const auto &arg : args) {
+    if (*updatedVar == arg) {
+      return true;
+    }
+  }
+  return false;
+}
+
+// The allowNonUpdate argument is used to generate slightly more helpful error
+// messages when the rhs is not an update to the updatedVar.
+const parser::Variable *AccStructureChecker::GetUpdatedVarIfAtomicUpdateStmt(
+    const parser::AssignmentStmt &assign, bool allowNonUpdate) const {
   // OpenACC 3.4, 2893-2898
   const auto &updatedVar{std::get<parser::Variable>(assign.t)};
   const auto &rhs{std::get<parser::Expr>(assign.t)};
 
   // Is the rhs something a valid operations that references the updatedVar?
-  const auto expr{GetExpr(context_, rhs)};
-  if (!expr) {
-    llvm::errs() << "IsAtomicUpdateStmt: no expr\n";
-    return nullptr;
-  }
-  const auto [op, args] = evaluate::GetTopLevelOperation(*expr);
-  switch (op) {
-  // 2909-2910 operator is one of +, *, -, /, .and., .or., .eqv., or .neqv..
-  case evaluate::operation::Operator::Add:
-  case evaluate::operation::Operator::Mul:
-  case evaluate::operation::Operator::Sub:
-  case evaluate::operation::Operator::Div:
-  case evaluate::operation::Operator::And:
-  case evaluate::operation::Operator::Or:
-  case evaluate::operation::Operator::Eqv:
-  case evaluate::operation::Operator::Neqv:
-  // 2909 intrinsic-procedure name is one of max, min, iand, ior, or ieor.
-  case evaluate::operation::Operator::Max:
-  case evaluate::operation::Operator::Min:
-    // case evaluate::operation::Operator::Iand:
-    // case evaluate::operation::Operator::Ior:
-    // case evaluate::operation::Operator::Ieor:
-    break;
-  // x = (x) is not allowed.
-  case evaluate::operation::Operator::Identity:
-  default:
+  const auto *expr{GetExpr(context_, rhs)};
+  const auto *updatedExpr{GetExpr(context_, updatedVar)};
+  if (!expr || !updatedExpr) {
     return nullptr;
   }
-  const auto &updatedExpr{GetExpr(context_, updatedVar)};
-  if (!updatedExpr) {
-    return nullptr;
-  }
-  for (const auto &arg : args) {
-    if (*updatedExpr == arg) {
-      return &updatedVar;
-    }
+  if (IsAtomicUpdateExpr(*expr, allowNonUpdate ? nullptr : updatedExpr,
+          /*allowConvert=*/true)) {
+    return &updatedVar;
   }
   return nullptr;
 }
 
 bool AccStructureChecker::IsAtomicCaptureStmt(
     const parser::AssignmentStmt &assign,
-    const parser::Variable &updatedVar) const {
+    const parser::Variable &updateVar) const {
   const auto &var{std::get<parser::Variable>(assign.t)};
   const auto &expr{std::get<parser::Expr>(assign.t)};
   const auto *varExpr{GetExpr(context_, var)};
-  const auto *updatedVarExpr{GetExpr(context_, updatedVar)};
+  const auto *updatedVarExpr{GetExpr(context_, updateVar)};
   const auto *exprExpr{GetExpr(context_, expr)};
   if (!varExpr || !updatedVarExpr || !exprExpr) {
     return false;
   }
-  return updatedVarExpr == exprExpr && !(updatedVarExpr == varExpr);
+  return *updatedVarExpr == *exprExpr && !(*updatedVarExpr == *varExpr);
 }
 
 void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
@@ -454,42 +539,104 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
       std::get<Fortran::parser::AccAtomicCapture::Stmt1>(capture.t).v.statement;
   const Fortran::parser::AssignmentStmt &stmt2 =
       std::get<Fortran::parser::AccAtomicCapture::Stmt2>(capture.t).v.statement;
-  if (const parser::Variable *updatedVar{GetIfAtomicUpdateVar(stmt1)};
-      updatedVar && IsAtomicCaptureStmt(stmt2, *updatedVar)) {
-    CheckAtomicUpdateStmt(stmt1);
-    CheckAtomicCaptureStmt(stmt2);
-  } else if (const parser::Variable *updatedVar{GetIfAtomicUpdateVar(stmt2)};
-      updatedVar && IsAtomicCaptureStmt(stmt1, *updatedVar)) {
-    CheckAtomicCaptureStmt(stmt1);
-    CheckAtomicUpdateStmt(stmt2);
-  } else {
-    // Note, the write-statement when unified with is just an assignment
-    // statement x is unconstrained. This is moral equivalent of `if (...
-    // updatedVar{IsAtomicWriteStmt(stmt2)}; updatedVar && ...)`
-    const auto &updatedVarV{std::get<parser::Variable>(stmt2.t)};
-    if (IsAtomicCaptureStmt(stmt1, updatedVarV)) {
-      CheckAtomicCaptureStmt(stmt1);
-      CheckAtomicWriteStmt(stmt2);
-    } else {
+  const auto &var1{std::get<parser::Variable>(stmt1.t)};
+  const auto &var2{std::get<parser::Variable>(stmt2.t)};
+  const auto *lhs1{GetExpr(context_, var1)};
+  const auto *lhs2{GetExpr(context_, var2)};
+  if (!lhs1 || !lhs2) {
+    // Not enough information to check.
+    return;
+  }
+  if (*lhs1 == *lhs2) {
+    context_.Say(std::get<parser::Verbatim>(capture.t).source,
+        "The variables assigned to in this atomic capture construct must be distinct"_err_en_US);
+    return;
+  }
+  const auto &expr1{std::get<parser::Expr>(stmt1.t)};
+  const auto &expr2{std::get<parser::Expr>(stmt2.t)};
+  const auto *rhs1{GetExpr(context_, expr1)};
+  const auto *rhs2{GetExpr(context_, expr2)};
+  if (!rhs1 || !rhs2) {
+    return;
+  }
+  bool stmt1CapturesLhs2{*lhs2 == GetExprModuloConversion(*rhs1)};
+  bool stmt2CapturesLhs1{*lhs1 == GetExprModuloConversion(*rhs2)};
+  if (stmt1CapturesLhs2 && !stmt2CapturesLhs1) {
+    if (*lhs2 == GetExprModuloConversion(*rhs2)) {
+      // y = x; x = x; Doesn't fit the spec;
       context_.Say(std::get<parser::Verbatim>(capture.t).source,
           "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
+      // TODO: Add attatchment that x = x is not considered an update.
+    } else if (omp::IsSubexpressionOf(*lhs2, *rhs2)) {
+      // Take y = x; x = <expr w/ x> as capture; update
+      const auto &updateVar{*lhs2};
+      const auto &captureVar{*lhs1};
+      CheckAtomicCaptureStmt(stmt1, &updateVar, captureVar);
+      CheckAtomicUpdateStmt(stmt2, updateVar, &captureVar);
+    } else {
+      // Take y = x; x = <expr w/o x> as capture; write
+      const auto &updateVar{*lhs2};
+      const auto &captureVar{*lhs1};
+      CheckAtomicCaptureStmt(stmt1, &updateVar, captureVar);
+      CheckAtomicWriteStmt(stmt2, updateVar, &captureVar);
     }
+  } else if (stmt2CapturesLhs1 && !stmt1CapturesLhs2) {
+    if (*lhs1 == GetExprModuloConversion(*rhs1)) {
+      // Error x = x; y = x;
+      context_.Say(var1.GetSource(),
+          "The first assignment in this atomic capture construct doesn't perform a valid update"_err_en_US);
+      // TODO: Add attatchment that x = x is not considered an update.
+      return;
+    } else {
+      // Take x = <expr>; y = x; as update; capture
+      const auto &updateVar{*lhs1};
+      const auto &captureVar{*lhs2};
+      CheckAtomicUpdateStmt(stmt1, updateVar, &captureVar);
+      CheckAtomicCaptureStmt(stmt2, &updateVar, captureVar);
+    }
+  } else if (stmt1CapturesLhs2 && stmt2CapturesLhs1) {
+    // x1 = x2; x2 = x1; Doesn't fit the spec;
+    context_.Say(std::get<parser::Verbatim>(capture.t).source,
+        "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
+  } else {
+    // y1 = <expr != y2>; y2 = <expr != y1>; Doesn't fit the spec
+    context_.Say(std::get<parser::Verbatim>(capture.t).source,
+        "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
   }
+  return;
 }
 
 void AccStructureChecker::Enter(const parser::AccAtomicUpdate &x) {
-  CheckAtomicUpdateStmt(
-      std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement);
+  const auto &assign{
+      std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement};
+  const auto &var{std::get<parser::Variable>(assign.t)};
+  const auto *updateVar{GetExpr(context_, var)};
+  if (!updateVar) {
+    return;
+  }
+  CheckAtomicUpdateStmt(assign, *updateVar, nullptr);
 }
 
 void AccStructureChecker::Enter(const parser::AccAtomicWrite &x) {
-  CheckAtomicWriteStmt(
-      std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement);
+  const auto &assign{
+      std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement};
+  const auto &var{std::get<parser::Variable>(assign.t)};
+  const auto *updateVar{GetExpr(context_, var)};
+  if (!updateVar) {
+    return;
+  }
+  CheckAtomicWriteStmt(assign, *updateVar, nullptr);
 }
 
 void AccStructureChecker::Enter(const parser::AccAtomicRead &x) {
-  CheckAtomicCaptureStmt(
-      std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement);
+  const auto &assign{
+      std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement};
+  const auto &var{std::get<parser::Variable>(assign.t)};
+  const auto *captureVar{GetExpr(context_, var)};
+  if (!captureVar) {
+    return;
+  }
+  CheckAtomicCaptureStmt(assign, nullptr, *captureVar);
 }
 
 void AccStructureChecker::Enter(const parser::OpenACCCacheConstruct &x) {
diff --git a/flang/lib/Semantics/check-acc-structure.h b/flang/lib/Semantics/check-acc-structure.h
index 207311be85973..8f74369e61b5e 100644
--- a/flang/lib/Semantics/check-acc-structure.h
+++ b/flang/lib/Semantics/check-acc-structure.h
@@ -87,15 +87,27 @@ class AccStructureChecker
       const parser::Expr &expr, const parser::Variable &updatedVar) const;
   bool IsAtomicUpdateIntrinsic(
       const parser::Expr &expr, const parser::Variable &updatedVar) const;
-  const parser::Variable *GetIfAtomicUpdateVar(
-      const parser::AssignmentStmt &assign) const;
+  bool IsAtomicUpdateExpr(const evaluate::Expr<evaluate::SomeType> &expr,
+      const evaluate::Expr<evaluate::SomeType> *updatedVar,
+      bool allowConvert) const;
+  const parser::Variable *GetUpdatedVarIfAtomicUpdateStmt(
+      const parser::AssignmentStmt &assign, bool allowNonUpdate) const;
   bool IsAtomicCaptureStmt(const parser::AssignmentStmt &assign,
       const parser::Variable &updatedVar) const;
   void CheckAtomicStmt(
       const parser::AssignmentStmt &assign, std::string &construct);
-  void CheckAtomicUpdateStmt(const parser::AssignmentStmt &assign);
-  void CheckAtomicCaptureStmt(const parser::AssignmentStmt &assign);
-  void CheckAtomicWriteStmt(const parser::AssignmentStmt &assign);
+  void CheckAtomicUpdateStmt(const parser::AssignmentStmt &assign,
+      const SomeExpr &updateVar, const SomeExpr *captureVar);
+  void CheckAtomicCaptureStmt(const parser::AssignmentStmt &assign,
+      const SomeExpr *updateVar, const SomeExpr &captureVar);
+  void CheckAtomicWriteStmt(const parser::AssignmentStmt &assign,
+      const SomeExpr &updateVar, const SomeExpr *captureVar);
+  void CheckAtomicUpdateVariable(
+      const parser::Variable &updateVar, const parser::Variable &captureVar);
+  void CheckAtomicCaptureVariable(
+      const parser::Variable &captureVar, const parser::Variable &updateVar);
+  bool DoesExprReferenceVar(const evaluate::Expr<evaluate::SomeType> &expr,
+      const evaluate::Expr<evaluate::SomeType> &var) const;
 
   bool CheckAllowedModifier(llvm::acc::Clause clause);
   bool IsComputeConstruct(llvm::acc::Directive directive) const;
diff --git a/flang/test/Lower/OpenACC/acc-atomic-capture.f90 b/flang/test/Lower/OpenACC/acc-atomic-capture.f90
index ee38ab6ce826a..30e60e34b13a2 100644
--- a/flang/test/Lower/OpenACC/acc-atomic-capture.f90
+++ b/flang/test/Lower/OpenACC/acc-atomic-capture.f90
@@ -123,17 +123,20 @@ subroutine capture_with_convert_f32_to_i32()
 ! CHECK: }
 
 subroutine capture_with_convert_i32_to_f64()
-  real(8) :: x
-  integer :: v
+  real(8) :: x 
+  integer :: v, u 
   x = 1.0
   v = 0
+  u = 1
   !$acc atomic capture
   v = x
-  x = v
+  x = u
   !$acc end atomic
 end subroutine capture_with_convert_i32_to_f64
 
 ! CHECK-LABEL: func.func @_QPcapture_with_convert_i32_to_f64()
+! CHECK: %[[U:.*]] = fir.alloca i32 {bindc_name = "u", uniq_name = "_QFcapture_with_convert_i32_to_f64Eu"}
+! CHECK: %[[U_DECL:.*]]:2 = hlfir.declare %[[U]] {uniq_name = "_QFcapture_with_convert_i32_to_f64Eu"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 ! CHECK: %[[V:.*]] = fir.alloca i32 {bindc_name = "v", uniq_name = "_QFcapture_with_convert_i32_to_f64Ev"}
 ! CHECK: %[[V_DECL:.*]]:2 = hlfir.declare %[[V]] {uniq_name = "_QFcapture_with_convert_i32_to_f64Ev"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 ! CHECK: %[[X:.*]] = fir.alloca f64 {bindc_name = "x", uniq_name = "_QFcapture_with_convert_i32_to_f64Ex"}
@@ -142,7 +145,9 @@ end subroutine capture_with_convert_i32_to_f64
 ! CHECK: hlfir.assign %[[CST]] to %[[X_DECL]]#0 : f64, !fir.ref<f64>
 ! CHECK: %c0_i32 = arith.constant 0 : i32
 ! CHECK: hlfir.assign %c0_i32 to %[[V_DECL]]#0 : i32, !fir.ref<i32>
-! CHECK: %[[LOAD:.*]] = fir.load %[[V_DECL]]#0 : !fir.ref<i32>
+! CHECK: %c1_i32 = arith.constant 1 : i32
+! CHECK: hlfir.assign %c1_i32 to %[[U_DECL]]#0 : i32, !fir.ref<i32>
+! CHECK: %[[LOAD:.*]] = fir.load %[[U_DECL]]#0 : !fir.ref<i32>
 ! CHECK: %[[CONV:.*]] = fir.convert %[[LOAD]] : (i32) -> f64
 ! CHECK: acc.atomic.capture {
 ! CHECK:   acc.atomic.read %[[V_DECL]]#0 = %[[X_DECL]]#0 : !fir.ref<i32>, !fir.ref<f64>, f64
@@ -155,7 +160,7 @@ subroutine capture_with_convert_f64_to_i32()
   x = 1
   v = 0
   !$acc atomic capture
-  x = v * v
+  x = x * 2.0_8
   v = x
   !$acc end atomic
 end subroutine capture_with_convert_f64_to_i32
@@ -167,15 +172,14 @@ end subroutine capture_with_convert_f64_to_i32
 ! CHECK: %[[X_DECL:.*]]:2 = hlfir.declare %[[X]] {uniq_name = "_QFcapture_with_convert_f64_to_i32Ex"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 ! CHECK: %c1_i32 = arith.constant 1 : i32
 ! CHECK: hlfir.assign %c1_i32 to %[[X_DECL]]#0 : i32, !fir.ref<i32>
-! CHECK: %[[CST:.*]] = arith.constant 0.000000e+00 : f64
-! CHECK: hlfir.assign %[[CST]] to %[[V_DECL]]#0 : f64, !fir.ref<f64>
-! CHECK: %[[LOAD:.*]] = fir.load %[[V_DECL]]#0 : !fir.ref<f64>
+! CHECK: %[[CST:.*]] = arith.constant 2.000000e+00 : f64
 ! CHECK: acc.atomic.capture {
 ! CHECK:   acc.atomic.update %[[X_DECL]]#0 : !fir.ref<i32> {
-! CHECK:   ^bb0(%arg0: i32):
-! CHECK:     %[[MUL:.*]] = arith.mulf %[[LOAD]], %[[LOAD]] fastmath<contract> : f64
-! CHECK:     %[[CONV:.*]] = fir.convert %[[MUL]] : (f64) -> i32
-! CHECK:     acc.yield %[[CONV]] : i32
+! CHECK:   ^bb0(%[[ARG:.*]]: i32):
+! CHECK:     %[[CONV_ARG:.*]] = fir.convert %[[ARG]] : (i32) -> f64
+! CHECK:     %[[MUL:.*]] = arith.mulf %[[CONV_ARG]], %[[CST]] fastmath<contract> : f64
+! CHECK:     %[[CONV_MUL:.*]] = fir.convert %[[MUL]] : (f64) -> i32
+! CHECK:     acc.yield %[[CONV_MUL]] : i32
 ! CHECK:   }
 ! CHECK:   acc.atomic.read %[[V_DECL]]#0 = %[[X_DECL]]#0 : !fir.ref<f64>, !fir.ref<i32>, i32
 ! CHECK: }
diff --git a/flang/test/Lower/OpenACC/acc-atomic-update.f90 b/flang/test/Lower/OpenACC/acc-atomic-update.f90
deleted file mode 100644
index 71aa69fd64eba..0000000000000
--- a/flang/test/Lower/OpenACC/acc-atomic-update.f90
+++ /dev/null
@@ -1,89 +0,0 @@
-! This test checks lowering of atomic and atomic update constructs
-! RUN: bbc -fopenacc -emit-hlfir %s -o - | FileCheck %s
-! RUN: %flang_fc1 -fopenacc -emit-hlfir %s -o - | FileCheck %s
-
-program acc_atomic_update_test
-    interface
-       integer function func(x)
-         integer :: x
-       end function func
-    end interface
-    integer :: x, y, z
-    integer, pointer :: a, b
-    integer, target :: c, d
-    integer(1) :: i1
-
-    a=>c
-    b=>d
-
-!CHECK: %[[A:.*]] = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "a", uniq_name = "_QFEa"}
-!CHECK: %[[A_DECL:.*]]:2 = hlfir.declare %[[A]] {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFEa"} : (!fir.ref<!fir.box<!fir.ptr<i32>>>) -> (!fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.ref<!fir.box<!fir.ptr<i32>>>)
-!CHECK: %[[B:.*]] = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "b", uniq_name = "_QFEb"}
-!CHECK: %[[B_DECL:.*]]:2 = hlfir.declare %[[B]] {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFEb"} : (!fir.ref<!fir.box<!fir.ptr<i32>>>) -> (!fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.ref<!fir.box<!fir.ptr<i32>>>)
-!CHECK: %[[C_ADDR:.*]] = fir.address_of(@_QFEc) : !fir.ref<i32>
-!CHECK: %[[D_ADDR:.*]] = fir.address_of(@_QFEd) : !fir.ref<i32>
-!CHECK: %[[I1:.*]] = fir.alloca i8 {bindc_name = "i1", uniq_name = "_QFEi1"}
-!CHECK: %[[I1_DECL:.*]]:2 = hlfir.declare %[[I1]] {uniq_name = "_QFEi1"} : (!fir.ref<i8>) -> (!fir.ref<i8>, !fir.ref<i8>)
-!CHECK: %[[X:.*]] = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFEx"}
-!CHECK: %[[X_DECL:.*]]:2 = hlfir.declare %[[X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
-!CHECK: %[[Y:.*]] = fir.alloca i32 {bindc_name = "y", uniq_name = "_QFEy"}
-!CHECK: %[[Y_DECL:.*]]:2 = hlfir.declare %[[Y]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
-!CHECK: %[[Z:.*]] = fir.alloca i32 {bindc_name = "z", uniq_name = "_QFEz"}
-!CHECK: %[[Z_DECL:.*]]:2 = hlfir.declare %[[Z]] {uniq_name = "_QFEz"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
-!CHECK: %[[LOAD_A:.*]] = fir.load %[[A_DECL]]#0 : !fir.ref<!fir.box<!fir.ptr<i32>>>
-!CHECK: %[[BOX_ADDR_A:.*]] = fir.box_addr %[[LOAD_A]] : (!fir.box<!fir.ptr<i32>>) -> !fir.ptr<i32>
-!CHECK: %[[LOAD_B:.*]] = fir.load %[[B_DECL]]#0 : !fir.ref<!fir.box<!fir.ptr<i32>>>
-!CHECK: %[[BOX_ADDR_B:.*]] = fir.box_addr %[[LOAD_B]] : (!fir.box<!fir.ptr<i32>>) -> !fir.ptr<i32>
-!CHECK: %[[LOAD_BOX_ADDR_B:.*]] = fir.load %[[BOX_ADDR_B]] : !fir.ptr<i32>
-!CHECK: acc.atomic.update %[[BOX_ADDR_A]] : !fir.ptr<i32> {
-!CHECK: ^bb0(%[[ARG0:.*]]: i32):
-!CHECK:   %[[ADD:.*]] = arith.addi %[[ARG0]], %[[LOAD_BOX_ADDR_B]] : i32
-!CHECK:   acc.yield %[[ADD]] : i32
-!CHECK: }
-
-    !$acc atomic update
-        a = a + b 
-
-!CHECK: {{.*}} = arith.constant 1 : i32
-!CHECK: acc.atomic.update %[[Y_DECL]]#0 : !fir.ref<i32> {
-!CHECK:  ^bb0(%[[ARG:.*]]: i32):
-!CHECK:    %[[RESULT:.*]] = arith.addi %[[ARG]], {{.*}} : i32
-!CHECK:    acc.yield %[[RESULT]] : i32
-!CHECK:  }
-!CHECK:  %[[LOADED_X:.*]] = fir.load %[[X_DECL]]#0 : !fir.ref<i32>
-!CHECK:  acc.atomic.update %[[Z_DECL]]#0 : !fir.ref<i32> {
-!CHECK:  ^bb0(%[[ARG:.*]]: i32):
-!CHECK:    %[[RESULT:.*]] = arith.muli %[[LOADED_X]], %[[ARG]] : i32
-!CHECK:    acc.yield %[[RESULT]] : i32
-!CHECK:  }
-    !$acc atomic 
-        y = y + 1
-    !$acc atomic update
-        z = x * z 
-
-!CHECK:  %[[C1_VAL:.*]] = arith.constant 1 : i32
-!CHECK:  acc.atomic.update %[[I1_DECL]]#0 : !fir.ref<i8> {
-!CHECK:  ^bb0(%[[VAL:.*]]: i8):
-!CHECK:    %[[CVT_VAL:.*]] = fir.convert %[[VAL]] : (i8) -> i32
-!CHECK:    %[[ADD_VAL:.*]] = arith.addi %[[CVT_VAL]], %[[C1_VAL]] : i32
-!CHECK:    %[[UPDATED_VAL:.*]] = fir.convert %[[ADD_VAL]] : (i32) -> i8
-!CHECK:    acc.yield %[[UPDATED_VAL]] : i8
-!CHECK:  }
-    !$acc atomic
-      i1 = i1 + 1
-    !$acc end atomic
-
-!CHECK:  %[[VAL_44:.*]]:3 = hlfir.associate %{{.*}} {adapt.valuebyref} : (i32) -> (!fir.ref<i32>, !fir.ref<i32>, i1)
-!CHECK:  %[[VAL_45:.*]] = fir.call @_QPfunc(%[[VAL_44]]#0) fastmath<contract> : (!fir.ref<i32>) -> i32
-!CHECK:  acc.atomic.update %[[X_DECL]]#0 : !fir.ref<i32> {
-!CHECK:  ^bb0(%[[VAL_46:.*]]: i32):
-!CHECK:    %[[VAL_47:.*]] = arith.addi %[[VAL_46]], %[[VAL_45]] : i32
-!CHECK:    acc.yield %[[VAL_47]] : i32
-!CHECK:  }
-!CHECK:  hlfir.end_associate %[[VAL_44]]#1, %[[VAL_44]]#2 : !fir.ref<i32>, i1
-    !$acc atomic update
-    x = x + func(z + 1)
-    !$acc end atomic
-!CHECK:  return
-!CHECK: }
-end program acc_atomic_update_test
diff --git a/flang/test/Semantics/OpenACC/acc-atomic-validity.f90 b/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
index 6e3cab41749db..276042ae39980 100644
--- a/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
+++ b/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
@@ -80,3 +80,45 @@ program openacc_atomic_validity
   !$acc end parallel
 
 end program openacc_atomic_validity
+
+subroutine capture_with_convert_f64_to_i32()
+  integer :: x
+  real(8) :: v, w
+  x = 1
+  v = 0
+  w = 2
+
+  !$acc atomic capture
+  x = x * 2.5_8
+  v = x
+  !$acc end atomic
+
+  !$acc atomic capture
+  !TODO: The rhs side of this update statement cannot reference v.
+  x = x * v
+  v = x
+  !$acc end atomic
+
+  !$acc atomic capture
+  !TODO: The rhs side of this update statement cannot reference v.
+  x = v * x
+  v = x
+  !$acc end atomic
+
+  !$acc atomic capture
+  !TODO: the rhs side of this update statement should reference x.
+  x = v * v
+  v = x
+  !$acc end atomic
+
+  !$acc atomic capture
+  x = v
+  !TODO: the rhs hand side of this write expression is not allowed to read v.
+  v = v * v
+  !$acc end atomic
+
+  !$acc atomic capture
+  v = x
+  x = w * w
+  !$acc end atomic
+end subroutine capture_with_convert_f64_to_i32
\ No newline at end of file

From 4e114f56d3f4ee4ca22c5ced9ef41cedfd7295cb Mon Sep 17 00:00:00 2001
From: Andre Kuhlenschmidt <akuhlenschmi@nvidia.com>
Date: Mon, 28 Jul 2025 08:59:19 -0700
Subject: [PATCH 3/8] Tidy Up TODOs

---
 flang/lib/Semantics/check-acc-structure.cpp | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/flang/lib/Semantics/check-acc-structure.cpp b/flang/lib/Semantics/check-acc-structure.cpp
index 66fbf50ef52c4..91feab3f4fe6a 100644
--- a/flang/lib/Semantics/check-acc-structure.cpp
+++ b/flang/lib/Semantics/check-acc-structure.cpp
@@ -436,8 +436,12 @@ void AccStructureChecker::CheckAtomicUpdateStmt(
       context_.Say(expr.source,
           "Invalid atomic update operation, can only use: *, +, -, *, /, and, or, eqv, neqv, max, min, iand, ior, ieor"_err_en_US,
           construct);
+    } else {
       // TODO: Check that the updateVar is referenced in the args.
       // TODO: Check that the captureVar is not referenced in the args.
+      // TODO: I ran into a case where equality gave me a backtrace...
+      // TODO: Seems like captureVar must be checked for equality module
+      // conversion too.
     }
   }
 }
@@ -567,6 +571,7 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
       context_.Say(std::get<parser::Verbatim>(capture.t).source,
           "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
       // TODO: Add attatchment that x = x is not considered an update.
+      // TODO: Add attatchment that y = x seems to be a capture.
     } else if (omp::IsSubexpressionOf(*lhs2, *rhs2)) {
       // Take y = x; x = <expr w/ x> as capture; update
       const auto &updateVar{*lhs2};
@@ -586,6 +591,7 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
       context_.Say(var1.GetSource(),
           "The first assignment in this atomic capture construct doesn't perform a valid update"_err_en_US);
       // TODO: Add attatchment that x = x is not considered an update.
+      // TODO: Add attatchment that y = x seems to be a capture.
       return;
     } else {
       // Take x = <expr>; y = x; as update; capture
@@ -598,10 +604,12 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
     // x1 = x2; x2 = x1; Doesn't fit the spec;
     context_.Say(std::get<parser::Verbatim>(capture.t).source,
         "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
+    // TODO: Add attatchment that both assignments seem to be captures.
   } else {
     // y1 = <expr != y2>; y2 = <expr != y1>; Doesn't fit the spec
     context_.Say(std::get<parser::Verbatim>(capture.t).source,
         "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
+    // TODO: Add attatchment that neither assignment seems to be a capture.
   }
   return;
 }

From df59fe92ff02acc1c2e9067b5cb0d77f10525538 Mon Sep 17 00:00:00 2001
From: Andre Kuhlenschmidt <akuhlenschmi@nvidia.com>
Date: Mon, 28 Jul 2025 14:15:30 -0700
Subject: [PATCH 4/8] last bit of cleanup

---
 flang/include/flang/Evaluate/tools.h          |   4 +
 flang/lib/Evaluate/tools.cpp                  |  29 ++++
 flang/lib/Semantics/check-acc-structure.cpp   | 146 ++++++------------
 flang/lib/Semantics/check-acc-structure.h     |  13 --
 flang/lib/Semantics/check-omp-atomic.cpp      |   6 +-
 flang/lib/Semantics/openmp-utils.cpp          |  26 ----
 flang/lib/Semantics/openmp-utils.h            |   1 -
 .../Semantics/OpenACC/acc-atomic-validity.f90 |  30 +++-
 8 files changed, 113 insertions(+), 142 deletions(-)

diff --git a/flang/include/flang/Evaluate/tools.h b/flang/include/flang/Evaluate/tools.h
index 96ed86f468350..43ad8707fa2b8 100644
--- a/flang/include/flang/Evaluate/tools.h
+++ b/flang/include/flang/Evaluate/tools.h
@@ -1512,6 +1512,10 @@ GetTopLevelOperation(const Expr<SomeType> &expr);
 // Check if expr is same as x, or a sequence of Convert operations on x.
 bool IsSameOrConvertOf(const Expr<SomeType> &expr, const Expr<SomeType> &x);
 
+// Check if the Variable appears as a subexpression of the expression.
+bool IsVarSubexpressionOf(
+    const Expr<SomeType> &var, const Expr<SomeType> &super);
+
 // Strip away any top-level Convert operations (if any exist) and return
 // the input value. A ComplexConstructor(x, 0) is also considered as a
 // convert operation.
diff --git a/flang/lib/Evaluate/tools.cpp b/flang/lib/Evaluate/tools.cpp
index 21e6b3c3dd50d..c34bf8b8ebfca 100644
--- a/flang/lib/Evaluate/tools.cpp
+++ b/flang/lib/Evaluate/tools.cpp
@@ -1936,6 +1936,35 @@ bool IsSameOrConvertOf(const Expr<SomeType> &expr, const Expr<SomeType> &x) {
     return false;
   }
 }
+
+struct VariableFinder : public evaluate::AnyTraverse<VariableFinder> {
+  using Base = evaluate::AnyTraverse<VariableFinder>;
+  using SomeExpr = Expr<SomeType>;
+  VariableFinder(const SomeExpr &v) : Base(*this), var(v) {}
+
+  using Base::operator();
+
+  template <typename T>
+  bool operator()(const evaluate::Designator<T> &x) const {
+    auto copy{x};
+    return evaluate::AsGenericExpr(std::move(copy)) == var;
+  }
+
+  template <typename T>
+  bool operator()(const evaluate::FunctionRef<T> &x) const {
+    auto copy{x};
+    return evaluate::AsGenericExpr(std::move(copy)) == var;
+  }
+
+private:
+  const SomeExpr &var;
+};
+
+bool IsVarSubexpressionOf(
+    const Expr<SomeType> &sub, const Expr<SomeType> &super) {
+  return VariableFinder{sub}(super);
+}
+
 } // namespace Fortran::evaluate
 
 namespace Fortran::semantics {
diff --git a/flang/lib/Semantics/check-acc-structure.cpp b/flang/lib/Semantics/check-acc-structure.cpp
index 91feab3f4fe6a..3a32ced5f7980 100644
--- a/flang/lib/Semantics/check-acc-structure.cpp
+++ b/flang/lib/Semantics/check-acc-structure.cpp
@@ -372,7 +372,7 @@ void AccStructureChecker::CheckAtomicStmt(
       context_.Say(var.GetSource(),
           "RHS of atomic %s statement must be scalar"_err_en_US, construct);
     }
-    // TODO: Check if lhs is intrinsic type
+    // TODO: Check if rhs is intrinsic type
   }
 }
 
@@ -402,15 +402,6 @@ static bool IsValidAtomicUpdateOperation(
   }
 }
 
-static bool IsConvertOperation(const evaluate::operation::Operator op) {
-  switch (op) {
-  case evaluate::operation::Operator::Convert:
-    return true;
-  default:
-    return false;
-  }
-}
-
 static SomeExpr GetExprModuloConversion(const SomeExpr &expr) {
   const auto [op, args]{evaluate::GetTopLevelOperation(expr)};
   // Check: if it is a conversion then it must have at least one argument.
@@ -437,11 +428,35 @@ void AccStructureChecker::CheckAtomicUpdateStmt(
           "Invalid atomic update operation, can only use: *, +, -, *, /, and, or, eqv, neqv, max, min, iand, ior, ieor"_err_en_US,
           construct);
     } else {
-      // TODO: Check that the updateVar is referenced in the args.
-      // TODO: Check that the captureVar is not referenced in the args.
-      // TODO: I ran into a case where equality gave me a backtrace...
-      // TODO: Seems like captureVar must be checked for equality module
-      // conversion too.
+      bool foundUpdateVar{false};
+      for (const auto &arg : args) {
+        if (updateVar == GetExprModuloConversion(arg)) {
+          if (foundUpdateVar) {
+            context_.Say(expr.source,
+                "The updated variable, %s, cannot appear more than once in the atomic update operation"_err_en_US,
+                updateVar.AsFortran());
+          } else {
+            foundUpdateVar = true;
+          }
+        } else if (evaluate::IsVarSubexpressionOf(updateVar, arg)) {
+          // TODO: Get the source location of arg and point to the individual
+          // argument.
+          context_.Say(expr.source,
+              "Arguments to the atomic update operation cannot reference the updated variable, %s, as a subexpression"_err_en_US,
+              updateVar.AsFortran());
+        }
+        // TODO:
+        // if (captureVar && omp::IsSubexpressionOf(*captureVar, arg)) {
+        //  context_.Say(expr.source,
+        //      "The RHS of this atomic update statement cannot reference the
+        //      capture variable: %s"_err_en_US, captureVar->AsFortran());
+        //}
+      }
+      if (!foundUpdateVar) {
+        context_.Say(expr.source,
+            "The RHS of this atomic update statement must reference the updated variable: %s"_err_en_US,
+            updateVar.AsFortran());
+      }
     }
   }
 }
@@ -454,24 +469,21 @@ void AccStructureChecker::CheckAtomicWriteStmt(
   const auto &expr{std::get<parser::Expr>(assign.t)};
   const auto *rhs{GetExpr(context_, expr)};
   if (rhs) {
-    if (omp::IsSubexpressionOf(updateVar, *rhs)) {
+    if (evaluate::IsVarSubexpressionOf(updateVar, *rhs)) {
       context_.Say(expr.source,
-          "The RHS of this atomic write statement cannot reference the updated variable: %s"_err_en_US,
+          "The RHS of this atomic write statement cannot reference the atomic variable: %s"_err_en_US,
           updateVar.AsFortran());
     }
-    // TODO: Check w/ Valintin about semantic there are a lot of lowering tests
-    // that seem to violate the spec.
+    // TODO fix tests that this breaks.
     // if (captureVar && omp::IsSubexpressionOf(*captureVar, *rhs)) {
     //  context_.Say(expr.source,
-    //      "The RHS of this atomic write statement cannot reference the capture
-    //      variable: %s"_err_en_US, captureVar->AsFortran());
+    //      "The RHS of this atomic write statement cannot reference the
+    //      variable, %s, used to capture the atomic variable"_err_en_US,
+    //      captureVar->AsFortran());
     //}
   }
 }
 
-// Todo, with an appropriate extractor this would be pretty easy to check
-// and provide updateVar in a context sensitive way. I would probably need to
-// to switch the arguments to be evaluate::Exprs.
 void AccStructureChecker::CheckAtomicCaptureStmt(
     const parser::AssignmentStmt &assign, const SomeExpr *updateVar,
     const SomeExpr &captureVar) {
@@ -479,65 +491,6 @@ void AccStructureChecker::CheckAtomicCaptureStmt(
   CheckAtomicStmt(assign, construct);
 }
 
-// If the updatedVar is null, then we don't bother checking for updates to it.
-// Just that the op is a valid atomic update operator.
-bool AccStructureChecker::IsAtomicUpdateExpr(
-    const evaluate::Expr<evaluate::SomeType> &expr,
-    const evaluate::Expr<evaluate::SomeType> *updatedVar,
-    bool allowConvert) const {
-  const auto [op, args]{evaluate::GetTopLevelOperation(expr)};
-  if (!IsValidAtomicUpdateOperation(op)) {
-    if (IsConvertOperation(op) && args.size() == 1) {
-      return IsAtomicUpdateExpr(args[0], updatedVar, /*allowConvert=*/false);
-    }
-    return false;
-  }
-  if (!updatedVar) {
-    return true;
-  }
-  for (const auto &arg : args) {
-    if (*updatedVar == arg) {
-      return true;
-    }
-  }
-  return false;
-}
-
-// The allowNonUpdate argument is used to generate slightly more helpful error
-// messages when the rhs is not an update to the updatedVar.
-const parser::Variable *AccStructureChecker::GetUpdatedVarIfAtomicUpdateStmt(
-    const parser::AssignmentStmt &assign, bool allowNonUpdate) const {
-  // OpenACC 3.4, 2893-2898
-  const auto &updatedVar{std::get<parser::Variable>(assign.t)};
-  const auto &rhs{std::get<parser::Expr>(assign.t)};
-
-  // Is the rhs something a valid operations that references the updatedVar?
-  const auto *expr{GetExpr(context_, rhs)};
-  const auto *updatedExpr{GetExpr(context_, updatedVar)};
-  if (!expr || !updatedExpr) {
-    return nullptr;
-  }
-  if (IsAtomicUpdateExpr(*expr, allowNonUpdate ? nullptr : updatedExpr,
-          /*allowConvert=*/true)) {
-    return &updatedVar;
-  }
-  return nullptr;
-}
-
-bool AccStructureChecker::IsAtomicCaptureStmt(
-    const parser::AssignmentStmt &assign,
-    const parser::Variable &updateVar) const {
-  const auto &var{std::get<parser::Variable>(assign.t)};
-  const auto &expr{std::get<parser::Expr>(assign.t)};
-  const auto *varExpr{GetExpr(context_, var)};
-  const auto *updatedVarExpr{GetExpr(context_, updateVar)};
-  const auto *exprExpr{GetExpr(context_, expr)};
-  if (!varExpr || !updatedVarExpr || !exprExpr) {
-    return false;
-  }
-  return *updatedVarExpr == *exprExpr && !(*updatedVarExpr == *varExpr);
-}
-
 void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
   const Fortran::parser::AssignmentStmt &stmt1 =
       std::get<Fortran::parser::AccAtomicCapture::Stmt1>(capture.t).v.statement;
@@ -553,7 +506,7 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
   }
   if (*lhs1 == *lhs2) {
     context_.Say(std::get<parser::Verbatim>(capture.t).source,
-        "The variables assigned to in this atomic capture construct must be distinct"_err_en_US);
+        "The variables assigned in this atomic capture construct must be distinct"_err_en_US);
     return;
   }
   const auto &expr1{std::get<parser::Expr>(stmt1.t)};
@@ -567,19 +520,19 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
   bool stmt2CapturesLhs1{*lhs1 == GetExprModuloConversion(*rhs2)};
   if (stmt1CapturesLhs2 && !stmt2CapturesLhs1) {
     if (*lhs2 == GetExprModuloConversion(*rhs2)) {
-      // y = x; x = x; Doesn't fit the spec;
+      // a = b; b = b; Doesn't fit the spec;
       context_.Say(std::get<parser::Verbatim>(capture.t).source,
           "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
-      // TODO: Add attatchment that x = x is not considered an update.
-      // TODO: Add attatchment that y = x seems to be a capture.
-    } else if (omp::IsSubexpressionOf(*lhs2, *rhs2)) {
-      // Take y = x; x = <expr w/ x> as capture; update
+      // TODO: Add attatchment that a = b seems to be a capture,
+      // but b = b is not a valid update or write.
+    } else if (evaluate::IsVarSubexpressionOf(*lhs2, *rhs2)) {
+      // Take v = x; x = <expr w/ x> as capture; update
       const auto &updateVar{*lhs2};
       const auto &captureVar{*lhs1};
       CheckAtomicCaptureStmt(stmt1, &updateVar, captureVar);
       CheckAtomicUpdateStmt(stmt2, updateVar, &captureVar);
     } else {
-      // Take y = x; x = <expr w/o x> as capture; write
+      // Take v = x; x = <expr w/o x> as capture; write
       const auto &updateVar{*lhs2};
       const auto &captureVar{*lhs1};
       CheckAtomicCaptureStmt(stmt1, &updateVar, captureVar);
@@ -587,14 +540,13 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
     }
   } else if (stmt2CapturesLhs1 && !stmt1CapturesLhs2) {
     if (*lhs1 == GetExprModuloConversion(*rhs1)) {
-      // Error x = x; y = x;
+      // Error a = a; b = a;
       context_.Say(var1.GetSource(),
           "The first assignment in this atomic capture construct doesn't perform a valid update"_err_en_US);
-      // TODO: Add attatchment that x = x is not considered an update.
-      // TODO: Add attatchment that y = x seems to be a capture.
-      return;
+      // Add attatchment that a = a is not considered an update,
+      // but b = a seems to be a capture.
     } else {
-      // Take x = <expr>; y = x; as update; capture
+      // Take x = <expr>; v = x; as update; capture
       const auto &updateVar{*lhs1};
       const auto &captureVar{*lhs2};
       CheckAtomicUpdateStmt(stmt1, updateVar, &captureVar);
@@ -605,8 +557,8 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
     context_.Say(std::get<parser::Verbatim>(capture.t).source,
         "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
     // TODO: Add attatchment that both assignments seem to be captures.
-  } else {
-    // y1 = <expr != y2>; y2 = <expr != y1>; Doesn't fit the spec
+  } else { // !stmt1CapturesLhs2 && !stmt2CapturesLhs1
+    // a = <expr != b>; b = <expr != a>; Doesn't fit the spec
     context_.Say(std::get<parser::Verbatim>(capture.t).source,
         "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
     // TODO: Add attatchment that neither assignment seems to be a capture.
diff --git a/flang/lib/Semantics/check-acc-structure.h b/flang/lib/Semantics/check-acc-structure.h
index 8f74369e61b5e..7e00dfca2d746 100644
--- a/flang/lib/Semantics/check-acc-structure.h
+++ b/flang/lib/Semantics/check-acc-structure.h
@@ -83,17 +83,6 @@ class AccStructureChecker
 #include "llvm/Frontend/OpenACC/ACC.inc"
 
 private:
-  bool IsAtomicUpdateOperator(
-      const parser::Expr &expr, const parser::Variable &updatedVar) const;
-  bool IsAtomicUpdateIntrinsic(
-      const parser::Expr &expr, const parser::Variable &updatedVar) const;
-  bool IsAtomicUpdateExpr(const evaluate::Expr<evaluate::SomeType> &expr,
-      const evaluate::Expr<evaluate::SomeType> *updatedVar,
-      bool allowConvert) const;
-  const parser::Variable *GetUpdatedVarIfAtomicUpdateStmt(
-      const parser::AssignmentStmt &assign, bool allowNonUpdate) const;
-  bool IsAtomicCaptureStmt(const parser::AssignmentStmt &assign,
-      const parser::Variable &updatedVar) const;
   void CheckAtomicStmt(
       const parser::AssignmentStmt &assign, std::string &construct);
   void CheckAtomicUpdateStmt(const parser::AssignmentStmt &assign,
@@ -106,8 +95,6 @@ class AccStructureChecker
       const parser::Variable &updateVar, const parser::Variable &captureVar);
   void CheckAtomicCaptureVariable(
       const parser::Variable &captureVar, const parser::Variable &updateVar);
-  bool DoesExprReferenceVar(const evaluate::Expr<evaluate::SomeType> &expr,
-      const evaluate::Expr<evaluate::SomeType> &var) const;
 
   bool CheckAllowedModifier(llvm::acc::Clause clause);
   bool IsComputeConstruct(llvm::acc::Directive directive) const;
diff --git a/flang/lib/Semantics/check-omp-atomic.cpp b/flang/lib/Semantics/check-omp-atomic.cpp
index c5ed8796f0c34..b20de7adf9385 100644
--- a/flang/lib/Semantics/check-omp-atomic.cpp
+++ b/flang/lib/Semantics/check-omp-atomic.cpp
@@ -399,8 +399,8 @@ OmpStructureChecker::CheckUpdateCapture(
   //    subexpression of the right-hand side.
   // 2. An assignment could be a capture (cbc) if the right-hand side is
   //    a variable (or a function ref), with potential type conversions.
-  bool cbu1{IsSubexpressionOf(as1.lhs, as1.rhs)}; // Can as1 be an update?
-  bool cbu2{IsSubexpressionOf(as2.lhs, as2.rhs)}; // Can as2 be an update?
+  bool cbu1{IsVarSubexpressionOf(as1.lhs, as1.rhs)}; // Can as1 be an update?
+  bool cbu2{IsVarSubexpressionOf(as2.lhs, as2.rhs)}; // Can as2 be an update?
   bool cbc1{IsVarOrFunctionRef(GetConvertInput(as1.rhs))}; // Can 1 be capture?
   bool cbc2{IsVarOrFunctionRef(GetConvertInput(as2.rhs))}; // Can 2 be capture?
 
@@ -657,7 +657,7 @@ void OmpStructureChecker::CheckAtomicUpdateAssignment(
       if (IsSameOrConvertOf(arg, atom)) {
         ++count;
       } else {
-        if (!subExpr && IsSubexpressionOf(atom, arg)) {
+        if (!subExpr && evaluate::IsVarSubexpressionOf(atom, arg)) {
           subExpr = arg;
         }
         nonAtom.push_back(arg);
diff --git a/flang/lib/Semantics/openmp-utils.cpp b/flang/lib/Semantics/openmp-utils.cpp
index da14507aa9fe6..7a492a4378907 100644
--- a/flang/lib/Semantics/openmp-utils.cpp
+++ b/flang/lib/Semantics/openmp-utils.cpp
@@ -270,28 +270,6 @@ struct DesignatorCollector : public evaluate::Traverse<DesignatorCollector,
   }
 };
 
-struct VariableFinder : public evaluate::AnyTraverse<VariableFinder> {
-  using Base = evaluate::AnyTraverse<VariableFinder>;
-  VariableFinder(const SomeExpr &v) : Base(*this), var(v) {}
-
-  using Base::operator();
-
-  template <typename T>
-  bool operator()(const evaluate::Designator<T> &x) const {
-    auto copy{x};
-    return evaluate::AsGenericExpr(std::move(copy)) == var;
-  }
-
-  template <typename T>
-  bool operator()(const evaluate::FunctionRef<T> &x) const {
-    auto copy{x};
-    return evaluate::AsGenericExpr(std::move(copy)) == var;
-  }
-
-private:
-  const SomeExpr &var;
-};
-
 std::vector<SomeExpr> GetAllDesignators(const SomeExpr &expr) {
   return DesignatorCollector{}(expr);
 }
@@ -380,10 +358,6 @@ const SomeExpr *HasStorageOverlap(
   return nullptr;
 }
 
-bool IsSubexpressionOf(const SomeExpr &sub, const SomeExpr &super) {
-  return VariableFinder{sub}(super);
-}
-
 // Check if the ActionStmt is actually a [Pointer]AssignmentStmt. This is
 // to separate cases where the source has something that looks like an
 // assignment, but is semantically wrong (diagnosed by general semantic
diff --git a/flang/lib/Semantics/openmp-utils.h b/flang/lib/Semantics/openmp-utils.h
index 001fbeb45ceec..b8ad9ed17c720 100644
--- a/flang/lib/Semantics/openmp-utils.h
+++ b/flang/lib/Semantics/openmp-utils.h
@@ -72,7 +72,6 @@ std::optional<bool> IsContiguous(
 std::vector<SomeExpr> GetAllDesignators(const SomeExpr &expr);
 const SomeExpr *HasStorageOverlap(
     const SomeExpr &base, llvm::ArrayRef<SomeExpr> exprs);
-bool IsSubexpressionOf(const SomeExpr &sub, const SomeExpr &super);
 bool IsAssignment(const parser::ActionStmt *x);
 bool IsPointerAssignment(const evaluate::Assignment &x);
 const parser::Block &GetInnermostExecPart(const parser::Block &block);
diff --git a/flang/test/Semantics/OpenACC/acc-atomic-validity.f90 b/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
index 276042ae39980..5c8d33fbbadc7 100644
--- a/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
+++ b/flang/test/Semantics/OpenACC/acc-atomic-validity.f90
@@ -60,6 +60,32 @@ program openacc_atomic_validity
   i = i + 1
   !$acc end atomic
 
+  !ERROR: The variables assigned in this atomic capture construct must be distinct
+  !$acc atomic capture
+  c(1) = c(2)
+  c(1) = c(3)
+  !$acc end atomic
+  
+  !ERROR: The assignments in this atomic capture construct do not update a variable and capture either its initial or final value
+  !$acc atomic capture
+  c(1) = c(2)
+  c(2) = c(2)
+  !$acc end atomic
+
+  !ERROR: The assignments in this atomic capture construct do not update a variable and capture either its initial or final value
+  !$acc atomic capture
+  c(1) = c(2)
+  c(2) = c(1)
+  !$acc end atomic
+
+  !ERROR: The assignments in this atomic capture construct do not update a variable and capture either its initial or final value
+  !$acc atomic capture
+  c(1) = c(2)
+  c(3) = c(2)
+  !$acc end atomic
+
+
+
   !$acc atomic capture if(l .EQV. .false.)
   c(i) = i
   i = i + 1
@@ -106,14 +132,14 @@ subroutine capture_with_convert_f64_to_i32()
   !$acc end atomic
 
   !$acc atomic capture
-  !TODO: the rhs side of this update statement should reference x.
+  !ERROR: The RHS of this atomic update statement must reference the updated variable: x
   x = v * v
   v = x
   !$acc end atomic
 
   !$acc atomic capture
   x = v
-  !TODO: the rhs hand side of this write expression is not allowed to read v.
+  !ERROR: The updated variable, v, cannot appear more than once in the atomic update operation
   v = v * v
   !$acc end atomic
 

From c7f0750dfa3007e7b7e9f5fa455b3179c7f2b35b Mon Sep 17 00:00:00 2001
From: Andre Kuhlenschmidt <akuhlenschmi@nvidia.com>
Date: Mon, 28 Jul 2025 14:18:47 -0700
Subject: [PATCH 5/8] remove some untested checks

---
 flang/lib/Semantics/check-acc-structure.cpp | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/flang/lib/Semantics/check-acc-structure.cpp b/flang/lib/Semantics/check-acc-structure.cpp
index 3a32ced5f7980..f18c974ebbe2d 100644
--- a/flang/lib/Semantics/check-acc-structure.cpp
+++ b/flang/lib/Semantics/check-acc-structure.cpp
@@ -365,14 +365,14 @@ void AccStructureChecker::CheckAtomicStmt(
       context_.Say(expr.source,
           "LHS of atomic %s statement must be scalar"_err_en_US, construct);
     }
-    // TODO: Check if lhs is intrinsic type
+    // TODO: Check if lhs is intrinsic type.
   }
   if (rhs) {
     if (rhs->Rank() != 0) {
       context_.Say(var.GetSource(),
           "RHS of atomic %s statement must be scalar"_err_en_US, construct);
     }
-    // TODO: Check if rhs is intrinsic type
+    // TODO: Check if rhs is intrinsic type.
   }
 }
 
@@ -445,12 +445,6 @@ void AccStructureChecker::CheckAtomicUpdateStmt(
               "Arguments to the atomic update operation cannot reference the updated variable, %s, as a subexpression"_err_en_US,
               updateVar.AsFortran());
         }
-        // TODO:
-        // if (captureVar && omp::IsSubexpressionOf(*captureVar, arg)) {
-        //  context_.Say(expr.source,
-        //      "The RHS of this atomic update statement cannot reference the
-        //      capture variable: %s"_err_en_US, captureVar->AsFortran());
-        //}
       }
       if (!foundUpdateVar) {
         context_.Say(expr.source,
@@ -474,13 +468,6 @@ void AccStructureChecker::CheckAtomicWriteStmt(
           "The RHS of this atomic write statement cannot reference the atomic variable: %s"_err_en_US,
           updateVar.AsFortran());
     }
-    // TODO fix tests that this breaks.
-    // if (captureVar && omp::IsSubexpressionOf(*captureVar, *rhs)) {
-    //  context_.Say(expr.source,
-    //      "The RHS of this atomic write statement cannot reference the
-    //      variable, %s, used to capture the atomic variable"_err_en_US,
-    //      captureVar->AsFortran());
-    //}
   }
 }
 

From 05f38dfd4be3dddf3aacd95c2a8c9b0c5c3fcf15 Mon Sep 17 00:00:00 2001
From: Andre Kuhlenschmidt <akuhlenschmi@nvidia.com>
Date: Tue, 29 Jul 2025 10:39:48 -0700
Subject: [PATCH 6/8] address feedback

---
 flang/include/flang/Evaluate/tools.h          |  8 ++
 flang/lib/Evaluate/tools.cpp                  | 13 ++-
 flang/lib/Lower/OpenMP/Atomic.cpp             |  5 +-
 flang/lib/Semantics/check-acc-structure.cpp   | 95 ++++++++-----------
 flang/lib/Semantics/check-acc-structure.h     |  2 +-
 flang/lib/Semantics/check-omp-atomic.cpp      |  7 +-
 .../test/Lower/OpenACC/acc-atomic-update.f90  | 89 +++++++++++++++++
 7 files changed, 150 insertions(+), 69 deletions(-)
 create mode 100644 flang/test/Lower/OpenACC/acc-atomic-update.f90

diff --git a/flang/include/flang/Evaluate/tools.h b/flang/include/flang/Evaluate/tools.h
index 43ad8707fa2b8..cef57f1851bcc 100644
--- a/flang/include/flang/Evaluate/tools.h
+++ b/flang/include/flang/Evaluate/tools.h
@@ -10,6 +10,7 @@
 #define FORTRAN_EVALUATE_TOOLS_H_
 
 #include "traverse.h"
+#include "flang/Common/enum-set.h"
 #include "flang/Common/idioms.h"
 #include "flang/Common/template.h"
 #include "flang/Common/unwrap.h"
@@ -1397,6 +1398,8 @@ enum class Operator {
   True,
 };
 
+using OperatorSet = common::EnumSet<Operator, 32>;
+
 std::string ToString(Operator op);
 
 template <typename... Ts, int Kind>
@@ -1509,6 +1512,11 @@ Operator OperationCode(const evaluate::ProcedureDesignator &proc);
 std::pair<operation::Operator, std::vector<Expr<SomeType>>>
 GetTopLevelOperation(const Expr<SomeType> &expr);
 
+// Return information about the top-level operation (ignoring parentheses, and
+// resizing converts)
+std::pair<operation::Operator, std::vector<Expr<SomeType>>>
+GetTopLevelOperationIgnoreResizing(const Expr<SomeType> &expr);
+
 // Check if expr is same as x, or a sequence of Convert operations on x.
 bool IsSameOrConvertOf(const Expr<SomeType> &expr, const Expr<SomeType> &x);
 
diff --git a/flang/lib/Evaluate/tools.cpp b/flang/lib/Evaluate/tools.cpp
index c34bf8b8ebfca..171dd91fa9fd1 100644
--- a/flang/lib/Evaluate/tools.cpp
+++ b/flang/lib/Evaluate/tools.cpp
@@ -1809,10 +1809,15 @@ operation::Operator operation::OperationCode(const ProcedureDesignator &proc) {
 }
 
 std::pair<operation::Operator, std::vector<Expr<SomeType>>>
-GetTopLevelOperation(const Expr<SomeType> &expr) {
+GetTopLevelOperationIgnoreResizing(const Expr<SomeType> &expr) {
   return operation::ArgumentExtractor<true>{}(expr);
 }
 
+std::pair<operation::Operator, std::vector<Expr<SomeType>>>
+GetTopLevelOperation(const Expr<SomeType> &expr) {
+  return operation::ArgumentExtractor<false>{}(expr);
+}
+
 namespace operation {
 struct ConvertCollector
     : public Traverse<ConvertCollector,
@@ -1946,14 +1951,12 @@ struct VariableFinder : public evaluate::AnyTraverse<VariableFinder> {
 
   template <typename T>
   bool operator()(const evaluate::Designator<T> &x) const {
-    auto copy{x};
-    return evaluate::AsGenericExpr(std::move(copy)) == var;
+    return evaluate::AsGenericExpr(common::Clone(x)) == var;
   }
 
   template <typename T>
   bool operator()(const evaluate::FunctionRef<T> &x) const {
-    auto copy{x};
-    return evaluate::AsGenericExpr(std::move(copy)) == var;
+    return evaluate::AsGenericExpr(common::Clone(x)) == var;
   }
 
 private:
diff --git a/flang/lib/Lower/OpenMP/Atomic.cpp b/flang/lib/Lower/OpenMP/Atomic.cpp
index d4f83f57bd5d4..c9a6dbabd4182 100644
--- a/flang/lib/Lower/OpenMP/Atomic.cpp
+++ b/flang/lib/Lower/OpenMP/Atomic.cpp
@@ -607,7 +607,7 @@ genAtomicUpdate(lower::AbstractConverter &converter,
   // This must exist by now.
   semantics::SomeExpr rhs = assign.rhs;
   semantics::SomeExpr input = *evaluate::GetConvertInput(rhs);
-  auto [opcode, args] = evaluate::GetTopLevelOperation(input);
+  auto [opcode, args] = evaluate::GetTopLevelOperationIgnoreResizing(input);
   assert(!args.empty() && "Update operation without arguments");
 
   // Pass args as an argument to avoid capturing a structured binding.
@@ -625,7 +625,8 @@ genAtomicUpdate(lower::AbstractConverter &converter,
     // operations with exactly two (non-optional) arguments.
     rhs = genReducedMinMax(rhs, atomArg, args);
     input = *evaluate::GetConvertInput(rhs);
-    std::tie(opcode, args) = evaluate::GetTopLevelOperation(input);
+    std::tie(opcode, args) =
+        evaluate::GetTopLevelOperationIgnoreResizing(input);
     atomArg = nullptr; // No longer valid.
   }
   for (auto &arg : args) {
diff --git a/flang/lib/Semantics/check-acc-structure.cpp b/flang/lib/Semantics/check-acc-structure.cpp
index f18c974ebbe2d..77e2b01578641 100644
--- a/flang/lib/Semantics/check-acc-structure.cpp
+++ b/flang/lib/Semantics/check-acc-structure.cpp
@@ -14,12 +14,8 @@
 #include "flang/Semantics/type.h"
 #include "flang/Support/Fortran.h"
 #include "llvm/Support/AtomicOrdering.h"
-#include "llvm/Support/raw_ostream.h"
-#include <optional>
 
-// TODO: Move the parts that I am reusing from openmp-utils.h to
-// evaluate/tools.h
-#include "openmp-utils.h"
+#include <optional>
 
 #define CHECK_SIMPLE_CLAUSE(X, Y) \
   void AccStructureChecker::Enter(const parser::AccClause::X &) { \
@@ -354,7 +350,7 @@ void AccStructureChecker::Leave(const parser::OpenACCAtomicConstruct &x) {
 }
 
 void AccStructureChecker::CheckAtomicStmt(
-    const parser::AssignmentStmt &assign, std::string &construct) {
+    const parser::AssignmentStmt &assign, const std::string &construct) {
   const auto &var{std::get<parser::Variable>(assign.t)};
   const auto &expr{std::get<parser::Expr>(assign.t)};
   const auto *rhs{GetExpr(context_, expr)};
@@ -376,38 +372,30 @@ void AccStructureChecker::CheckAtomicStmt(
   }
 }
 
+static constexpr evaluate::operation::OperatorSet validAccAtomicUpdateOperators{
+    evaluate::operation::Operator::Add, evaluate::operation::Operator::Mul,
+    evaluate::operation::Operator::Sub, evaluate::operation::Operator::Div,
+    evaluate::operation::Operator::And, evaluate::operation::Operator::Or,
+    evaluate::operation::Operator::Eqv, evaluate::operation::Operator::Neqv,
+    evaluate::operation::Operator::Max, evaluate::operation::Operator::Min};
+
 static bool IsValidAtomicUpdateOperation(
     const evaluate::operation::Operator &op) {
-  switch (op) {
-  case evaluate::operation::Operator::Add:
-  case evaluate::operation::Operator::Mul:
-  case evaluate::operation::Operator::Sub:
-  case evaluate::operation::Operator::Div:
-  case evaluate::operation::Operator::And:
-  case evaluate::operation::Operator::Or:
-  case evaluate::operation::Operator::Eqv:
-  case evaluate::operation::Operator::Neqv:
-  // 2909 intrinsic-procedure name is one of max, min, iand, ior, or ieor.
-  case evaluate::operation::Operator::Max:
-  case evaluate::operation::Operator::Min:
-    // Currently all get mapped to And, Or, and Neqv
-    // case evaluate::operation::Operator::Iand:
-    // case evaluate::operation::Operator::Ior:
-    // case evaluate::operation::Operator::Ieor:
-    return true;
-  case evaluate::operation::Operator::Convert:
-  case evaluate::operation::Operator::Identity:
-  default:
-    return false;
-  }
+  return validAccAtomicUpdateOperators.test(op);
 }
 
+// Couldn't reproduce this behavior with evaluate::UnwrapConvertedExpr which
+// is similar but only works within a single type category.
 static SomeExpr GetExprModuloConversion(const SomeExpr &expr) {
   const auto [op, args]{evaluate::GetTopLevelOperation(expr)};
   // Check: if it is a conversion then it must have at least one argument.
-  CHECK((op != evaluate::operation::Operator::Convert || args.size() >= 1) &&
+  CHECK(((op != evaluate::operation::Operator::Convert &&
+             op != evaluate::operation::Operator::Resize) ||
+            args.size() >= 1) &&
       "Invalid conversion operation");
-  if (op == evaluate::operation::Operator::Convert && args.size() == 1) {
+  if ((op == evaluate::operation::Operator::Convert ||
+          op == evaluate::operation::Operator::Resize) &&
+      args.size() >= 1) {
     return args[0];
   }
   return expr;
@@ -416,8 +404,7 @@ static SomeExpr GetExprModuloConversion(const SomeExpr &expr) {
 void AccStructureChecker::CheckAtomicUpdateStmt(
     const parser::AssignmentStmt &assign, const SomeExpr &updateVar,
     const SomeExpr *captureVar) {
-  static std::string construct{"update"};
-  CheckAtomicStmt(assign, construct);
+  CheckAtomicStmt(assign, "update");
   const auto &expr{std::get<parser::Expr>(assign.t)};
   const auto *rhs{GetExpr(context_, expr)};
   if (rhs) {
@@ -425,8 +412,7 @@ void AccStructureChecker::CheckAtomicUpdateStmt(
         evaluate::GetTopLevelOperation(GetExprModuloConversion(*rhs))};
     if (!IsValidAtomicUpdateOperation(op)) {
       context_.Say(expr.source,
-          "Invalid atomic update operation, can only use: *, +, -, *, /, and, or, eqv, neqv, max, min, iand, ior, ieor"_err_en_US,
-          construct);
+          "Invalid atomic update operation, can only use: *, +, -, *, /, and, or, eqv, neqv, max, min, iand, ior, ieor"_err_en_US);
     } else {
       bool foundUpdateVar{false};
       for (const auto &arg : args) {
@@ -458,8 +444,7 @@ void AccStructureChecker::CheckAtomicUpdateStmt(
 void AccStructureChecker::CheckAtomicWriteStmt(
     const parser::AssignmentStmt &assign, const SomeExpr &updateVar,
     const SomeExpr *captureVar) {
-  static std::string construct{"write"};
-  CheckAtomicStmt(assign, construct);
+  CheckAtomicStmt(assign, "write");
   const auto &expr{std::get<parser::Expr>(assign.t)};
   const auto *rhs{GetExpr(context_, expr)};
   if (rhs) {
@@ -474,15 +459,16 @@ void AccStructureChecker::CheckAtomicWriteStmt(
 void AccStructureChecker::CheckAtomicCaptureStmt(
     const parser::AssignmentStmt &assign, const SomeExpr *updateVar,
     const SomeExpr &captureVar) {
-  static std::string construct{"capture"};
-  CheckAtomicStmt(assign, construct);
+  CheckAtomicStmt(assign, "capture");
 }
 
 void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
-  const Fortran::parser::AssignmentStmt &stmt1 =
-      std::get<Fortran::parser::AccAtomicCapture::Stmt1>(capture.t).v.statement;
-  const Fortran::parser::AssignmentStmt &stmt2 =
-      std::get<Fortran::parser::AccAtomicCapture::Stmt2>(capture.t).v.statement;
+  const Fortran::parser::AssignmentStmt &stmt1{
+      std::get<Fortran::parser::AccAtomicCapture::Stmt1>(capture.t)
+          .v.statement};
+  const Fortran::parser::AssignmentStmt &stmt2{
+      std::get<Fortran::parser::AccAtomicCapture::Stmt2>(capture.t)
+          .v.statement};
   const auto &var1{std::get<parser::Variable>(stmt1.t)};
   const auto &var2{std::get<parser::Variable>(stmt2.t)};
   const auto *lhs1{GetExpr(context_, var1)};
@@ -507,7 +493,7 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
   bool stmt2CapturesLhs1{*lhs1 == GetExprModuloConversion(*rhs2)};
   if (stmt1CapturesLhs2 && !stmt2CapturesLhs1) {
     if (*lhs2 == GetExprModuloConversion(*rhs2)) {
-      // a = b; b = b; Doesn't fit the spec;
+      // a = b; b = b: Doesn't fit the spec.
       context_.Say(std::get<parser::Verbatim>(capture.t).source,
           "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
       // TODO: Add attatchment that a = b seems to be a capture,
@@ -533,14 +519,14 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
       // Add attatchment that a = a is not considered an update,
       // but b = a seems to be a capture.
     } else {
-      // Take x = <expr>; v = x; as update; capture
+      // Take x = <expr>; v = x: as update; capture
       const auto &updateVar{*lhs1};
       const auto &captureVar{*lhs2};
       CheckAtomicUpdateStmt(stmt1, updateVar, &captureVar);
       CheckAtomicCaptureStmt(stmt2, &updateVar, captureVar);
     }
   } else if (stmt1CapturesLhs2 && stmt2CapturesLhs1) {
-    // x1 = x2; x2 = x1; Doesn't fit the spec;
+    // x1 = x2; x2 = x1; Doesn't fit the spec.
     context_.Say(std::get<parser::Verbatim>(capture.t).source,
         "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
     // TODO: Add attatchment that both assignments seem to be captures.
@@ -550,40 +536,33 @@ void AccStructureChecker::Enter(const parser::AccAtomicCapture &capture) {
         "The assignments in this atomic capture construct do not update a variable and capture either its initial or final value"_err_en_US);
     // TODO: Add attatchment that neither assignment seems to be a capture.
   }
-  return;
 }
 
 void AccStructureChecker::Enter(const parser::AccAtomicUpdate &x) {
   const auto &assign{
       std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement};
   const auto &var{std::get<parser::Variable>(assign.t)};
-  const auto *updateVar{GetExpr(context_, var)};
-  if (!updateVar) {
-    return;
+  if (const auto *updateVar{GetExpr(context_, var)}) {
+    CheckAtomicUpdateStmt(assign, *updateVar, /*captureVar=*/nullptr);
   }
-  CheckAtomicUpdateStmt(assign, *updateVar, nullptr);
 }
 
 void AccStructureChecker::Enter(const parser::AccAtomicWrite &x) {
   const auto &assign{
       std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement};
   const auto &var{std::get<parser::Variable>(assign.t)};
-  const auto *updateVar{GetExpr(context_, var)};
-  if (!updateVar) {
-    return;
+  if (const auto *updateVar{GetExpr(context_, var)}) {
+    CheckAtomicWriteStmt(assign, *updateVar, /*captureVar=*/nullptr);
   }
-  CheckAtomicWriteStmt(assign, *updateVar, nullptr);
 }
 
 void AccStructureChecker::Enter(const parser::AccAtomicRead &x) {
   const auto &assign{
       std::get<parser::Statement<parser::AssignmentStmt>>(x.t).statement};
   const auto &var{std::get<parser::Variable>(assign.t)};
-  const auto *captureVar{GetExpr(context_, var)};
-  if (!captureVar) {
-    return;
+  if (const auto *captureVar{GetExpr(context_, var)}) {
+    CheckAtomicCaptureStmt(assign, /*updateVar=*/nullptr, *captureVar);
   }
-  CheckAtomicCaptureStmt(assign, nullptr, *captureVar);
 }
 
 void AccStructureChecker::Enter(const parser::OpenACCCacheConstruct &x) {
diff --git a/flang/lib/Semantics/check-acc-structure.h b/flang/lib/Semantics/check-acc-structure.h
index 7e00dfca2d746..359f1557b62cb 100644
--- a/flang/lib/Semantics/check-acc-structure.h
+++ b/flang/lib/Semantics/check-acc-structure.h
@@ -84,7 +84,7 @@ class AccStructureChecker
 
 private:
   void CheckAtomicStmt(
-      const parser::AssignmentStmt &assign, std::string &construct);
+      const parser::AssignmentStmt &assign, const std::string &construct);
   void CheckAtomicUpdateStmt(const parser::AssignmentStmt &assign,
       const SomeExpr &updateVar, const SomeExpr *captureVar);
   void CheckAtomicCaptureStmt(const parser::AssignmentStmt &assign,
diff --git a/flang/lib/Semantics/check-omp-atomic.cpp b/flang/lib/Semantics/check-omp-atomic.cpp
index b20de7adf9385..333fad0ca8678 100644
--- a/flang/lib/Semantics/check-omp-atomic.cpp
+++ b/flang/lib/Semantics/check-omp-atomic.cpp
@@ -197,7 +197,8 @@ static std::pair<parser::CharBlock, parser::CharBlock> SplitAssignmentSource(
 }
 
 static bool IsCheckForAssociated(const SomeExpr &cond) {
-  return GetTopLevelOperation(cond).first == operation::Operator::Associated;
+  return GetTopLevelOperationIgnoreResizing(cond).first ==
+      operation::Operator::Associated;
 }
 
 static bool IsMaybeAtomicWrite(const evaluate::Assignment &assign) {
@@ -607,7 +608,7 @@ void OmpStructureChecker::CheckAtomicUpdateAssignment(
   std::pair<operation::Operator, std::vector<SomeExpr>> top{
       operation::Operator::Unknown, {}};
   if (auto &&maybeInput{GetConvertInput(update.rhs)}) {
-    top = GetTopLevelOperation(*maybeInput);
+    top = GetTopLevelOperationIgnoreResizing(*maybeInput);
   }
   switch (top.first) {
   case operation::Operator::Add:
@@ -715,7 +716,7 @@ void OmpStructureChecker::CheckAtomicConditionalUpdateAssignment(
 
   CheckAtomicVariable(atom, alsrc);
 
-  auto top{GetTopLevelOperation(cond)};
+  auto top{GetTopLevelOperationIgnoreResizing(cond)};
   // Missing arguments to operations would have been diagnosed by now.
 
   switch (top.first) {
diff --git a/flang/test/Lower/OpenACC/acc-atomic-update.f90 b/flang/test/Lower/OpenACC/acc-atomic-update.f90
new file mode 100644
index 0000000000000..71aa69fd64eba
--- /dev/null
+++ b/flang/test/Lower/OpenACC/acc-atomic-update.f90
@@ -0,0 +1,89 @@
+! This test checks lowering of atomic and atomic update constructs
+! RUN: bbc -fopenacc -emit-hlfir %s -o - | FileCheck %s
+! RUN: %flang_fc1 -fopenacc -emit-hlfir %s -o - | FileCheck %s
+
+program acc_atomic_update_test
+    interface
+       integer function func(x)
+         integer :: x
+       end function func
+    end interface
+    integer :: x, y, z
+    integer, pointer :: a, b
+    integer, target :: c, d
+    integer(1) :: i1
+
+    a=>c
+    b=>d
+
+!CHECK: %[[A:.*]] = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "a", uniq_name = "_QFEa"}
+!CHECK: %[[A_DECL:.*]]:2 = hlfir.declare %[[A]] {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFEa"} : (!fir.ref<!fir.box<!fir.ptr<i32>>>) -> (!fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.ref<!fir.box<!fir.ptr<i32>>>)
+!CHECK: %[[B:.*]] = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "b", uniq_name = "_QFEb"}
+!CHECK: %[[B_DECL:.*]]:2 = hlfir.declare %[[B]] {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFEb"} : (!fir.ref<!fir.box<!fir.ptr<i32>>>) -> (!fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.ref<!fir.box<!fir.ptr<i32>>>)
+!CHECK: %[[C_ADDR:.*]] = fir.address_of(@_QFEc) : !fir.ref<i32>
+!CHECK: %[[D_ADDR:.*]] = fir.address_of(@_QFEd) : !fir.ref<i32>
+!CHECK: %[[I1:.*]] = fir.alloca i8 {bindc_name = "i1", uniq_name = "_QFEi1"}
+!CHECK: %[[I1_DECL:.*]]:2 = hlfir.declare %[[I1]] {uniq_name = "_QFEi1"} : (!fir.ref<i8>) -> (!fir.ref<i8>, !fir.ref<i8>)
+!CHECK: %[[X:.*]] = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFEx"}
+!CHECK: %[[X_DECL:.*]]:2 = hlfir.declare %[[X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[Y:.*]] = fir.alloca i32 {bindc_name = "y", uniq_name = "_QFEy"}
+!CHECK: %[[Y_DECL:.*]]:2 = hlfir.declare %[[Y]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[Z:.*]] = fir.alloca i32 {bindc_name = "z", uniq_name = "_QFEz"}
+!CHECK: %[[Z_DECL:.*]]:2 = hlfir.declare %[[Z]] {uniq_name = "_QFEz"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK: %[[LOAD_A:.*]] = fir.load %[[A_DECL]]#0 : !fir.ref<!fir.box<!fir.ptr<i32>>>
+!CHECK: %[[BOX_ADDR_A:.*]] = fir.box_addr %[[LOAD_A]] : (!fir.box<!fir.ptr<i32>>) -> !fir.ptr<i32>
+!CHECK: %[[LOAD_B:.*]] = fir.load %[[B_DECL]]#0 : !fir.ref<!fir.box<!fir.ptr<i32>>>
+!CHECK: %[[BOX_ADDR_B:.*]] = fir.box_addr %[[LOAD_B]] : (!fir.box<!fir.ptr<i32>>) -> !fir.ptr<i32>
+!CHECK: %[[LOAD_BOX_ADDR_B:.*]] = fir.load %[[BOX_ADDR_B]] : !fir.ptr<i32>
+!CHECK: acc.atomic.update %[[BOX_ADDR_A]] : !fir.ptr<i32> {
+!CHECK: ^bb0(%[[ARG0:.*]]: i32):
+!CHECK:   %[[ADD:.*]] = arith.addi %[[ARG0]], %[[LOAD_BOX_ADDR_B]] : i32
+!CHECK:   acc.yield %[[ADD]] : i32
+!CHECK: }
+
+    !$acc atomic update
+        a = a + b 
+
+!CHECK: {{.*}} = arith.constant 1 : i32
+!CHECK: acc.atomic.update %[[Y_DECL]]#0 : !fir.ref<i32> {
+!CHECK:  ^bb0(%[[ARG:.*]]: i32):
+!CHECK:    %[[RESULT:.*]] = arith.addi %[[ARG]], {{.*}} : i32
+!CHECK:    acc.yield %[[RESULT]] : i32
+!CHECK:  }
+!CHECK:  %[[LOADED_X:.*]] = fir.load %[[X_DECL]]#0 : !fir.ref<i32>
+!CHECK:  acc.atomic.update %[[Z_DECL]]#0 : !fir.ref<i32> {
+!CHECK:  ^bb0(%[[ARG:.*]]: i32):
+!CHECK:    %[[RESULT:.*]] = arith.muli %[[LOADED_X]], %[[ARG]] : i32
+!CHECK:    acc.yield %[[RESULT]] : i32
+!CHECK:  }
+    !$acc atomic 
+        y = y + 1
+    !$acc atomic update
+        z = x * z 
+
+!CHECK:  %[[C1_VAL:.*]] = arith.constant 1 : i32
+!CHECK:  acc.atomic.update %[[I1_DECL]]#0 : !fir.ref<i8> {
+!CHECK:  ^bb0(%[[VAL:.*]]: i8):
+!CHECK:    %[[CVT_VAL:.*]] = fir.convert %[[VAL]] : (i8) -> i32
+!CHECK:    %[[ADD_VAL:.*]] = arith.addi %[[CVT_VAL]], %[[C1_VAL]] : i32
+!CHECK:    %[[UPDATED_VAL:.*]] = fir.convert %[[ADD_VAL]] : (i32) -> i8
+!CHECK:    acc.yield %[[UPDATED_VAL]] : i8
+!CHECK:  }
+    !$acc atomic
+      i1 = i1 + 1
+    !$acc end atomic
+
+!CHECK:  %[[VAL_44:.*]]:3 = hlfir.associate %{{.*}} {adapt.valuebyref} : (i32) -> (!fir.ref<i32>, !fir.ref<i32>, i1)
+!CHECK:  %[[VAL_45:.*]] = fir.call @_QPfunc(%[[VAL_44]]#0) fastmath<contract> : (!fir.ref<i32>) -> i32
+!CHECK:  acc.atomic.update %[[X_DECL]]#0 : !fir.ref<i32> {
+!CHECK:  ^bb0(%[[VAL_46:.*]]: i32):
+!CHECK:    %[[VAL_47:.*]] = arith.addi %[[VAL_46]], %[[VAL_45]] : i32
+!CHECK:    acc.yield %[[VAL_47]] : i32
+!CHECK:  }
+!CHECK:  hlfir.end_associate %[[VAL_44]]#1, %[[VAL_44]]#2 : !fir.ref<i32>, i1
+    !$acc atomic update
+    x = x + func(z + 1)
+    !$acc end atomic
+!CHECK:  return
+!CHECK: }
+end program acc_atomic_update_test

From 79107060739a6805b6d7d4e94b3207bd1dc751de Mon Sep 17 00:00:00 2001
From: Andre Kuhlenschmidt <akuhlenschmi@nvidia.com>
Date: Tue, 29 Jul 2025 10:59:42 -0700
Subject: [PATCH 7/8] remove std::tie

---
 flang/lib/Lower/OpenMP/Atomic.cpp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/flang/lib/Lower/OpenMP/Atomic.cpp b/flang/lib/Lower/OpenMP/Atomic.cpp
index c9a6dbabd4182..fdb90a32fffae 100644
--- a/flang/lib/Lower/OpenMP/Atomic.cpp
+++ b/flang/lib/Lower/OpenMP/Atomic.cpp
@@ -625,8 +625,7 @@ genAtomicUpdate(lower::AbstractConverter &converter,
     // operations with exactly two (non-optional) arguments.
     rhs = genReducedMinMax(rhs, atomArg, args);
     input = *evaluate::GetConvertInput(rhs);
-    std::tie(opcode, args) =
-        evaluate::GetTopLevelOperationIgnoreResizing(input);
+    auto [opcode, args] = evaluate::GetTopLevelOperationIgnoreResizing(input);
     atomArg = nullptr; // No longer valid.
   }
   for (auto &arg : args) {

From c4e897038ac706dfe5b9f36bc5a223894012a3ac Mon Sep 17 00:00:00 2001
From: Andre Kuhlenschmidt <akuhlenschmi@nvidia.com>
Date: Wed, 30 Jul 2025 06:43:31 -0700
Subject: [PATCH 8/8] revert remove std::tie

---
 flang/lib/Lower/OpenMP/Atomic.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/flang/lib/Lower/OpenMP/Atomic.cpp b/flang/lib/Lower/OpenMP/Atomic.cpp
index fdb90a32fffae..c9a6dbabd4182 100644
--- a/flang/lib/Lower/OpenMP/Atomic.cpp
+++ b/flang/lib/Lower/OpenMP/Atomic.cpp
@@ -625,7 +625,8 @@ genAtomicUpdate(lower::AbstractConverter &converter,
     // operations with exactly two (non-optional) arguments.
     rhs = genReducedMinMax(rhs, atomArg, args);
     input = *evaluate::GetConvertInput(rhs);
-    auto [opcode, args] = evaluate::GetTopLevelOperationIgnoreResizing(input);
+    std::tie(opcode, args) =
+        evaluate::GetTopLevelOperationIgnoreResizing(input);
     atomArg = nullptr; // No longer valid.
   }
   for (auto &arg : args) {