[Mlir-commits] [mlir] [OpenACC] Fix pattern API check failures in acc-loop-tiling pass (PR #188968)
Mehdi Amini
llvmlistbot at llvm.org
Fri Mar 27 04:15:51 PDT 2026
https://github.com/joker-eph created https://github.com/llvm/llvm-project/pull/188968
Two bugs were introduced/revealed by MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS:
1. `ACCLoopTilingImpl::matchAndRewrite` returned `success()` for loops with no tile values, triggering "pattern returned success but IR did not change". Fixed by returning `failure()` instead.
2. `moveOpsAndReplaceIVs` moved ops between blocks via `splice()` and updated operands via `replaceAllUsesInRegionWith()` without notifying the rewriter. This caused "operation fingerprint changed" errors since the moved ops' parent op and operands changed without `startOpModification`/ `finalizeOpModification`. Fixed by wrapping all moved ops (and their nested ops) with rewriter modification notifications.
Assisted-by: Claude Code
>From bfa5faf46ce52a3a0e68eb12e28809a5341cc870 Mon Sep 17 00:00:00 2001
From: Mehdi Amini <joker.eph at gmail.com>
Date: Thu, 26 Mar 2026 15:57:29 -0700
Subject: [PATCH] [OpenACC] Fix pattern API check failures in acc-loop-tiling
pass
Two bugs were introduced/revealed by MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS:
1. `ACCLoopTilingImpl::matchAndRewrite` returned `success()` for loops
with no tile values, triggering "pattern returned success but IR did
not change". Fixed by returning `failure()` instead.
2. `moveOpsAndReplaceIVs` moved ops between blocks via `splice()` and
updated operands via `replaceAllUsesInRegionWith()` without notifying
the rewriter. This caused "operation fingerprint changed" errors since
the moved ops' parent op and operands changed without `startOpModification`/
`finalizeOpModification`. Fixed by wrapping all moved ops (and their
nested ops) with rewriter modification notifications.
Assisted-by: Claude Code
Fix a failure present with MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS=ON.
---
.../OpenACC/Transforms/ACCLoopTiling.cpp | 2 +-
.../OpenACC/Utils/OpenACCUtilsTiling.cpp | 21 +++++++-
.../test/Dialect/OpenACC/acc-loop-tiling.mlir | 51 +++++++++++++++++++
3 files changed, 71 insertions(+), 3 deletions(-)
diff --git a/mlir/lib/Dialect/OpenACC/Transforms/ACCLoopTiling.cpp b/mlir/lib/Dialect/OpenACC/Transforms/ACCLoopTiling.cpp
index 495d0247d86d3..6bc95ca896f37 100644
--- a/mlir/lib/Dialect/OpenACC/Transforms/ACCLoopTiling.cpp
+++ b/mlir/lib/Dialect/OpenACC/Transforms/ACCLoopTiling.cpp
@@ -158,7 +158,7 @@ struct ACCLoopTilingImpl : public OpRewritePattern<acc::LoopOp> {
PatternRewriter &rewriter) const override {
if (origLoop.getTileValues().empty())
- return success();
+ return failure();
SmallVector<Value> tileSizes(origLoop.getTileValues().begin(),
origLoop.getTileValues().end());
diff --git a/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp b/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
index c39bd06c81cbf..dddaebdee5ebc 100644
--- a/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
+++ b/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
@@ -121,15 +121,32 @@ static void moveOpsAndReplaceIVs(mlir::acc::LoopOp sourceLoop,
llvm::ArrayRef<mlir::Value> newIVs,
llvm::ArrayRef<mlir::Value> origIVs,
size_t nOps, mlir::RewriterBase &rewriter) {
- // Move ops from source to target loop [begin, begin + nOps - 1)
+ // nOps includes the terminator; move all ops except the terminator:
+ // [begin, begin + nOps - 1)
mlir::Block::iterator begin = sourceLoop.getBody().begin();
+ mlir::Block::iterator end = std::next(begin, nOps - 1);
+
+ // Notify the rewriter about all ops being moved (and their nested ops).
+ // Directly moved ops have their parent block changed (rewriter fingerprint
+ // tracking invalidated). Nested ops may have operands replaced by
+ // replaceAllUsesInRegionWith below.
+ llvm::SmallVector<mlir::Operation *> movedOps;
+ for (mlir::Block::iterator it = begin; it != end; ++it)
+ it->walk([&](mlir::Operation *op) {
+ movedOps.push_back(op);
+ rewriter.startOpModification(op);
+ });
+
targetLoop.getBody().getOperations().splice(
targetLoop.getBody().getOperations().begin(),
- sourceLoop.getBody().getOperations(), begin, std::next(begin, nOps - 1));
+ sourceLoop.getBody().getOperations(), begin, end);
// Replace uses of origIV with newIV
for (auto [i, newIV] : llvm::enumerate(newIVs))
mlir::replaceAllUsesInRegionWith(origIVs[i], newIV, targetLoop.getRegion());
+
+ for (mlir::Operation *op : movedOps)
+ rewriter.finalizeOpModification(op);
}
mlir::acc::LoopOp
diff --git a/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir b/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
index f4a46186b118d..cd13b3d8427f0 100644
--- a/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
+++ b/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
@@ -102,3 +102,54 @@ func.func @unknown_tile_size(%arg0: memref<1000xf32>) {
} attributes {independent = [#acc.device_type<none>]}
return
}
+
+// Test loop with no tile values: pattern should not apply and loop is unchanged.
+
+// CHECK-LABEL: func.func @no_tile_values
+// CHECK: acc.loop control(%{{.*}} : index) = (%{{.*}} : index) to (%{{.*}} : index) step (%{{.*}} : index) {
+// CHECK-NOT: acc.loop
+// CHECK: acc.yield
+// CHECK: }
+func.func @no_tile_values() {
+ %c0 = arith.constant 0 : index
+ %c10 = arith.constant 10 : index
+ %c1 = arith.constant 1 : index
+ acc.loop control(%i : index) = (%c0 : index) to (%c10 : index) step (%c1 : index) {
+ acc.yield
+ } attributes {independent = [#acc.device_type<none>]}
+ return
+}
+
+// Test loop tiling when the body contains ops with nested regions.
+// Exercises the walk() in moveOpsAndReplaceIVs that must notify the rewriter
+// about nested ops (required by MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS).
+
+// CHECK-LABEL: func.func @body_with_nested_region
+// CHECK: acc.loop control(%[[TILE_IV:.*]] : index) = ({{.*}}) to ({{.*}}) step ({{.*}}) {
+// CHECK: acc.loop control(%[[ELEM_IV:.*]] : index) = ({{.*}}) to ({{.*}}) step ({{.*}}) {
+// CHECK: scf.if
+// CHECK: arith.index_castui %[[ELEM_IV]]
+// CHECK: acc.yield
+// CHECK: }
+// CHECK: acc.yield
+// CHECK: }
+func.func @body_with_nested_region(%arg0: memref<10xi32>) {
+ %c0 = arith.constant 0 : index
+ %c10 = arith.constant 10 : index
+ %c1 = arith.constant 1 : index
+ %c2 = arith.constant 2 : index
+ acc.loop tile({%c2 : index}) control(%i : index) = (%c0 : index) to (%c10 : index) step (%c1 : index) {
+ %threshold = arith.constant 5 : index
+ %cond = arith.cmpi ult, %i, %threshold : index
+ %val = scf.if %cond -> (i32) {
+ %cast = arith.index_castui %i : index to i32
+ scf.yield %cast : i32
+ } else {
+ %c99 = arith.constant 99 : i32
+ scf.yield %c99 : i32
+ }
+ memref.store %val, %arg0[%i] : memref<10xi32>
+ acc.yield
+ } attributes {independent = [#acc.device_type<none>]}
+ return
+}
More information about the Mlir-commits
mailing list