[flang-commits] [flang] [mlir] [flang][acc] Remove dangling operands in OpenACCUtilsTiling (PR #208261)
Susan Tan ス-ザン タン via flang-commits
flang-commits at lists.llvm.org
Wed Jul 8 12:59:30 PDT 2026
https://github.com/SusanTan updated https://github.com/llvm/llvm-project/pull/208261
>From 7428e41773155ef0216861f8f1166e15a3cfa167 Mon Sep 17 00:00:00 2001
From: Susan Tan <zujunt at nvidia.com>
Date: Wed, 8 Jul 2026 09:39:14 -0700
Subject: [PATCH 1/4] add fix
---
mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp b/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
index dddaebdee5ebc..6452d7e0ed8c8 100644
--- a/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
+++ b/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
@@ -97,6 +97,13 @@ createInnerLoop(mlir::acc::LoopOp inputLoop, mlir::RewriterBase &rewriter,
elementLoop.removeGangOperandsArgTypeAttr();
elementLoop.removeGangOperandsSegmentsAttr();
elementLoop.removeGangOperandsDeviceTypeAttr();
+ // Also drop the operand values themselves so that elementLoop does not
+ // end up with a non-empty gang operand list but no corresponding
+ // device-type/segment/arg-type attributes. Leaving stale operands behind
+ // makes elementLoop look like it still has gang operands to later
+ // queries (e.g. LoopOp::getGangValue), which then dereference the
+ // now-missing device-type attribute.
+ elementLoop.getGangOperandsMutable().clear();
}
if (inputLoop.hasVector() || inputLoop.getVectorValue()) {
elementLoop.removeWorkerAttr();
>From 02340ce1bce6f42f8c41d295909f3c48fe8a9caf Mon Sep 17 00:00:00 2001
From: Susan Tan <zujunt at nvidia.com>
Date: Wed, 8 Jul 2026 10:05:40 -0700
Subject: [PATCH 2/4] add test
---
.../test/Dialect/OpenACC/acc-loop-tiling.mlir | 43 +++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir b/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
index cd13b3d8427f0..3b032959b1cc6 100644
--- a/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
+++ b/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
@@ -74,6 +74,49 @@ func.func @nested_loop_tile(%arg0: memref<100x50xf32>) {
return
}
+// Regression test: a loop with GANG(STATIC: N) combined with a multi-dim
+// TILE clause used to crash with an assertion failure inside
+// LoopOp::getGangValue() because uncollapseLoops() (needed here since the
+// tile count exceeds the implicit collapse count of 1) left one of the
+// generated inner loops with a leftover gang operand but no corresponding
+// gang device-type attribute. Check that the pass completes and that gang
+// is only preserved on the outermost tile loop.
+
+// CHECK-LABEL: func.func @gang_static_with_multi_dim_tile
+// CHECK: acc.loop gang({static=%{{.*}} : i32}) control(%[[I:.*]] : index) = ({{.*}}) to ({{.*}}) step ({{.*}}) {
+// CHECK-NOT: gang
+// CHECK: acc.loop control(%[[J:.*]] : index) = ({{.*}}) to ({{.*}}) step ({{.*}}) {
+// CHECK-NOT: gang
+// CHECK: acc.loop control({{.*}} : index) = (%[[I]] : index) to ({{.*}}) step ({{.*}}) {
+// CHECK-NOT: gang
+// CHECK: acc.loop control({{.*}} : index) = (%[[J]] : index) to ({{.*}}) step ({{.*}}) {
+// CHECK-NOT: gang
+// CHECK: acc.yield
+// CHECK: }
+// CHECK: acc.yield
+// CHECK: }
+// CHECK: acc.yield
+// CHECK: }
+// CHECK: acc.yield
+// CHECK: }
+func.func @gang_static_with_multi_dim_tile(%arg0: memref<100x50xf32>) {
+ %c0 = arith.constant 0 : index
+ %c100 = arith.constant 100 : index
+ %c50 = arith.constant 50 : index
+ %c1 = arith.constant 1 : index
+ %c4 = arith.constant 4 : index
+ %c8 = arith.constant 8 : index
+ %cs = arith.constant 1 : i32
+ acc.loop gang({static=%cs : i32}) tile({%c4 : index, %c8 : index}) control(%i : index, %j : index) = (%c0, %c0 : index, index) to (%c100, %c50 : index, index) step (%c1, %c1 : index, index) {
+ %sum = arith.addi %i, %j : index
+ %val = arith.index_castui %sum : index to i32
+ %fval = arith.sitofp %val : i32 to f32
+ memref.store %fval, %arg0[%i, %j] : memref<100x50xf32>
+ acc.yield
+ } attributes {independent = [#acc.device_type<none>]}
+ return
+}
+
// Test unknown tile size (*) represented as -1
// Should use default tile size (32)
>From 7a79628faffedbc0f8e3a47bd1d9940a7cad4190 Mon Sep 17 00:00:00 2001
From: Susan Tan <zujunt at nvidia.com>
Date: Wed, 8 Jul 2026 10:48:30 -0700
Subject: [PATCH 3/4] remove worker value operands as well
---
.../OpenACC/Utils/OpenACCUtilsTiling.cpp | 3 ++
.../test/Dialect/OpenACC/acc-loop-tiling.mlir | 49 +++++++++++++++++++
2 files changed, 52 insertions(+)
diff --git a/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp b/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
index 6452d7e0ed8c8..09e83af2bc686 100644
--- a/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
+++ b/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
@@ -108,6 +108,9 @@ createInnerLoop(mlir::acc::LoopOp inputLoop, mlir::RewriterBase &rewriter,
if (inputLoop.hasVector() || inputLoop.getVectorValue()) {
elementLoop.removeWorkerAttr();
elementLoop.removeWorkerNumOperandsDeviceTypeAttr();
+ // As above for gang, also drop the worker operand values so elementLoop
+ // does not keep a dangling worker operand with no device-type attribute.
+ elementLoop.getWorkerNumOperandsMutable().clear();
}
rewriter.finalizeOpModification(elementLoop);
diff --git a/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir b/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
index 3b032959b1cc6..a1f80ad9defdf 100644
--- a/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
+++ b/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
@@ -117,6 +117,55 @@ func.func @gang_static_with_multi_dim_tile(%arg0: memref<100x50xf32>) {
return
}
+// Regression test: a loop with WORKER(N) combined with VECTOR and a
+// multi-dim TILE clause used to produce invalid IR because createInnerLoop()
+// (via uncollapseLoops()) and removeWorkerVectorFromLoop() removed the
+// worker attributes from generated inner/tile loops without also clearing
+// the worker operand value copied onto them. This left loops with a
+// non-empty worker operand list but no worker device-type attribute,
+// which the verifier rejects ('worker operands count must match worker
+// device_type count'). Check that the pass produces valid IR, with worker
+// only preserved on the outermost tile loop and vector only on the
+// outermost element loop.
+
+// CHECK-LABEL: func.func @worker_num_with_multi_dim_tile
+// CHECK: acc.loop worker(%{{.*}} : i32) control(%[[I:.*]] : index) = ({{.*}}) to ({{.*}}) step ({{.*}}) {
+// CHECK-NOT: worker
+// CHECK-NOT: vector
+// CHECK: acc.loop control(%[[J:.*]] : index) = ({{.*}}) to ({{.*}}) step ({{.*}}) {
+// CHECK-NOT: worker
+// CHECK-NOT: vector
+// CHECK: acc.loop vector control({{.*}} : index) = (%[[I]] : index) to ({{.*}}) step ({{.*}}) {
+// CHECK-NOT: worker
+// CHECK: acc.loop control({{.*}} : index) = (%[[J]] : index) to ({{.*}}) step ({{.*}}) {
+// CHECK-NOT: worker
+// CHECK-NOT: vector
+// CHECK: acc.yield
+// CHECK: }
+// CHECK: acc.yield
+// CHECK: }
+// CHECK: acc.yield
+// CHECK: }
+// CHECK: acc.yield
+// CHECK: }
+func.func @worker_num_with_multi_dim_tile(%arg0: memref<100x50xf32>) {
+ %c0 = arith.constant 0 : index
+ %c100 = arith.constant 100 : index
+ %c50 = arith.constant 50 : index
+ %c1 = arith.constant 1 : index
+ %c4 = arith.constant 4 : index
+ %c8 = arith.constant 8 : index
+ %cw = arith.constant 4 : i32
+ acc.loop worker(%cw : i32) vector tile({%c4 : index, %c8 : index}) control(%i : index, %j : index) = (%c0, %c0 : index, index) to (%c100, %c50 : index, index) step (%c1, %c1 : index, index) {
+ %sum = arith.addi %i, %j : index
+ %val = arith.index_castui %sum : index to i32
+ %fval = arith.sitofp %val : i32 to f32
+ memref.store %fval, %arg0[%i, %j] : memref<100x50xf32>
+ acc.yield
+ } attributes {independent = [#acc.device_type<none>]}
+ return
+}
+
// Test unknown tile size (*) represented as -1
// Should use default tile size (32)
>From 5d65b3deddac5758cb2e687b8b721beac7080478 Mon Sep 17 00:00:00 2001
From: Susan Tan <zujunt at nvidia.com>
Date: Wed, 8 Jul 2026 12:59:17 -0700
Subject: [PATCH 4/4] change to drop dummy of original
---
.../OpenACC/Support/FIROpenACCOpsInterfaces.h | 14 ++++++++++++
.../Support/FIROpenACCOpsInterfaces.cpp | 20 +++++++++++++++++
.../Support/RegisterOpenACCExtensions.cpp | 10 +++++++++
.../Dialect/OpenACC/OpenACCOpsInterfaces.td | 22 +++++++++++++++++++
.../OffloadLiveInValueCanonicalization.cpp | 15 +++++++++++++
5 files changed, 81 insertions(+)
diff --git a/flang/include/flang/Optimizer/OpenACC/Support/FIROpenACCOpsInterfaces.h b/flang/include/flang/Optimizer/OpenACC/Support/FIROpenACCOpsInterfaces.h
index a39d517412c07..31e1593e11325 100644
--- a/flang/include/flang/Optimizer/OpenACC/Support/FIROpenACCOpsInterfaces.h
+++ b/flang/include/flang/Optimizer/OpenACC/Support/FIROpenACCOpsInterfaces.h
@@ -99,6 +99,20 @@ struct OutlineRematerializationModel<fir::ConvertOp>
bool isRematerializationCandidate(mlir::Operation *op) const;
};
+/// External model for OutlineIdentityOperandOpInterface.
+/// Drops the dummy_scope operand (an identity token that must not be
+/// duplicated) from a [hl]fir.declare instance that has been sunk or
+/// rematerialized into an offload region. Alias analysis falls back to a
+/// dominance-based lookup of the enclosing dummy scope when this operand is
+/// absent, so correctness is preserved without needing to keep or clone the
+/// original operand.
+template <typename Op>
+struct OutlineIdentityOperandDeclareModel
+ : public mlir::acc::OutlineIdentityOperandOpInterface::ExternalModel<
+ OutlineIdentityOperandDeclareModel<Op>, Op> {
+ void dropOutlinedIdentityOperands(mlir::Operation *op) const;
+};
+
/// External model for OffloadRegionOpInterface.
/// This interface marks operations whose regions are targets for offloading
/// and outlining.
diff --git a/flang/lib/Optimizer/OpenACC/Support/FIROpenACCOpsInterfaces.cpp b/flang/lib/Optimizer/OpenACC/Support/FIROpenACCOpsInterfaces.cpp
index 590619a31a33f..8e4b55fe5f4a8 100644
--- a/flang/lib/Optimizer/OpenACC/Support/FIROpenACCOpsInterfaces.cpp
+++ b/flang/lib/Optimizer/OpenACC/Support/FIROpenACCOpsInterfaces.cpp
@@ -143,6 +143,26 @@ bool OutlineRematerializationModel<
fir::ConvertOp::isIntegerCompatible(outTy);
}
+template <>
+void OutlineIdentityOperandDeclareModel<
+ fir::DeclareOp>::dropOutlinedIdentityOperands(mlir::Operation *op) const {
+ auto declareOp = mlir::cast<fir::DeclareOp>(op);
+ if (declareOp.getDummyScope()) {
+ declareOp.getDummyScopeMutable().clear();
+ declareOp->removeAttr(declareOp.getDummyArgNoAttrName());
+ }
+}
+
+template <>
+void OutlineIdentityOperandDeclareModel<
+ hlfir::DeclareOp>::dropOutlinedIdentityOperands(mlir::Operation *op) const {
+ auto declareOp = mlir::cast<hlfir::DeclareOp>(op);
+ if (declareOp.getDummyScope()) {
+ declareOp.getDummyScopeMutable().clear();
+ declareOp->removeAttr(declareOp.getDummyArgNoAttrName());
+ }
+}
+
// Helper to recursively process address-of operations in derived type
// descriptors and collect all needed fir.globals.
static void processAddrOfOpInDerivedTypeDescriptor(
diff --git a/flang/lib/Optimizer/OpenACC/Support/RegisterOpenACCExtensions.cpp b/flang/lib/Optimizer/OpenACC/Support/RegisterOpenACCExtensions.cpp
index 070208d22ba05..28fbbc7b5444c 100644
--- a/flang/lib/Optimizer/OpenACC/Support/RegisterOpenACCExtensions.cpp
+++ b/flang/lib/Optimizer/OpenACC/Support/RegisterOpenACCExtensions.cpp
@@ -53,6 +53,12 @@ void registerOpenACCExtensions(mlir::DialectRegistry ®istry) {
PartialEntityAccessModel<fir::CoordinateOp>>(*ctx);
fir::DeclareOp::attachInterface<PartialEntityAccessModel<fir::DeclareOp>>(
*ctx);
+ // fir.declare's dummy_scope operand is an identity token (see
+ // fir.dummy_scope) that must not be duplicated when the declare is
+ // sunk/rematerialized into an offload region; it must instead be
+ // dropped from the sunk/cloned instance.
+ fir::DeclareOp::attachInterface<
+ OutlineIdentityOperandDeclareModel<fir::DeclareOp>>(*ctx);
fir::AddrOfOp::attachInterface<AddressOfGlobalModel>(*ctx);
fir::GlobalOp::attachInterface<GlobalVariableModel>(*ctx);
@@ -94,6 +100,10 @@ void registerOpenACCExtensions(mlir::DialectRegistry ®istry) {
PartialEntityAccessModel<hlfir::DesignateOp>>(*ctx);
hlfir::DeclareOp::attachInterface<
PartialEntityAccessModel<hlfir::DeclareOp>>(*ctx);
+ // See the fir::DeclareOp registration above for why dummy_scope must
+ // be dropped rather than duplicated.
+ hlfir::DeclareOp::attachInterface<
+ OutlineIdentityOperandDeclareModel<hlfir::DeclareOp>>(*ctx);
});
// Register CUF operation interfaces
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOpsInterfaces.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOpsInterfaces.td
index a42bb7a66aae7..be7b5950eddfc 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOpsInterfaces.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOpsInterfaces.td
@@ -45,6 +45,28 @@ def PartialEntityAccessOpInterface : OpInterface<"PartialEntityAccessOpInterface
];
}
+def OutlineIdentityOperandOpInterface
+ : OpInterface<"OutlineIdentityOperandOpInterface"> {
+ let cppNamespace = "::mlir::acc";
+
+ let description = [{
+ An interface for operations with an "identity" operand (e.g. FIR's
+ dummy scope handle) that must never be duplicated, unlike operands
+ handled by `OutlineRematerializationOpInterface`. When such an
+ operation is sunk/cloned into an offload region and the operand would
+ otherwise reference a value outside it, drop the operand instead of
+ cloning it; implementations must preserve correctness some other way
+ (e.g. FIR's dominance-based scope lookup fallback).
+ }];
+
+ let methods = [
+ InterfaceMethod<"Drop any identity operands from this operation "
+ "instance that reference values defined outside its "
+ "containing region.",
+ "void", "dropOutlinedIdentityOperands", (ins), [{}]>,
+ ];
+}
+
def AddressOfGlobalOpInterface : OpInterface<"AddressOfGlobalOpInterface"> {
let cppNamespace = "::mlir::acc";
diff --git a/mlir/lib/Dialect/OpenACC/Transforms/OffloadLiveInValueCanonicalization.cpp b/mlir/lib/Dialect/OpenACC/Transforms/OffloadLiveInValueCanonicalization.cpp
index 60285c8a02bb9..4b57354b8b5d8 100644
--- a/mlir/lib/Dialect/OpenACC/Transforms/OffloadLiveInValueCanonicalization.cpp
+++ b/mlir/lib/Dialect/OpenACC/Transforms/OffloadLiveInValueCanonicalization.cpp
@@ -246,6 +246,13 @@ class OffloadLiveInValueCanonicalization
Operation *sinkOp = sinkCandidate.getDefiningOp();
assert(sinkOp && "must have op to be considered");
sinkOp->moveBefore(®ion.front().front());
+ // Some operations (e.g. fir.declare) may carry identity operands that
+ // must not be left referencing a value defined outside the region.
+ // Unlike other operands, such identity operands cannot simply be
+ // treated as new live-ins to legalize; they must be dropped instead.
+ if (auto identityOp =
+ dyn_cast<acc::OutlineIdentityOperandOpInterface>(sinkOp))
+ identityOp.dropOutlinedIdentityOperands();
LLVM_DEBUG(llvm::dbgs() << "\t\tSunk: " << *sinkOp << "\n");
}
@@ -262,6 +269,14 @@ class OffloadLiveInValueCanonicalization
computeTopologicalSorting(opsToRematerialize);
for (Operation *rematerializationOp : opsToRematerialize) {
Operation *clonedOp = builder.clone(*rematerializationOp);
+ // See the comment above: identity operands must be dropped from the
+ // clone rather than duplicated, since duplicating them would make the
+ // clone appear to belong to a different instantiation than the
+ // original operation (and any of its siblings left outside the
+ // region).
+ if (auto identityOp =
+ dyn_cast<acc::OutlineIdentityOperandOpInterface>(clonedOp))
+ identityOp.dropOutlinedIdentityOperands();
for (auto [oldResult, newResult] : llvm::zip(
rematerializationOp->getResults(), clonedOp->getResults())) {
replaceAllUsesInRegionWith(oldResult, newResult, region);
More information about the flang-commits
mailing list