From a9f7208fed2fee89c7b4c6146531a9314b2b5880 Mon Sep 17 00:00:00 2001 From: Krish Gupta Date: Fri, 10 Oct 2025 19:10:05 +0530 Subject: [PATCH 1/3] [Flang][OpenMP] Fix overly restrictive atomic capture validation OpenMP atomic capture allows four operation pairs in any order: READ+UPDATE, UPDATE+READ, READ+WRITE, UPDATE+WRITE. Previous code only allowed READ+UPDATE and UPDATE+WRITE, causing CI failures on existing tests. Fixes #161934 --- flang/lib/Lower/OpenMP/Atomic.cpp | 19 +++++++++++++ .../atomic-capture-derived-array-diag.f90 | 27 +++++++++++++++++++ .../OpenMP/atomic-capture-same-element.f90 | 21 +++++++++++++++ 3 files changed, 67 insertions(+) create mode 100644 flang/test/Lower/OpenMP/atomic-capture-derived-array-diag.f90 create mode 100644 flang/test/Lower/OpenMP/atomic-capture-same-element.f90 diff --git a/flang/lib/Lower/OpenMP/Atomic.cpp b/flang/lib/Lower/OpenMP/Atomic.cpp index ff82a36951bfa..ca1d7b34857f7 100644 --- a/flang/lib/Lower/OpenMP/Atomic.cpp +++ b/flang/lib/Lower/OpenMP/Atomic.cpp @@ -19,6 +19,7 @@ #include "flang/Lower/SymbolMap.h" #include "flang/Optimizer/Builder/FIRBuilder.h" #include "flang/Optimizer/Builder/Todo.h" +#include "flang/Optimizer/Dialect/FIRType.h" #include "flang/Parser/parse-tree.h" #include "flang/Semantics/semantics.h" #include "flang/Semantics/type.h" @@ -459,6 +460,24 @@ void Fortran::lower::omp::lowerAtomic( } else { int action0 = analysis.op0.what & analysis.Action; int action1 = analysis.op1.what & analysis.Action; + + // Check for invalid atomic capture sequence. + // OpenMP allows: READ+UPDATE, UPDATE+READ, READ+WRITE, or UPDATE+WRITE + // in any order. Ensure we have one read-like and one write-like operation. + if (construct.IsCapture()) { + using Action = parser::OpenMPAtomicConstruct::Analysis; + const bool hasRead = (action0 == Action::Read || action1 == Action::Read); + const bool hasWriteOrUpdate = + (action0 == Action::Write || action0 == Action::Update || + action1 == Action::Write || action1 == Action::Update); + + if (!(hasRead && hasWriteOrUpdate)) { + mlir::emitError(loc, "OpenMP atomic capture requires one read and one " + "write/update operation"); + return; + } + } + mlir::Operation *captureOp = nullptr; fir::FirOpBuilder::InsertPoint preAt = builder.saveInsertionPoint(); fir::FirOpBuilder::InsertPoint atomicAt, postAt; diff --git a/flang/test/Lower/OpenMP/atomic-capture-derived-array-diag.f90 b/flang/test/Lower/OpenMP/atomic-capture-derived-array-diag.f90 new file mode 100644 index 0000000000000..f4ba5b474804b --- /dev/null +++ b/flang/test/Lower/OpenMP/atomic-capture-derived-array-diag.f90 @@ -0,0 +1,27 @@ +! Test that atomic capture with derived-type array elements fails appropriately +! RUN: not %flang_fc1 -emit-fir -fopenmp %s 2>&1 | FileCheck %s + +! This test verifies that atomic capture with derived-type component array +! elements using different indices fails during MLIR verification. The issue +! is that different array elements represent different memory locations, which +! violates OpenMP's requirement that both statements in atomic capture must +! operate on the same variable. This will fail in the MLIR verifier until +! semantic analysis is fixed to detect this earlier. + +program test_atomic_capture_invalid_sequence + type t1 + integer :: i(1) + end type + type(t1) :: t2 + integer :: j + + t2%i = 0 + j = 1 + + ! CHECK: invalid sequence of operations in the capture region + !$omp atomic capture + t2%i(j*1) = t2%i(1) + 1 + t2%i(1) = t2%i(j*1) + !$omp end atomic + +end program test_atomic_capture_invalid_sequence diff --git a/flang/test/Lower/OpenMP/atomic-capture-same-element.f90 b/flang/test/Lower/OpenMP/atomic-capture-same-element.f90 new file mode 100644 index 0000000000000..208a2db362369 --- /dev/null +++ b/flang/test/Lower/OpenMP/atomic-capture-same-element.f90 @@ -0,0 +1,21 @@ +! Test that atomic capture works correctly with same element (positive control) +! RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | FileCheck %s + +! This test verifies that atomic capture with the same array element in both +! statements works correctly and doesn't trigger the invalid sequence diagnostic. + +! CHECK-LABEL: func @_QQmain +program test_atomic_capture_same_element + integer :: a(10) + integer :: v + + a = 0 + + ! This should work - same element a(1) in both statements + ! CHECK: omp.atomic.capture + !$omp atomic capture + v = a(1) + a(1) = a(1) + 1 + !$omp end atomic + +end program test_atomic_capture_same_element From ad66daa05074219d0b94314728336296ad2c05d8 Mon Sep 17 00:00:00 2001 From: Krish Gupta Date: Tue, 14 Oct 2025 02:34:19 +0530 Subject: [PATCH 2/3] [flang][OpenMP] Remove ineffective atomic capture validation - Removed redundant lowering-layer check in Atomic.cpp per review feedback - Added semantic layer placeholder for issue #161934 (not yet implemented) - Full semantic check deferred due to complexity of Flang expression traversal The MLIR verification layer continues to catch these cases as a safety net. --- flang/lib/Lower/OpenMP/Atomic.cpp | 18 ------------- flang/lib/Semantics/check-omp-atomic.cpp | 16 +++++++++++ .../atomic-capture-derived-array-diag.f90 | 27 ------------------- .../OpenMP/atomic-capture-same-element.f90 | 21 --------------- 4 files changed, 16 insertions(+), 66 deletions(-) delete mode 100644 flang/test/Lower/OpenMP/atomic-capture-derived-array-diag.f90 delete mode 100644 flang/test/Lower/OpenMP/atomic-capture-same-element.f90 diff --git a/flang/lib/Lower/OpenMP/Atomic.cpp b/flang/lib/Lower/OpenMP/Atomic.cpp index ca1d7b34857f7..20e245b469289 100644 --- a/flang/lib/Lower/OpenMP/Atomic.cpp +++ b/flang/lib/Lower/OpenMP/Atomic.cpp @@ -19,7 +19,6 @@ #include "flang/Lower/SymbolMap.h" #include "flang/Optimizer/Builder/FIRBuilder.h" #include "flang/Optimizer/Builder/Todo.h" -#include "flang/Optimizer/Dialect/FIRType.h" #include "flang/Parser/parse-tree.h" #include "flang/Semantics/semantics.h" #include "flang/Semantics/type.h" @@ -461,23 +460,6 @@ void Fortran::lower::omp::lowerAtomic( int action0 = analysis.op0.what & analysis.Action; int action1 = analysis.op1.what & analysis.Action; - // Check for invalid atomic capture sequence. - // OpenMP allows: READ+UPDATE, UPDATE+READ, READ+WRITE, or UPDATE+WRITE - // in any order. Ensure we have one read-like and one write-like operation. - if (construct.IsCapture()) { - using Action = parser::OpenMPAtomicConstruct::Analysis; - const bool hasRead = (action0 == Action::Read || action1 == Action::Read); - const bool hasWriteOrUpdate = - (action0 == Action::Write || action0 == Action::Update || - action1 == Action::Write || action1 == Action::Update); - - if (!(hasRead && hasWriteOrUpdate)) { - mlir::emitError(loc, "OpenMP atomic capture requires one read and one " - "write/update operation"); - return; - } - } - mlir::Operation *captureOp = nullptr; fir::FirOpBuilder::InsertPoint preAt = builder.saveInsertionPoint(); fir::FirOpBuilder::InsertPoint atomicAt, postAt; diff --git a/flang/lib/Semantics/check-omp-atomic.cpp b/flang/lib/Semantics/check-omp-atomic.cpp index 351af5c099aee..1e282a35dfd68 100644 --- a/flang/lib/Semantics/check-omp-atomic.cpp +++ b/flang/lib/Semantics/check-omp-atomic.cpp @@ -18,6 +18,7 @@ #include "flang/Evaluate/match.h" #include "flang/Evaluate/rewrite.h" #include "flang/Evaluate/tools.h" +#include "flang/Evaluate/traverse.h" #include "flang/Parser/char-block.h" #include "flang/Parser/parse-tree.h" #include "flang/Semantics/openmp-utils.h" @@ -692,6 +693,15 @@ OmpStructureChecker::CheckUpdateCapture( "In ATOMIC UPDATE operation with CAPTURE neither statement could be the update or the capture"_err_en_US); }}; + auto checkSameAtomicLocation{[&](const evaluate::Assignment &upd, + const evaluate::Assignment &cap) { + // For now, this is a placeholder. A complete implementation would need + // to walk the entire expression tree of upd.rhs to find all DataRef nodes + // and compare them structurally with the atomic variable (upd.lhs). + // This is complex and requires proper expression traversal. + // The MLIR verification layer catches these cases as a safety net. + }}; + auto makeSelectionFromDet{[&](int det) -> ReturnTy { // If det != 0, then the checks unambiguously suggest a specific // categorization. @@ -701,6 +711,7 @@ OmpStructureChecker::CheckUpdateCapture( if (det > 0) { // as1 is update, as2 is capture if (isUpdateCapture(as1, as2)) { + checkSameAtomicLocation(as1, as2); return std::make_pair(/*Update=*/ec1, /*Capture=*/ec2); } else { errorCaptureShouldRead(act2.source, as1.lhs.AsFortran()); @@ -709,6 +720,7 @@ OmpStructureChecker::CheckUpdateCapture( } else if (det < 0) { // as2 is update, as1 is capture if (isUpdateCapture(as2, as1)) { + checkSameAtomicLocation(as2, as1); return std::make_pair(/*Update=*/ec2, /*Capture=*/ec1); } else { errorCaptureShouldRead(act1.source, as2.lhs.AsFortran()); @@ -722,11 +734,15 @@ OmpStructureChecker::CheckUpdateCapture( // capture, emit a warning about the ambiguity. context_.Say(act1.source, "In ATOMIC UPDATE operation with CAPTURE either statement could be the update and the capture, assuming the first one is the capture statement"_warn_en_US); + checkSameAtomicLocation(as2, as1); return std::make_pair(/*Update=*/ec2, /*Capture=*/ec1); } if (updateFirst != captureFirst) { const parser::ExecutionPartConstruct *upd{updateFirst ? ec1 : ec2}; const parser::ExecutionPartConstruct *cap{captureFirst ? ec1 : ec2}; + const evaluate::Assignment &updAssign{updateFirst ? as1 : as2}; + const evaluate::Assignment &capAssign{captureFirst ? as1 : as2}; + checkSameAtomicLocation(updAssign, capAssign); return std::make_pair(upd, cap); } assert(!updateFirst && !captureFirst); diff --git a/flang/test/Lower/OpenMP/atomic-capture-derived-array-diag.f90 b/flang/test/Lower/OpenMP/atomic-capture-derived-array-diag.f90 deleted file mode 100644 index f4ba5b474804b..0000000000000 --- a/flang/test/Lower/OpenMP/atomic-capture-derived-array-diag.f90 +++ /dev/null @@ -1,27 +0,0 @@ -! Test that atomic capture with derived-type array elements fails appropriately -! RUN: not %flang_fc1 -emit-fir -fopenmp %s 2>&1 | FileCheck %s - -! This test verifies that atomic capture with derived-type component array -! elements using different indices fails during MLIR verification. The issue -! is that different array elements represent different memory locations, which -! violates OpenMP's requirement that both statements in atomic capture must -! operate on the same variable. This will fail in the MLIR verifier until -! semantic analysis is fixed to detect this earlier. - -program test_atomic_capture_invalid_sequence - type t1 - integer :: i(1) - end type - type(t1) :: t2 - integer :: j - - t2%i = 0 - j = 1 - - ! CHECK: invalid sequence of operations in the capture region - !$omp atomic capture - t2%i(j*1) = t2%i(1) + 1 - t2%i(1) = t2%i(j*1) - !$omp end atomic - -end program test_atomic_capture_invalid_sequence diff --git a/flang/test/Lower/OpenMP/atomic-capture-same-element.f90 b/flang/test/Lower/OpenMP/atomic-capture-same-element.f90 deleted file mode 100644 index 208a2db362369..0000000000000 --- a/flang/test/Lower/OpenMP/atomic-capture-same-element.f90 +++ /dev/null @@ -1,21 +0,0 @@ -! Test that atomic capture works correctly with same element (positive control) -! RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | FileCheck %s - -! This test verifies that atomic capture with the same array element in both -! statements works correctly and doesn't trigger the invalid sequence diagnostic. - -! CHECK-LABEL: func @_QQmain -program test_atomic_capture_same_element - integer :: a(10) - integer :: v - - a = 0 - - ! This should work - same element a(1) in both statements - ! CHECK: omp.atomic.capture - !$omp atomic capture - v = a(1) - a(1) = a(1) + 1 - !$omp end atomic - -end program test_atomic_capture_same_element From 3c44c167997e5e961817da4a5820c72944f70b09 Mon Sep 17 00:00:00 2001 From: Krish Gupta Date: Tue, 14 Oct 2025 02:58:02 +0530 Subject: [PATCH 3/3] Apply clang-format and remove unrelated whitespace change --- flang/lib/Lower/OpenMP/Atomic.cpp | 1 - flang/lib/Semantics/check-omp-atomic.cpp | 16 ++++++++-------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/flang/lib/Lower/OpenMP/Atomic.cpp b/flang/lib/Lower/OpenMP/Atomic.cpp index 20e245b469289..ff82a36951bfa 100644 --- a/flang/lib/Lower/OpenMP/Atomic.cpp +++ b/flang/lib/Lower/OpenMP/Atomic.cpp @@ -459,7 +459,6 @@ void Fortran::lower::omp::lowerAtomic( } 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/lib/Semantics/check-omp-atomic.cpp b/flang/lib/Semantics/check-omp-atomic.cpp index 1e282a35dfd68..6252fb0646006 100644 --- a/flang/lib/Semantics/check-omp-atomic.cpp +++ b/flang/lib/Semantics/check-omp-atomic.cpp @@ -693,14 +693,14 @@ OmpStructureChecker::CheckUpdateCapture( "In ATOMIC UPDATE operation with CAPTURE neither statement could be the update or the capture"_err_en_US); }}; - auto checkSameAtomicLocation{[&](const evaluate::Assignment &upd, - const evaluate::Assignment &cap) { - // For now, this is a placeholder. A complete implementation would need - // to walk the entire expression tree of upd.rhs to find all DataRef nodes - // and compare them structurally with the atomic variable (upd.lhs). - // This is complex and requires proper expression traversal. - // The MLIR verification layer catches these cases as a safety net. - }}; + auto checkSameAtomicLocation{ + [&](const evaluate::Assignment &upd, const evaluate::Assignment &cap) { + // For now, this is a placeholder. A complete implementation would need + // to walk the entire expression tree of upd.rhs to find all DataRef + // nodes and compare them structurally with the atomic variable + // (upd.lhs). This is complex and requires proper expression traversal. + // The MLIR verification layer catches these cases as a safety net. + }}; auto makeSelectionFromDet{[&](int det) -> ReturnTy { // If det != 0, then the checks unambiguously suggest a specific