From 60b2f04925e5417eb5727e0ce8fb3b0b9e6558ad Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Thu, 16 Oct 2025 10:23:56 -0500 Subject: [PATCH 1/2] [flang][OpenMP] Handle conflicts between REQUIRES and ATOMIC restrictions When the atomic default memory order specified on a REQUIRES directive is disallowed on a given ATOMIC operation, and it's not ACQ_REL, the order reverts to RELAXED. ACQ_REL decays to either ACQUIRE or RELEASE, depending on the operation. This fixes MLIR verification failure in Fortran/gfortran/regression/gomp/requires-9.f90 --- flang/lib/Lower/OpenMP/Atomic.cpp | 97 ++++++++++++++++--- .../OpenMP/atomic-requires-conflict-v50-1.f90 | 11 +++ .../OpenMP/atomic-requires-conflict-v50-2.f90 | 11 +++ .../OpenMP/atomic-requires-conflict-v50-3.f90 | 11 +++ .../OpenMP/atomic-requires-conflict-v50-4.f90 | 13 +++ .../OpenMP/atomic-requires-conflict-v60-1.f90 | 11 +++ .../OpenMP/atomic-requires-conflict-v60-2.f90 | 11 +++ 7 files changed, 149 insertions(+), 16 deletions(-) create mode 100644 flang/test/Lower/OpenMP/atomic-requires-conflict-v50-1.f90 create mode 100644 flang/test/Lower/OpenMP/atomic-requires-conflict-v50-2.f90 create mode 100644 flang/test/Lower/OpenMP/atomic-requires-conflict-v50-3.f90 create mode 100644 flang/test/Lower/OpenMP/atomic-requires-conflict-v50-4.f90 create mode 100644 flang/test/Lower/OpenMP/atomic-requires-conflict-v60-1.f90 create mode 100644 flang/test/Lower/OpenMP/atomic-requires-conflict-v60-2.f90 diff --git a/flang/lib/Lower/OpenMP/Atomic.cpp b/flang/lib/Lower/OpenMP/Atomic.cpp index ff82a36951bfa..58df5fdb32aac 100644 --- a/flang/lib/Lower/OpenMP/Atomic.cpp +++ b/flang/lib/Lower/OpenMP/Atomic.cpp @@ -20,6 +20,7 @@ #include "flang/Optimizer/Builder/FIRBuilder.h" #include "flang/Optimizer/Builder/Todo.h" #include "flang/Parser/parse-tree.h" +#include "flang/Semantics/openmp-utils.h" #include "flang/Semantics/semantics.h" #include "flang/Semantics/type.h" #include "flang/Support/Fortran.h" @@ -183,12 +184,8 @@ getMemoryOrderFromRequires(const semantics::Scope &scope) { // scope. // For safety, traverse all enclosing scopes and check if their symbol // contains REQUIRES. - for (const auto *sc{&scope}; sc->kind() != semantics::Scope::Kind::Global; - sc = &sc->parent()) { - const semantics::Symbol *sym = sc->symbol(); - if (!sym) - continue; - + const semantics::Scope &unitScope = semantics::omp::GetProgramUnit(scope); + if (auto *symbol = unitScope.symbol()) { const common::OmpMemoryOrderType *admo = common::visit( [](auto &&s) { using WithOmpDeclarative = semantics::WithOmpDeclarative; @@ -198,7 +195,8 @@ getMemoryOrderFromRequires(const semantics::Scope &scope) { } return static_cast(nullptr); }, - sym->details()); + symbol->details()); + if (admo) return getMemoryOrderKind(*admo); } @@ -214,19 +212,83 @@ getDefaultAtomicMemOrder(semantics::SemanticsContext &semaCtx) { return std::nullopt; } -static std::optional +static std::pair, bool> getAtomicMemoryOrder(semantics::SemanticsContext &semaCtx, const omp::List &clauses, const semantics::Scope &scope) { for (const omp::Clause &clause : clauses) { if (auto maybeKind = getMemoryOrderKind(clause.id)) - return *maybeKind; + return std::make_pair(*maybeKind, /*canOverride=*/false); } if (auto maybeKind = getMemoryOrderFromRequires(scope)) - return *maybeKind; + return std::make_pair(*maybeKind, /*canOverride=*/true); - return getDefaultAtomicMemOrder(semaCtx); + return std::make_pair(getDefaultAtomicMemOrder(semaCtx), + /*canOverride=*/false); +} + +static std::optional +makeValidForAction(std::optional memOrder, + int action0, int action1, unsigned version) { +// When the atomic default memory order specified on a REQUIRES directive is +// disallowed on a given ATOMIC operation, and it's not ACQ_REL, the order +// reverts to RELAXED. ACQ_REL decays to either ACQUIRE or RELEASE, depending +// on the operation. + + if (!memOrder) { + return memOrder; + } + + using Analysis = parser::OpenMPAtomicConstruct::Analysis; + // Figure out the main action (i.e. disregard a potential capture operation) + int action = action0; + if (action1 != Analysis::None) + action = action0 == Analysis::Read ? action1 : action0; + + // Avaliable orderings: acquire, acq_rel, relaxed, release, seq_cst + + if (action == Analysis::Read) { + // "acq_rel" decays to "acquire" + if (*memOrder == mlir::omp::ClauseMemoryOrderKind::Acq_rel) + return mlir::omp::ClauseMemoryOrderKind::Acquire;; + } else if (action == Analysis::Write) { + // "acq_rel" decays to "release" + if (*memOrder == mlir::omp::ClauseMemoryOrderKind::Acq_rel) + return mlir::omp::ClauseMemoryOrderKind::Release; + } + + if (version > 50) { + if (action == Analysis::Read) { + // "release" prohibited + if (*memOrder == mlir::omp::ClauseMemoryOrderKind::Release) + return mlir::omp::ClauseMemoryOrderKind::Relaxed; + } + if (action == Analysis::Write) { + // "acquire" prohibited + if (*memOrder == mlir::omp::ClauseMemoryOrderKind::Acquire) + return mlir::omp::ClauseMemoryOrderKind::Relaxed; + } + } else { + if (action == Analysis::Read) { + // "release" prohibited + if (*memOrder == mlir::omp::ClauseMemoryOrderKind::Release) + return mlir::omp::ClauseMemoryOrderKind::Relaxed; + } else { + if (action & Analysis::Write) { // include "update" + // "acquire" prohibited + if (*memOrder == mlir::omp::ClauseMemoryOrderKind::Acquire) + return mlir::omp::ClauseMemoryOrderKind::Relaxed; + if (action == Analysis::Update) { + // "acq_rel" prohibited + if (*memOrder == mlir::omp::ClauseMemoryOrderKind::Acq_rel) + return mlir::omp::ClauseMemoryOrderKind::Relaxed; + } + } + } + } + + return memOrder; } static mlir::omp::ClauseMemoryOrderKindAttr @@ -449,16 +511,19 @@ void Fortran::lower::omp::lowerAtomic( mlir::Value atomAddr = fir::getBase(converter.genExprAddr(atom, stmtCtx, &loc)); mlir::IntegerAttr hint = getAtomicHint(converter, clauses); - std::optional memOrder = - getAtomicMemoryOrder(semaCtx, clauses, - semaCtx.FindScope(construct.source)); + auto [memOrder, canOverride] = getAtomicMemoryOrder( + semaCtx, clauses, semaCtx.FindScope(construct.source)); + + unsigned version = semaCtx.langOptions().OpenMPVersion; + int action0 = analysis.op0.what & analysis.Action; + int action1 = analysis.op1.what & analysis.Action; + if (canOverride) + memOrder = makeValidForAction(memOrder, action0, action1, version); if (auto *cond = get(analysis.cond)) { (void)cond; TODO(loc, "OpenMP ATOMIC COMPARE"); } else { - int action0 = analysis.op0.what & analysis.Action; - int action1 = analysis.op1.what & analysis.Action; mlir::Operation *captureOp = nullptr; fir::FirOpBuilder::InsertPoint preAt = builder.saveInsertionPoint(); fir::FirOpBuilder::InsertPoint atomicAt, postAt; diff --git a/flang/test/Lower/OpenMP/atomic-requires-conflict-v50-1.f90 b/flang/test/Lower/OpenMP/atomic-requires-conflict-v50-1.f90 new file mode 100644 index 0000000000000..eb8c4b4a343d0 --- /dev/null +++ b/flang/test/Lower/OpenMP/atomic-requires-conflict-v50-1.f90 @@ -0,0 +1,11 @@ +!RUN: bbc %openmp_flags -fopenmp-version=50 -emit-hlfir %s -o - | FileCheck %s +!RUN: %flang_fc1 -emit-hlfir %openmp_flags -fopenmp-version=50 %s -o - | FileCheck %s + +!CHECK: omp.atomic.read %{{[0-9]+}}#0 = %{{[0-9]+}}#0 memory_order(acquire) + +subroutine f00(x, v) + integer :: x, v + !$omp requires atomic_default_mem_order(acq_rel) + !$omp atomic read + v = x +end diff --git a/flang/test/Lower/OpenMP/atomic-requires-conflict-v50-2.f90 b/flang/test/Lower/OpenMP/atomic-requires-conflict-v50-2.f90 new file mode 100644 index 0000000000000..d309a21a2b938 --- /dev/null +++ b/flang/test/Lower/OpenMP/atomic-requires-conflict-v50-2.f90 @@ -0,0 +1,11 @@ +!RUN: bbc %openmp_flags -fopenmp-version=50 -emit-hlfir %s -o - | FileCheck %s +!RUN: %flang_fc1 -emit-hlfir %openmp_flags -fopenmp-version=50 %s -o - | FileCheck %s + +!CHECK: omp.atomic.write %{{[0-9]+}}#0 = %{{[0-9]+}} memory_order(relaxed) + +subroutine f02(x, v) + integer :: x, v + !$omp requires atomic_default_mem_order(acquire) + !$omp atomic write + x = v +end diff --git a/flang/test/Lower/OpenMP/atomic-requires-conflict-v50-3.f90 b/flang/test/Lower/OpenMP/atomic-requires-conflict-v50-3.f90 new file mode 100644 index 0000000000000..bc7529c69340a --- /dev/null +++ b/flang/test/Lower/OpenMP/atomic-requires-conflict-v50-3.f90 @@ -0,0 +1,11 @@ +!RUN: bbc %openmp_flags -fopenmp-version=50 -emit-hlfir %s -o - | FileCheck %s +!RUN: %flang_fc1 -emit-hlfir %openmp_flags -fopenmp-version=50 %s -o - | FileCheck %s + +!CHECK: omp.atomic.update memory_order(relaxed) + +subroutine f05(x, v) + integer :: x, v + !$omp requires atomic_default_mem_order(acq_rel) + !$omp atomic update + x = x + 1 +end diff --git a/flang/test/Lower/OpenMP/atomic-requires-conflict-v50-4.f90 b/flang/test/Lower/OpenMP/atomic-requires-conflict-v50-4.f90 new file mode 100644 index 0000000000000..5cffb1ac9d6c4 --- /dev/null +++ b/flang/test/Lower/OpenMP/atomic-requires-conflict-v50-4.f90 @@ -0,0 +1,13 @@ +!RUN: bbc %openmp_flags -fopenmp-version=50 -emit-hlfir %s -o - | FileCheck %s +!RUN: %flang_fc1 -emit-hlfir %openmp_flags -fopenmp-version=50 %s -o - | FileCheck %s + +!CHECK: omp.atomic.capture memory_order(relaxed) + +subroutine f06(x, v) + integer :: x, v + !$omp requires atomic_default_mem_order(acquire) + !$omp atomic update capture + v = x + x = x + 1 + !$omp end atomic +end diff --git a/flang/test/Lower/OpenMP/atomic-requires-conflict-v60-1.f90 b/flang/test/Lower/OpenMP/atomic-requires-conflict-v60-1.f90 new file mode 100644 index 0000000000000..55f21978b715e --- /dev/null +++ b/flang/test/Lower/OpenMP/atomic-requires-conflict-v60-1.f90 @@ -0,0 +1,11 @@ +!RUN: bbc %openmp_flags -fopenmp-version=60 -emit-hlfir %s -o - | FileCheck %s +!RUN: %flang_fc1 -emit-hlfir %openmp_flags -fopenmp-version=60 %s -o - | FileCheck %s + +!CHECK: omp.atomic.read %{{[0-9]+}}#0 = %{{[0-9]+}}#0 memory_order(relaxed) + +subroutine f01(x, v) + integer :: x, v + !$omp requires atomic_default_mem_order(release) + !$omp atomic read + v = x +end diff --git a/flang/test/Lower/OpenMP/atomic-requires-conflict-v60-2.f90 b/flang/test/Lower/OpenMP/atomic-requires-conflict-v60-2.f90 new file mode 100644 index 0000000000000..ca048798d8d18 --- /dev/null +++ b/flang/test/Lower/OpenMP/atomic-requires-conflict-v60-2.f90 @@ -0,0 +1,11 @@ +!RUN: bbc %openmp_flags -fopenmp-version=60 -emit-hlfir %s -o - | FileCheck %s +!RUN: %flang_fc1 -emit-hlfir %openmp_flags -fopenmp-version=60 %s -o - | FileCheck %s + +!CHECK: omp.atomic.write %{{[0-9]+}}#0 = %{{[0-9]+}} memory_order(relaxed) + +subroutine f02(x, v) + integer :: x, v + !$omp requires atomic_default_mem_order(acquire) + !$omp atomic write + x = v +end From 08561d2dc0c346a0fff356bca9c9e4caf384e88c Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Thu, 16 Oct 2025 10:32:53 -0500 Subject: [PATCH 2/2] format --- flang/lib/Lower/OpenMP/Atomic.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/flang/lib/Lower/OpenMP/Atomic.cpp b/flang/lib/Lower/OpenMP/Atomic.cpp index 58df5fdb32aac..3ab8a5891e8a6 100644 --- a/flang/lib/Lower/OpenMP/Atomic.cpp +++ b/flang/lib/Lower/OpenMP/Atomic.cpp @@ -231,10 +231,10 @@ getAtomicMemoryOrder(semantics::SemanticsContext &semaCtx, static std::optional makeValidForAction(std::optional memOrder, int action0, int action1, unsigned version) { -// When the atomic default memory order specified on a REQUIRES directive is -// disallowed on a given ATOMIC operation, and it's not ACQ_REL, the order -// reverts to RELAXED. ACQ_REL decays to either ACQUIRE or RELEASE, depending -// on the operation. + // When the atomic default memory order specified on a REQUIRES directive is + // disallowed on a given ATOMIC operation, and it's not ACQ_REL, the order + // reverts to RELAXED. ACQ_REL decays to either ACQUIRE or RELEASE, depending + // on the operation. if (!memOrder) { return memOrder; @@ -251,7 +251,7 @@ makeValidForAction(std::optional memOrder, if (action == Analysis::Read) { // "acq_rel" decays to "acquire" if (*memOrder == mlir::omp::ClauseMemoryOrderKind::Acq_rel) - return mlir::omp::ClauseMemoryOrderKind::Acquire;; + return mlir::omp::ClauseMemoryOrderKind::Acquire; } else if (action == Analysis::Write) { // "acq_rel" decays to "release" if (*memOrder == mlir::omp::ClauseMemoryOrderKind::Acq_rel)