[flang-commits] [flang] [flang][openacc] Add semantic checks for atomic constructs (PR #149579)
Andre Kuhlenschmidt via flang-commits
flang-commits at lists.llvm.org
Wed Jul 30 07:11:36 PDT 2025
https://github.com/akuhlens updated https://github.com/llvm/llvm-project/pull/149579
>From e39900727ba16b78a4434876cb21103078821feb Mon Sep 17 00:00:00 2001
From: Andre Kuhlenschmidt <akuhlenschmi at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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) {
More information about the flang-commits
mailing list