[flang][OpenMP] Reassociate ATOMIC update expressions #153098
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Turn it into a class that combines the information and generates the analysis instead of having independent functions do it.
The structure of evaluate::Expr is highly customized for the specific operation or entity that it represents. The different cases are expressed with different types, which makes the traversal and modifications somewhat complicated. There exists a framework for read-only traversal (traverse.h), but there is nothing that helps with modifying evaluate::Expr. It's rare that evaluate::Expr needs to be modified, but for the cases where it needs to be, this code will make it easier.
There semantic analysis of the ATOMIC construct will require additional rewriting (reassociation of certain expressions for user convenience), and that will be driven by diagnoses made in the semantic checks. While the rewriting of min/max is not required to be done in semantic analysis, moving it there will make all rewriting for ATOMIC construct be located in a single location.
Implement a framework to make it easier to detect if evaluate::Expr<T> has certain structure.
An atomic update expression of form x = x + a + b is technically illegal, since the right-hand side is parsed as (x+a)+b, and the atomic variable x should be an argument to the top-level +. When the type of x is integer, the result of (x+a)+b is guaranteed to be the same as x+(a+b), so instead of reporting an error, the compiler can treat (x+a)+b as x+(a+b). This PR implements this kind of reassociation for integral types, and for the two arithmetic associative/commutative operators: + and *.
llvmbot
added
flang
Member
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-semantics Author: Krzysztof Parzyszek (kparzysz) ChangesAn atomic update expression of form This PR implements this kind of reassociation for integral types, and for the two arithmetic associative/commutative operators: + and *. Full diff: https://github.com/llvm/llvm-project/pull/153098.diff 5 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-atomic.cpp b/flang/lib/Semantics/check-omp-atomic.cpp
index 0c0e6158485e9..38f5ea114babd 100644
--- a/flang/lib/Semantics/check-omp-atomic.cpp
+++ b/flang/lib/Semantics/check-omp-atomic.cpp
@@ -13,7 +13,9 @@
#include "check-omp-structure.h"
#include "flang/Common/indirection.h"
+#include "flang/Common/template.h"
#include "flang/Evaluate/expression.h"
+#include "flang/Evaluate/match.h"
#include "flang/Evaluate/rewrite.h"
#include "flang/Evaluate/tools.h"
#include "flang/Parser/char-block.h"
@@ -50,6 +52,106 @@ static bool operator!=(const evaluate::Expr<T> &e, const evaluate::Expr<U> &f) {
return !(e == f);
}
+namespace {
+template <typename...> struct IsIntegral {
+ static constexpr bool value{false};
+};
+
+template <common::TypeCategory C, int K>
+struct IsIntegral<evaluate::Type<C, K>> {
+ static constexpr bool value{//
+ C == common::TypeCategory::Integer ||
+ C == common::TypeCategory::Unsigned ||
+ C == common::TypeCategory::Logical};
+};
+
+template <typename T> constexpr bool is_integral_v{IsIntegral<T>::value};
+
+template <typename T, typename Op0, typename Op1>
+using ReassocOpBase = evaluate::match::AnyOfPattern< //
+ evaluate::match::Add<T, Op0, Op1>, //
+ evaluate::match::Mul<T, Op0, Op1>>;
+
+template <typename T, typename Op0, typename Op1>
+struct ReassocOp : public ReassocOpBase<T, Op0, Op1> {
+ using Base = ReassocOpBase<T, Op0, Op1>;
+ using Base::Base;
+};
+
+template <typename T, typename Op0, typename Op1>
+ReassocOp<T, Op0, Op1> reassocOp(const Op0 &op0, const Op1 &op1) {
+ return ReassocOp<T, Op0, Op1>(op0, op1);
+}
+} // namespace
+
+struct ReassocRewriter : public evaluate::rewrite::Identity {
+ using Id = evaluate::rewrite::Identity;
+ using Id::operator();
+ struct NonIntegralTag {};
+
+ ReassocRewriter(const SomeExpr &atom) : atom_(atom) {}
+
+ template <typename T, typename U,
+ typename = std::enable_if_t<is_integral_v<T>>>
+ evaluate::Expr<T> operator()(evaluate::Expr<T> &&x, const U &u) {
+ evaluate::match::Expr<T> sub[3];
+ auto inner{reassocOp<T>(sub[0], sub[1])};
+ auto outer1{reassocOp<T>(inner, sub[2])};
+ auto outer2{reassocOp<T>(sub[2], inner)};
+ if ((match(outer1, x) && outer1.ref.index() == inner.ref.index()) ||
+ (match(outer2, x) && outer2.ref.index() == inner.ref.index())) {
+ size_t atomIdx{[&]() {
+ size_t idx;
+ for (idx = 0; idx != 3; ++idx) {
+ if (IsAtom(*sub[idx].ref)) {
+ break;
+ }
+ }
+ return idx;
+ }()};
+
+ if (atomIdx > 2) {
+ return Id::operator()(std::move(x), u);
+ }
+ return common::visit(
+ [&](auto &&s) {
+ using Expr = evaluate::Expr<T>;
+ using TypeS = llvm::remove_cvref_t<decltype(s)>;
+ if constexpr (common::HasMember<TypeS,
+ typename decltype(outer1)::MatchTypes>) {
+ Expr atom{*sub[atomIdx].ref};
+ Expr op1{*sub[(atomIdx + 1) % 3].ref};
+ Expr op2{*sub[(atomIdx + 2) % 3].ref};
+ return Expr(TypeS(atom, parenthesize(Expr(TypeS(op1, op2)))));
+ } else {
+ return Expr(TypeS(s));
+ }
+ },
+ evaluate::match::deparen(x).u);
+ }
+ return Id::operator()(std::move(x), u);
+ }
+
+ template <typename T, typename U,
+ typename = std::enable_if_t<!is_integral_v<T>>>
+ evaluate::Expr<T> operator()(
+ evaluate::Expr<T> &&x, const U &u, NonIntegralTag = {}) {
+ return Id::operator()(std::move(x), u);
+ }
+
+private:
+ template <typename T>
+ evaluate::Expr<T> parenthesize(const evaluate::Expr<T> &x) const {
+ return evaluate::Expr<T>(evaluate::Parentheses<T>(x));
+ }
+
+ template <typename T> bool IsAtom(const evaluate::Expr<T> &x) const {
+ return IsSameOrConvertOf(evaluate::AsGenericExpr(AsRvalue(x)), atom_);
+ }
+
+ const SomeExpr &atom_;
+};
+
struct AnalyzedCondStmt {
SomeExpr cond{evaluate::NullPointer{}}; // Default ctor is deleted
parser::CharBlock source;
@@ -199,6 +301,26 @@ static std::pair<parser::CharBlock, parser::CharBlock> SplitAssignmentSource(
llvm_unreachable("Could not find assignment operator");
}
+static std::vector<SomeExpr> GetNonAtomExpressions(
+ const SomeExpr &atom, const std::vector<SomeExpr> &exprs) {
+ std::vector<SomeExpr> nonAtom;
+ for (const SomeExpr &e : exprs) {
+ if (!IsSameOrConvertOf(e, atom)) {
+ nonAtom.push_back(e);
+ }
+ }
+ return nonAtom;
+}
+
+static std::vector<SomeExpr> GetNonAtomArguments(
+ const SomeExpr &atom, const SomeExpr &expr) {
+ if (auto &&maybe{GetConvertInput(expr)}) {
+ return GetNonAtomExpressions(
+ atom, GetTopLevelOperationIgnoreResizing(*maybe).second);
+ }
+ return {};
+}
+
static bool IsCheckForAssociated(const SomeExpr &cond) {
return GetTopLevelOperationIgnoreResizing(cond).first ==
operation::Operator::Associated;
@@ -625,7 +747,8 @@ void OmpStructureChecker::CheckAtomicWriteAssignment(
}
}
-void OmpStructureChecker::CheckAtomicUpdateAssignment(
+std::optional<evaluate::Assignment>
+OmpStructureChecker::CheckAtomicUpdateAssignment(
const evaluate::Assignment &update, parser::CharBlock source) {
// [6.0:191:1-7]
// An update structured block is update-statement, an update statement
@@ -641,14 +764,46 @@ void OmpStructureChecker::CheckAtomicUpdateAssignment(
if (!IsVarOrFunctionRef(atom)) {
ErrorShouldBeVariable(atom, rsrc);
// Skip other checks.
- return;
+ return std::nullopt;
}
CheckAtomicVariable(atom, lsrc);
+ auto [hasErrors, tryReassoc]{CheckAtomicUpdateAssignmentRhs(
+ atom, update.rhs, source, /*suppressDiagnostics=*/true)};
+
+ if (!hasErrors) {
+ CheckStorageOverlap(atom, GetNonAtomArguments(atom, update.rhs), source);
+ return std::nullopt;
+ } else if (tryReassoc) {
+ ReassocRewriter ra(atom);
+ SomeExpr raRhs{evaluate::rewrite::Mutator(ra)(update.rhs)};
+
+ std::tie(hasErrors, tryReassoc) = CheckAtomicUpdateAssignmentRhs(
+ atom, raRhs, source, /*suppressDiagnostics=*/true);
+ if (!hasErrors) {
+ CheckStorageOverlap(atom, GetNonAtomArguments(atom, raRhs), source);
+
+ evaluate::Assignment raAssign(update);
+ raAssign.rhs = raRhs;
+ return raAssign;
+ }
+ }
+
+ // This is guaranteed to report errors.
+ CheckAtomicUpdateAssignmentRhs(
+ atom, update.rhs, source, /*suppressDiagnostics=*/false);
+ return std::nullopt;
+}
+
+std::pair<bool, bool> OmpStructureChecker::CheckAtomicUpdateAssignmentRhs(
+ const SomeExpr &atom, const SomeExpr &rhs, parser::CharBlock source,
+ bool suppressDiagnostics) {
+ auto [lsrc, rsrc]{SplitAssignmentSource(source)};
+
std::pair<operation::Operator, std::vector<SomeExpr>> top{
operation::Operator::Unknown, {}};
- if (auto &&maybeInput{GetConvertInput(update.rhs)}) {
+ if (auto &&maybeInput{GetConvertInput(rhs)}) {
top = GetTopLevelOperationIgnoreResizing(*maybeInput);
}
switch (top.first) {
@@ -665,29 +820,39 @@ void OmpStructureChecker::CheckAtomicUpdateAssignment(
case operation::Operator::Identity:
break;
case operation::Operator::Call:
- context_.Say(source,
- "A call to this function is not a valid ATOMIC UPDATE operation"_err_en_US);
- return;
+ if (!suppressDiagnostics) {
+ context_.Say(source,
+ "A call to this function is not a valid ATOMIC UPDATE operation"_err_en_US);
+ }
+ return std::make_pair(true, false);
case operation::Operator::Convert:
- context_.Say(source,
- "An implicit or explicit type conversion is not a valid ATOMIC UPDATE operation"_err_en_US);
- return;
+ if (!suppressDiagnostics) {
+ context_.Say(source,
+ "An implicit or explicit type conversion is not a valid ATOMIC UPDATE operation"_err_en_US);
+ }
+ return std::make_pair(true, false);
case operation::Operator::Intrinsic:
- context_.Say(source,
- "This intrinsic function is not a valid ATOMIC UPDATE operation"_err_en_US);
- return;
+ if (!suppressDiagnostics) {
+ context_.Say(source,
+ "This intrinsic function is not a valid ATOMIC UPDATE operation"_err_en_US);
+ }
+ return std::make_pair(true, false);
case operation::Operator::Constant:
case operation::Operator::Unknown:
- context_.Say(
- source, "This is not a valid ATOMIC UPDATE operation"_err_en_US);
- return;
+ if (!suppressDiagnostics) {
+ context_.Say(
+ source, "This is not a valid ATOMIC UPDATE operation"_err_en_US);
+ }
+ return std::make_pair(true, false);
default:
assert(
top.first != operation::Operator::Identity && "Handle this separately");
- context_.Say(source,
- "The %s operator is not a valid ATOMIC UPDATE operation"_err_en_US,
- operation::ToString(top.first));
- return;
+ if (!suppressDiagnostics) {
+ context_.Say(source,
+ "The %s operator is not a valid ATOMIC UPDATE operation"_err_en_US,
+ operation::ToString(top.first));
+ }
+ return std::make_pair(true, false);
}
// Check how many times `atom` occurs as an argument, if it's a subexpression
// of an argument, and collect the non-atom arguments.
@@ -708,39 +873,48 @@ void OmpStructureChecker::CheckAtomicUpdateAssignment(
return count;
}()};
- bool hasError{false};
+ bool hasError{false}, tryReassoc{false};
if (subExpr) {
- context_.Say(rsrc,
- "The atomic variable %s cannot be a proper subexpression of an argument (here: %s) in the update operation"_err_en_US,
- atom.AsFortran(), subExpr->AsFortran());
+ if (!suppressDiagnostics) {
+ context_.Say(rsrc,
+ "The atomic variable %s cannot be a proper subexpression of an argument (here: %s) in the update operation"_err_en_US,
+ atom.AsFortran(), subExpr->AsFortran());
+ }
hasError = true;
}
if (top.first == operation::Operator::Identity) {
// This is "x = y".
assert((atomCount == 0 || atomCount == 1) && "Unexpected count");
if (atomCount == 0) {
- context_.Say(rsrc,
- "The atomic variable %s should appear as an argument in the update operation"_err_en_US,
- atom.AsFortran());
+ if (!suppressDiagnostics) {
+ context_.Say(rsrc,
+ "The atomic variable %s should appear as an argument in the update operation"_err_en_US,
+ atom.AsFortran());
+ }
hasError = true;
}
} else {
if (atomCount == 0) {
- context_.Say(rsrc,
- "The atomic variable %s should appear as an argument of the top-level %s operator"_err_en_US,
- atom.AsFortran(), operation::ToString(top.first));
+ if (!suppressDiagnostics) {
+ context_.Say(rsrc,
+ "The atomic variable %s should appear as an argument of the top-level %s operator"_err_en_US,
+ atom.AsFortran(), operation::ToString(top.first));
+ }
+ // If `atom` is a proper subexpression, and it not present as an
+ // argument on its own, reassociation may be able to help.
+ tryReassoc = subExpr.has_value();
hasError = true;
} else if (atomCount > 1) {
- context_.Say(rsrc,
- "The atomic variable %s should be exactly one of the arguments of the top-level %s operator"_err_en_US,
- atom.AsFortran(), operation::ToString(top.first));
+ if (!suppressDiagnostics) {
+ context_.Say(rsrc,
+ "The atomic variable %s should be exactly one of the arguments of the top-level %s operator"_err_en_US,
+ atom.AsFortran(), operation::ToString(top.first));
+ }
hasError = true;
}
}
- if (!hasError) {
- CheckStorageOverlap(atom, nonAtom, source);
- }
+ return std::make_pair(hasError, tryReassoc);
}
void OmpStructureChecker::CheckAtomicConditionalUpdateAssignment(
@@ -843,11 +1017,13 @@ void OmpStructureChecker::CheckAtomicUpdateOnly(
SourcedActionStmt action{GetActionStmt(&body.front())};
if (auto maybeUpdate{GetEvaluateAssignment(action.stmt)}) {
const SomeExpr &atom{maybeUpdate->lhs};
- CheckAtomicUpdateAssignment(*maybeUpdate, action.source);
+ auto maybeAssign{
+ CheckAtomicUpdateAssignment(*maybeUpdate, action.source)};
+ auto &updateAssign{maybeAssign.has_value() ? maybeAssign : maybeUpdate};
using Analysis = parser::OpenMPAtomicConstruct::Analysis;
x.analysis = AtomicAnalysis(atom)
- .addOp0(Analysis::Update, maybeUpdate)
+ .addOp0(Analysis::Update, updateAssign)
.addOp1(Analysis::None);
} else if (!IsAssignment(action.stmt)) {
context_.Say(
@@ -963,16 +1139,19 @@ void OmpStructureChecker::CheckAtomicUpdateCapture(
using Analysis = parser::OpenMPAtomicConstruct::Analysis;
int action;
+ std::optional<evaluate::Assignment> updateAssign{update};
if (IsMaybeAtomicWrite(update)) {
action = Analysis::Write;
CheckAtomicWriteAssignment(update, uact.source);
} else {
action = Analysis::Update;
- CheckAtomicUpdateAssignment(update, uact.source);
+ if (auto &&maybe{CheckAtomicUpdateAssignment(update, uact.source)}) {
+ updateAssign = maybe;
+ }
}
CheckAtomicCaptureAssignment(capture, atom, cact.source);
- if (IsPointerAssignment(update) != IsPointerAssignment(capture)) {
+ if (IsPointerAssignment(*updateAssign) != IsPointerAssignment(capture)) {
context_.Say(cact.source,
"The update and capture assignments should both be pointer-assignments or both be non-pointer-assignments"_err_en_US);
return;
@@ -980,12 +1159,12 @@ void OmpStructureChecker::CheckAtomicUpdateCapture(
if (GetActionStmt(&body.front()).stmt == uact.stmt) {
x.analysis = AtomicAnalysis(atom)
- .addOp0(action, update)
+ .addOp0(action, updateAssign)
.addOp1(Analysis::Read, capture);
} else {
x.analysis = AtomicAnalysis(atom)
.addOp0(Analysis::Read, capture)
- .addOp1(action, update);
+ .addOp1(action, updateAssign);
}
}
diff --git a/flang/lib/Semantics/check-omp-structure.h b/flang/lib/Semantics/check-omp-structure.h
index 6b33ca6ab583f..f9c61bdd9fc1c 100644
--- a/flang/lib/Semantics/check-omp-structure.h
+++ b/flang/lib/Semantics/check-omp-structure.h
@@ -267,8 +267,11 @@ class OmpStructureChecker
const evaluate::Assignment &read, parser::CharBlock source);
void CheckAtomicWriteAssignment(
const evaluate::Assignment &write, parser::CharBlock source);
- void CheckAtomicUpdateAssignment(
+ std::optional<evaluate::Assignment> CheckAtomicUpdateAssignment(
const evaluate::Assignment &update, parser::CharBlock source);
+ std::pair<bool, bool> CheckAtomicUpdateAssignmentRhs(
+ const SomeExpr &atom, const SomeExpr &rhs, parser::CharBlock source,
+ bool suppressDiagnostics);
void CheckAtomicConditionalUpdateAssignment(const SomeExpr &cond,
parser::CharBlock condSource, const evaluate::Assignment &assign,
parser::CharBlock assignSource);
diff --git a/flang/test/Lower/OpenMP/atomic-update-reassoc.f90 b/flang/test/Lower/OpenMP/atomic-update-reassoc.f90
new file mode 100644
index 0000000000000..25e8cf62d8630
--- /dev/null
+++ b/flang/test/Lower/OpenMP/atomic-update-reassoc.f90
@@ -0,0 +1,80 @@
+!RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=60 %s -o - | FileCheck %s
+
+subroutine f00(x, y)
+ implicit none
+ integer :: x, y
+
+ !$omp atomic update
+ x = ((x + 1) + y) + 2
+end
+
+!CHECK-LABEL: func.func @_QPf00
+!CHECK: %[[X:[0-9]+]]:2 = hlfir.declare %arg0
+!CHECK: %[[Y:[0-9]+]]:2 = hlfir.declare %arg1
+!CHECK: %c1_i32 = arith.constant 1 : i32
+!CHECK: %[[LOAD_Y:[0-9]+]] = fir.load %[[Y]]#0 : !fir.ref<i32>
+!CHECK: %[[Y_1:[0-9]+]] = arith.addi %c1_i32, %[[LOAD_Y]] : i32
+!CHECK: %[[Y_1_NR:[0-9]+]] = hlfir.no_reassoc %[[Y_1]] : i32
+!CHECK: %c2_i32 = arith.constant 2 : i32
+!CHECK: %[[Y_1_2:[0-9]+]] = arith.addi %[[Y_1_NR]], %c2_i32 : i32
+!CHECK: %[[Y_1_2_NR:[0-9]+]] = hlfir.no_reassoc %[[Y_1_2]] : i32
+!CHECK: omp.atomic.update memory_order(relaxed) %[[X]]#0 : !fir.ref<i32> {
+!CHECK: ^bb0(%[[ARG:arg[0-9]+]]: i32):
+!CHECK: %[[ARG_P:[0-9]+]] = arith.addi %[[ARG]], %[[Y_1_2_NR]] : i32
+!CHECK: omp.yield(%[[ARG_P]] : i32)
+!CHECK: }
+
+
+subroutine f01(x, y)
+ implicit none
+ real :: x
+ integer :: y
+
+ !$omp atomic update
+ x = (int(x) + y) + 1
+end
+
+!CHECK-LABEL: func.func @_QPf01
+!CHECK: %[[X:[0-9]+]]:2 = hlfir.declare %arg0
+!CHECK: %[[Y:[0-9]+]]:2 = hlfir.declare %arg1
+!CHECK: %[[LOAD_Y:[0-9]+]] = fir.load %[[Y]]#0 : !fir.ref<i32>
+!CHECK: %c1_i32 = arith.constant 1 : i32
+!CHECK: %[[Y_1:[0-9]+]] = arith.addi %[[LOAD_Y]], %c1_i32 : i32
+!CHECK: %[[Y_1_NR:[0-9]+]] = hlfir.no_reassoc %[[Y_1]] : i32
+!CHECK: omp.atomic.update memory_order(relaxed) %[[X]]#0 : !fir.ref<f32> {
+!CHECK: ^bb0(%[[ARG:arg[0-9]+]]: f32):
+!CHECK: %[[ARG_I:[0-9]+]] = fir.convert %[[ARG]] : (f32) -> i32
+!CHECK: %[[ARG_P:[0-9]+]] = arith.addi %[[ARG_I]], %[[Y_1_NR]] : i32
+!CHECK: %[[ARG_F:[0-9]+]] = fir.convert %[[ARG_P]] : (i32) -> f32
+!CHECK: omp.yield(%[[ARG_F]] : f32)
+!CHECK: }
+
+
+subroutine f02(x, a, b, c)
+ implicit none
+ integer(kind=4) :: x
+ integer(kind=8) :: a, b, c
+
+ !$omp atomic update
+ x = ((b + a) + x) + c
+end
+
+!CHECK-LABEL: func.func @_QPf02
+!CHECK: %[[A:[0-9]+]]:2 = hlfir.declare %arg1
+!CHECK: %[[B:[0-9]+]]:2 = hlfir.declare %arg2
+!CHECK: %[[C:[0-9]+]]:2 = hlfir.declare %arg3
+!CHECK: %[[X:[0-9]+]]:2 = hlfir.declare %arg0
+!CHECK: %[[LOAD_B:[0-9]+]] = fir.load %[[B]]#0 : !fir.ref<i64>
+!CHECK: %[[LOAD_A:[0-9]+]] = fir.load %[[A]]#0 : !fir.ref<i64>
+!CHECK: %[[A_B:[0-9]+]] = arith.addi %[[LOAD_B]], %[[LOAD_A]] : i64
+!CHECK: %[[A_B_NR:[0-9]+]] = hlfir.no_reassoc %[[A_B]] : i64
+!CHECK: %[[LOAD_C:[0-9]+]] = fir.load %[[C]]#0 : !fir.ref<i64>
+!CHECK: %[[A_B_C:[0-9]+]] = arith.addi %[[A_B_NR]], %[[LOAD_C]] : i64
+!CHECK: %[[A_B_C_NR:[0-9]+]] = hlfir.no_reassoc %[[A_B_C]] : i64
+!CHECK: omp.atomic.update memory_order(relaxed) %[[X]]#0 : !fir.ref<i32> {
+!CHECK: ^bb0(%[[ARG:arg[0-9]+]]: i32):
+!CHECK: %[[ARG_8:[0-9]+]] = fir.convert %[[ARG]] : (i32) -> i64
+!CHECK: %[[ARG_P:[0-9]+]] = arith.addi %[[ARG_8]], %[[A_B_C_NR]] : i64
+!CHECK: %[[ARG_4:[0-9]+]] = fir.convert %[[ARG_P]] : (i64) -> i32
+!CHECK: omp.yield(%[[ARG_4]] : i32)
+!CHECK: }
diff --git a/flang/test/Semantics/OpenMP/atomic-update-only.f90 b/flang/test/Semantics/OpenMP/atomic-update-only.f90
index 3c027924a1423..8ae261c463b00 100644
--- a/flang/test/Semantics/OpenMP/atomic-update-only.f90
+++ b/flang/test/Semantics/OpenMP/atomic-update-only.f90
@@ -28,11 +28,18 @@ subroutine f02
subroutine f03
integer :: x, y
+ real :: xr, yr
+ !With integer type the reassociation should be able to bring the `x` to
+ !the top of the + operator. Expect no diagnostics.
!$omp atomic update
- !ERROR: The atomic variable x cannot be a proper subexpression of an argument (here: (x+y)) in the update operation
- !ERROR: The atomic variable x should appear as an argument of the top-level + operator
x = (x + y) + 1
+
+ !Real variables cannot be reassociated (unless fastmath options are present).
+ !$omp atomic update
+ !ERROR: The atomic variable xr cannot be a proper subexpression of an argument (here: (xr+yr)) in the update operation
+ !ERROR: The atomic variable xr should appear as an argument of the top-level + operator
+ xr = (xr + yr) + 1
end
subroutine f04
diff --git a/flang/test/Semantics/OpenMP/atomic04.f90 b/flang/test/Semantics/OpenMP/atomic04.f90
index 8f8af31245404..002e06be68656 100644
--- a/flang/test/Semantics/OpenMP/atomic04.f90
+++ b/flang/test/Semantics/OpenMP/atomic04.f90
@@ -205,9 +205,8 @@ subroutine more_invalid_atomic_update_stmts()
!ERROR: The atomic variable a should appear as an argument of the top-level + operator
a = a * b + c
+ !This is expected to work due to reassociation.
!$omp atomic update
- !ERROR: The atomic variable a cannot be a proper subexpression of an argument (here: a+b) in the update operation
- !ERROR: The atomic variable a should appear as an argument of the top-level + operator
a = a + b + c
!$omp atomic
|
This was referenced Aug 11, 2025
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
tblah
approved these changes
Aug 13, 2025
Contributor
tblah
left a comment
tblah
left a comment
There was a problem hiding this comment.
This is great. Thank you for adding the extra comments, I think they make it a lot easier to follow what is being done here.
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/204/builds/18491 Here is the relevant piece of the build log for the reference |
Contributor
|
Broke my builds on x86 and aarch64; please fix or revert quickly, I'm dead in the water. |
Contributor
Author
|
Reverted in 6b5c38d. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An atomic update expression of form
x = x + a + b
is technically illegal, since the right-hand side is parsed as (x+a)+b, and the atomic variable x should be an argument to the top-level +. When the type of x is integer, the result of (x+a)+b is guaranteed to be the same as x+(a+b), so instead of reporting an error, the compiler can treat (x+a)+b as x+(a+b).
This PR implements this kind of reassociation for integral types, and for the two arithmetic associative/commutative operators: + and *.