[Mlir-commits] [mlir] 91c0ba6 - [OpenACC] Fix pattern API check failures in acc-loop-tiling pass (#188968)

[llvmlistbot at llvm.org]

Author: Mehdi Amini
Date: 2026-04-13T12:20:55+02:00
New Revision: 91c0ba6de8e742fc92bf166f508157c6fe8b9df4

URL: https://github.com/llvm/llvm-project/commit/91c0ba6de8e742fc92bf166f508157c6fe8b9df4
DIFF: https://github.com/llvm/llvm-project/commit/91c0ba6de8e742fc92bf166f508157c6fe8b9df4.diff

LOG: [OpenACC] Fix pattern API check failures in acc-loop-tiling pass (#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

Added: 
    

Modified: 
    mlir/lib/Dialect/OpenACC/Transforms/ACCLoopTiling.cpp
    mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
    mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir

Removed: 
    


################################################################################
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
+}