[flang-commits] [flang] early feedback commit (PR #149579)
Andre Kuhlenschmidt via flang-commits
flang-commits at lists.llvm.org
Mon Jul 28 08:59:47 PDT 2025
https://github.com/akuhlens updated https://github.com/llvm/llvm-project/pull/149579
>From 757f85b5743c86e81d3b28bd54b6c85d885fbfeb 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/3] 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 0fd7631f06a54339d1fe30a436ac9f2849196ef6 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/3] 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 8b6f2df78ef374e8a10f58761bf83b390202aab2 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/3] 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;
}
More information about the flang-commits
mailing list