From 6b5c38dbbb14e09733e8046f4a70598d128f5484 Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Wed, 13 Aug 2025 10:25:24 -0500 Subject: [PATCH] Revert "[flang][OpenMP] Reassociate ATOMIC update expressions (#153098)" This reverts commit 4f6ae2af3563a7eefbe4179eabe10ef5898a5963. This PR causes build breaks with older versions of GCC. --- flang/lib/Semantics/check-omp-atomic.cpp | 282 +++--------------- flang/lib/Semantics/check-omp-structure.h | 4 +- .../Lower/OpenMP/atomic-update-reassoc.f90 | 75 ----- .../Semantics/OpenMP/atomic-update-only.f90 | 11 +- flang/test/Semantics/OpenMP/atomic04.f90 | 3 +- 5 files changed, 46 insertions(+), 329 deletions(-) delete mode 100644 flang/test/Lower/OpenMP/atomic-update-reassoc.f90 diff --git a/flang/lib/Semantics/check-omp-atomic.cpp b/flang/lib/Semantics/check-omp-atomic.cpp index f98a00070f74..0c0e6158485e 100644 --- a/flang/lib/Semantics/check-omp-atomic.cpp +++ b/flang/lib/Semantics/check-omp-atomic.cpp @@ -13,9 +13,7 @@ #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" @@ -52,127 +50,6 @@ static bool operator!=(const evaluate::Expr &e, const evaluate::Expr &f) { return !(e == f); } -namespace { -template struct IsIntegral { - static constexpr bool value{false}; -}; - -template -struct IsIntegral> { - static constexpr bool value{// - C == common::TypeCategory::Integer || - C == common::TypeCategory::Unsigned || - C == common::TypeCategory::Logical}; -}; - -template constexpr bool is_integral_v{IsIntegral::value}; - -template -using ReassocOpBase = evaluate::match::AnyOfPattern< // - evaluate::match::Add, // - evaluate::match::Mul>; - -template -struct ReassocOp : public ReassocOpBase { - using Base = ReassocOpBase; - using Base::Base; -}; - -template -ReassocOp reassocOp(const Op0 &op0, const Op1 &op1) { - return ReassocOp(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) {} - - // Try to find cases where the input expression is of the form - // (1) (a . b) . c, or - // (2) a . (b . c), - // where . denotes an associative operation (currently + or *), and a, b, c - // are some subexpresions. - // If one of the operands in the nested operation is the atomic variable - // (with some possible type conversions applied to it), bring it to the - // top-level operation, and move the top-level operand into the nested - // operation. - // For example, assuming x is the atomic variable: - // (a + x) + b -> (a + b) + x, i.e. (conceptually) swap x and b. - template >> - evaluate::Expr operator()(evaluate::Expr &&x, const U &u) { - // As per the above comment, there are 3 subexpressions involved in this - // transformation. A match::Expr will match evaluate::Expr when T is - // same as U, plus it will store a pointer (ref) to the matched expression. - // When the match is successful, the sub[i].ref will point to a, b, x (in - // some order) from the example above. - evaluate::match::Expr sub[3]; - auto inner{reassocOp(sub[0], sub[1])}; - auto outer1{reassocOp(inner, sub[2])}; // inner + something - auto outer2{reassocOp(sub[2], inner)}; // something + inner - // There is no way to ensure that the outer operation is the same as - // the inner one. They are matched independently, so we need to compare - // the index in the member variant that represents the matched type. - if ((match(outer1, x) && outer1.ref.index() == inner.ref.index()) || - (match(outer2, x) && outer2.ref.index() == inner.ref.index())) { - size_t atomIdx{[&]() { // sub[atomIdx] will be the atom. - 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; - using TypeS = llvm::remove_cvref_t; - // This visitor has to be semantically correct for all possible - // types of s even though at runtime s will only be one of the - // matched types. - // Limit the construction to the operation types that we tried - // to match (otherwise TypeS(op1, op2) would fail for non-binary - // operations). - if constexpr (common::HasMember) { - Expr atom{*sub[atomIdx].ref}; - Expr op1{*sub[(atomIdx + 1) % 3].ref}; - Expr op2{*sub[(atomIdx + 2) % 3].ref}; - return Expr( - TypeS(atom, Expr(TypeS(std::move(op1), std::move(op2))))); - } else { - return Expr(TypeS(s)); - } - }, - evaluate::match::deparen(x).u); - } - return Id::operator()(std::move(x), u); - } - - template >> - evaluate::Expr operator()( - evaluate::Expr &&x, const U &u, NonIntegralTag = {}) { - return Id::operator()(std::move(x), u); - } - -private: - template bool IsAtom(const evaluate::Expr &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; @@ -322,26 +199,6 @@ static std::pair SplitAssignmentSource( llvm_unreachable("Could not find assignment operator"); } -static std::vector GetNonAtomExpressions( - const SomeExpr &atom, const std::vector &exprs) { - std::vector nonAtom; - for (const SomeExpr &e : exprs) { - if (!IsSameOrConvertOf(e, atom)) { - nonAtom.push_back(e); - } - } - return nonAtom; -} - -static std::vector 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; @@ -768,8 +625,7 @@ void OmpStructureChecker::CheckAtomicWriteAssignment( } } -std::optional -OmpStructureChecker::CheckAtomicUpdateAssignment( +void OmpStructureChecker::CheckAtomicUpdateAssignment( const evaluate::Assignment &update, parser::CharBlock source) { // [6.0:191:1-7] // An update structured block is update-statement, an update statement @@ -785,46 +641,14 @@ OmpStructureChecker::CheckAtomicUpdateAssignment( if (!IsVarOrFunctionRef(atom)) { ErrorShouldBeVariable(atom, rsrc); // Skip other checks. - return std::nullopt; + return; } 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 OmpStructureChecker::CheckAtomicUpdateAssignmentRhs( - const SomeExpr &atom, const SomeExpr &rhs, parser::CharBlock source, - bool suppressDiagnostics) { - auto [lsrc, rsrc]{SplitAssignmentSource(source)}; - std::pair> top{ operation::Operator::Unknown, {}}; - if (auto &&maybeInput{GetConvertInput(rhs)}) { + if (auto &&maybeInput{GetConvertInput(update.rhs)}) { top = GetTopLevelOperationIgnoreResizing(*maybeInput); } switch (top.first) { @@ -841,39 +665,29 @@ std::pair OmpStructureChecker::CheckAtomicUpdateAssignmentRhs( case operation::Operator::Identity: break; case operation::Operator::Call: - 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); + context_.Say(source, + "A call to this function is not a valid ATOMIC UPDATE operation"_err_en_US); + return; case operation::Operator::Convert: - 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); + context_.Say(source, + "An implicit or explicit type conversion is not a valid ATOMIC UPDATE operation"_err_en_US); + return; case operation::Operator::Intrinsic: - if (!suppressDiagnostics) { - context_.Say(source, - "This intrinsic function is not a valid ATOMIC UPDATE operation"_err_en_US); - } - return std::make_pair(true, false); + context_.Say(source, + "This intrinsic function is not a valid ATOMIC UPDATE operation"_err_en_US); + return; case operation::Operator::Constant: case operation::Operator::Unknown: - if (!suppressDiagnostics) { - context_.Say( - source, "This is not a valid ATOMIC UPDATE operation"_err_en_US); - } - return std::make_pair(true, false); + context_.Say( + source, "This is not a valid ATOMIC UPDATE operation"_err_en_US); + return; default: assert( top.first != operation::Operator::Identity && "Handle this separately"); - 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); + context_.Say(source, + "The %s operator is not a valid ATOMIC UPDATE operation"_err_en_US, + operation::ToString(top.first)); + return; } // Check how many times `atom` occurs as an argument, if it's a subexpression // of an argument, and collect the non-atom arguments. @@ -894,48 +708,39 @@ std::pair OmpStructureChecker::CheckAtomicUpdateAssignmentRhs( return count; }()}; - bool hasError{false}, tryReassoc{false}; + bool hasError{false}; if (subExpr) { - 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()); - } + 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) { - if (!suppressDiagnostics) { - context_.Say(rsrc, - "The atomic variable %s should appear as an argument in the update operation"_err_en_US, - atom.AsFortran()); - } + 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) { - 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(); + 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)); hasError = true; } else if (atomCount > 1) { - 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)); - } + 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; } } - return std::make_pair(hasError, tryReassoc); + if (!hasError) { + CheckStorageOverlap(atom, nonAtom, source); + } } void OmpStructureChecker::CheckAtomicConditionalUpdateAssignment( @@ -1038,13 +843,11 @@ void OmpStructureChecker::CheckAtomicUpdateOnly( SourcedActionStmt action{GetActionStmt(&body.front())}; if (auto maybeUpdate{GetEvaluateAssignment(action.stmt)}) { const SomeExpr &atom{maybeUpdate->lhs}; - auto maybeAssign{ - CheckAtomicUpdateAssignment(*maybeUpdate, action.source)}; - auto &updateAssign{maybeAssign.has_value() ? maybeAssign : maybeUpdate}; + CheckAtomicUpdateAssignment(*maybeUpdate, action.source); using Analysis = parser::OpenMPAtomicConstruct::Analysis; x.analysis = AtomicAnalysis(atom) - .addOp0(Analysis::Update, updateAssign) + .addOp0(Analysis::Update, maybeUpdate) .addOp1(Analysis::None); } else if (!IsAssignment(action.stmt)) { context_.Say( @@ -1160,19 +963,16 @@ void OmpStructureChecker::CheckAtomicUpdateCapture( using Analysis = parser::OpenMPAtomicConstruct::Analysis; int action; - std::optional updateAssign{update}; if (IsMaybeAtomicWrite(update)) { action = Analysis::Write; CheckAtomicWriteAssignment(update, uact.source); } else { action = Analysis::Update; - if (auto &&maybe{CheckAtomicUpdateAssignment(update, uact.source)}) { - updateAssign = maybe; - } + CheckAtomicUpdateAssignment(update, uact.source); } CheckAtomicCaptureAssignment(capture, atom, cact.source); - if (IsPointerAssignment(*updateAssign) != IsPointerAssignment(capture)) { + if (IsPointerAssignment(update) != 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; @@ -1180,12 +980,12 @@ void OmpStructureChecker::CheckAtomicUpdateCapture( if (GetActionStmt(&body.front()).stmt == uact.stmt) { x.analysis = AtomicAnalysis(atom) - .addOp0(action, updateAssign) + .addOp0(action, update) .addOp1(Analysis::Read, capture); } else { x.analysis = AtomicAnalysis(atom) .addOp0(Analysis::Read, capture) - .addOp1(action, updateAssign); + .addOp1(action, update); } } diff --git a/flang/lib/Semantics/check-omp-structure.h b/flang/lib/Semantics/check-omp-structure.h index a973aee28d0e..6b33ca6ab583 100644 --- a/flang/lib/Semantics/check-omp-structure.h +++ b/flang/lib/Semantics/check-omp-structure.h @@ -267,10 +267,8 @@ private: const evaluate::Assignment &read, parser::CharBlock source); void CheckAtomicWriteAssignment( const evaluate::Assignment &write, parser::CharBlock source); - std::optional CheckAtomicUpdateAssignment( + void CheckAtomicUpdateAssignment( const evaluate::Assignment &update, parser::CharBlock source); - std::pair 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 deleted file mode 100644 index 96ebb56b8d6c..000000000000 --- a/flang/test/Lower/OpenMP/atomic-update-reassoc.f90 +++ /dev/null @@ -1,75 +0,0 @@ -!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 -!CHECK: %[[Y_1:[0-9]+]] = arith.addi %c1_i32, %[[LOAD_Y]] : i32 -!CHECK: %c2_i32 = arith.constant 2 : i32 -!CHECK: %[[Y_1_2:[0-9]+]] = arith.addi %[[Y_1]], %c2_i32 : i32 -!CHECK: omp.atomic.update memory_order(relaxed) %[[X]]#0 : !fir.ref { -!CHECK: ^bb0(%[[ARG:arg[0-9]+]]: i32): -!CHECK: %[[ARG_P:[0-9]+]] = arith.addi %[[ARG]], %[[Y_1_2]] : 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 -!CHECK: %c1_i32 = arith.constant 1 : i32 -!CHECK: %[[Y_1:[0-9]+]] = arith.addi %[[LOAD_Y]], %c1_i32 : i32 -!CHECK: omp.atomic.update memory_order(relaxed) %[[X]]#0 : !fir.ref { -!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]] : 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 -!CHECK: %[[LOAD_A:[0-9]+]] = fir.load %[[A]]#0 : !fir.ref -!CHECK: %[[A_B:[0-9]+]] = arith.addi %[[LOAD_B]], %[[LOAD_A]] : i64 -!CHECK: %[[LOAD_C:[0-9]+]] = fir.load %[[C]]#0 : !fir.ref -!CHECK: %[[A_B_C:[0-9]+]] = arith.addi %[[A_B]], %[[LOAD_C]] : i64 -!CHECK: omp.atomic.update memory_order(relaxed) %[[X]]#0 : !fir.ref { -!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]] : 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 8ae261c463b0..3c027924a142 100644 --- a/flang/test/Semantics/OpenMP/atomic-update-only.f90 +++ b/flang/test/Semantics/OpenMP/atomic-update-only.f90 @@ -28,18 +28,11 @@ end 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 002e06be6865..8f8af3124540 100644 --- a/flang/test/Semantics/OpenMP/atomic04.f90 +++ b/flang/test/Semantics/OpenMP/atomic04.f90 @@ -205,8 +205,9 @@ 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