[MLIR] Change getBackwardSlice to return a logicalresult rather than crash #140961

wsmoses · 2025-05-21T21:07:43Z

The current implementation of getBackwardSlice will crash if an operation in the dependency chain is defined by an operation with multiple regions or blocks. Crashing is bad (and forbids many analyses from using getBackwardSlice, as well as causing existing users of getBackwardSlice to fail for IR with this property).

This PR instead causes the analysis to return a failure, rather than crash in the cases it cannot compute the full slice

llvmbot · 2025-05-21T21:08:19Z

@llvm/pr-subscribers-mlir-nvgpu
@llvm/pr-subscribers-mlir
@llvm/pr-subscribers-mlir-gpu

@llvm/pr-subscribers-mlir-linalg

Author: William Moses (wsmoses)

Changes

…crash

Full diff: https://github.com/llvm/llvm-project/pull/140961.diff

10 Files Affected:

(modified) mlir/include/mlir/Analysis/SliceAnalysis.h (+6-4)
(modified) mlir/include/mlir/Query/Matcher/SliceMatchers.h (+2-1)
(modified) mlir/lib/Analysis/SliceAnalysis.cpp (+32-20)
(modified) mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp (+3-1)
(modified) mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp (+5-2)
(modified) mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp (+4-2)
(modified) mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp (+2-1)
(modified) mlir/lib/Transforms/Utils/RegionUtils.cpp (+4-2)
(modified) mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp (+2-1)
(modified) mlir/test/lib/IR/TestSlicing.cpp (+2-1)

diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h
index 3b731e8bb1c22..192b096fd31f6 100644
--- a/mlir/include/mlir/Analysis/SliceAnalysis.h
+++ b/mlir/include/mlir/Analysis/SliceAnalysis.h
@@ -138,13 +138,15 @@ void getForwardSlice(Value root, SetVector<Operation *> *forwardSlice,
 /// Assuming all local orders match the numbering order:
 ///    {1, 2, 5, 3, 4, 6}
 ///
-void getBackwardSlice(Operation *op, SetVector<Operation *> *backwardSlice,
-                      const BackwardSliceOptions &options = {});
+LogicalResult getBackwardSlice(Operation *op,
+                               SetVector<Operation *> *backwardSlice,
+                               const BackwardSliceOptions &options = {});
 
 /// Value-rooted version of `getBackwardSlice`. Return the union of all backward
 /// slices for the op defining or owning the value `root`.
-void getBackwardSlice(Value root, SetVector<Operation *> *backwardSlice,
-                      const BackwardSliceOptions &options = {});
+LogicalResult getBackwardSlice(Value root,
+                               SetVector<Operation *> *backwardSlice,
+                               const BackwardSliceOptions &options = {});
 
 /// Iteratively computes backward slices and forward slices until
 /// a fixed point is reached. Returns an `SetVector<Operation *>` which
diff --git a/mlir/include/mlir/Query/Matcher/SliceMatchers.h b/mlir/include/mlir/Query/Matcher/SliceMatchers.h
index 1b0e4c32dbe94..6b7d10ccaed64 100644
--- a/mlir/include/mlir/Query/Matcher/SliceMatchers.h
+++ b/mlir/include/mlir/Query/Matcher/SliceMatchers.h
@@ -112,7 +112,8 @@ bool BackwardSliceMatcher<Matcher>::matches(
     }
     return true;
   };
-  getBackwardSlice(rootOp, &backwardSlice, options);
+  auto result = getBackwardSlice(rootOp, &backwardSlice, options);
+  assert(result.succeeded());
   return options.inclusive ? backwardSlice.size() > 1
                            : backwardSlice.size() >= 1;
 }
diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp
index 5aebb19e3a86e..5992aceba82d3 100644
--- a/mlir/lib/Analysis/SliceAnalysis.cpp
+++ b/mlir/lib/Analysis/SliceAnalysis.cpp
@@ -80,22 +80,25 @@ void mlir::getForwardSlice(Value root, SetVector<Operation *> *forwardSlice,
   forwardSlice->insert(v.rbegin(), v.rend());
 }
 
-static void getBackwardSliceImpl(Operation *op,
-                                 SetVector<Operation *> *backwardSlice,
-                                 const BackwardSliceOptions &options) {
+static LogicalResult getBackwardSliceImpl(Operation *op,
+                                          SetVector<Operation *> *backwardSlice,
+                                          const BackwardSliceOptions &options) {
   if (!op || op->hasTrait<OpTrait::IsIsolatedFromAbove>())
-    return;
+    return success();
 
   // Evaluate whether we should keep this def.
   // This is useful in particular to implement scoping; i.e. return the
   // transitive backwardSlice in the current scope.
   if (options.filter && !options.filter(op))
-    return;
+    return success();
+
+  bool succeeded = true;
 
   auto processValue = [&](Value value) {
     if (auto *definingOp = value.getDefiningOp()) {
       if (backwardSlice->count(definingOp) == 0)
-        getBackwardSliceImpl(definingOp, backwardSlice, options);
+        succeeded &= getBackwardSliceImpl(definingOp, backwardSlice, options)
+                         .succeeded();
     } else if (auto blockArg = dyn_cast<BlockArgument>(value)) {
       if (options.omitBlockArguments)
         return;
@@ -106,9 +109,13 @@ static void getBackwardSliceImpl(Operation *op,
       // blocks of parentOp, which are not technically backward unless they flow
       // into us. For now, just bail.
       if (parentOp && backwardSlice->count(parentOp) == 0) {
-        assert(parentOp->getNumRegions() == 1 &&
-               llvm::hasSingleElement(parentOp->getRegion(0).getBlocks()));
-        getBackwardSliceImpl(parentOp, backwardSlice, options);
+        if (parentOp->getNumRegions() == 1 &&
+            llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())) {
+          succeeded &= getBackwardSliceImpl(parentOp, backwardSlice, options)
+                           .succeeded();
+        } else {
+          succeeded = false;
+        }
       }
     } else {
       llvm_unreachable("No definingOp and not a block argument.");
@@ -133,28 +140,30 @@ static void getBackwardSliceImpl(Operation *op,
   llvm::for_each(op->getOperands(), processValue);
 
   backwardSlice->insert(op);
+  return succeess(succeeded);
 }
 
-void mlir::getBackwardSlice(Operation *op,
-                            SetVector<Operation *> *backwardSlice,
-                            const BackwardSliceOptions &options) {
-  getBackwardSliceImpl(op, backwardSlice, options);
+LogicalResult result
+mlir::getBackwardSlice(Operation *op, SetVector<Operation *> *backwardSlice,
+                       const BackwardSliceOptions &options) {
+  LogicalResult result = getBackwardSliceImpl(op, backwardSlice, options);
 
   if (!options.inclusive) {
     // Don't insert the top level operation, we just queried on it and don't
     // want it in the results.
     backwardSlice->remove(op);
   }
+  return result;
 }
 
-void mlir::getBackwardSlice(Value root, SetVector<Operation *> *backwardSlice,
-                            const BackwardSliceOptions &options) {
+LogicalResult mlir::getBackwardSlice(Value root,
+                                     SetVector<Operation *> *backwardSlice,
+                                     const BackwardSliceOptions &options) {
   if (Operation *definingOp = root.getDefiningOp()) {
-    getBackwardSlice(definingOp, backwardSlice, options);
-    return;
+    return getBackwardSlice(definingOp, backwardSlice, options);
   }
   Operation *bbAargOwner = cast<BlockArgument>(root).getOwner()->getParentOp();
-  getBackwardSlice(bbAargOwner, backwardSlice, options);
+  return getBackwardSlice(bbAargOwner, backwardSlice, options);
 }
 
 SetVector<Operation *>
@@ -170,7 +179,9 @@ mlir::getSlice(Operation *op, const BackwardSliceOptions &backwardSliceOptions,
     auto *currentOp = (slice)[currentIndex];
     // Compute and insert the backwardSlice starting from currentOp.
     backwardSlice.clear();
-    getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions);
+    auto result =
+        getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions);
+    assert(result.succeeded());
     slice.insert_range(backwardSlice);
 
     // Compute and insert the forwardSlice starting from currentOp.
@@ -193,7 +204,8 @@ static bool dependsOnCarriedVals(Value value,
   sliceOptions.filter = [&](Operation *op) {
     return !ancestorOp->isAncestor(op);
   };
-  getBackwardSlice(value, &slice, sliceOptions);
+  auto result = getBackwardSlice(value, &slice, sliceOptions);
+  assert(result.succeeded());
 
   // Check that none of the operands of the operations in the backward slice are
   // loop iteration arguments, and neither is the value itself.
diff --git a/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp b/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp
index 8b16da387457d..22df878cfe18d 100644
--- a/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp
+++ b/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp
@@ -317,7 +317,9 @@ getSliceContract(Operation *op,
     auto *currentOp = (slice)[currentIndex];
     // Compute and insert the backwardSlice starting from currentOp.
     backwardSlice.clear();
-    getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions);
+    auto result =
+        getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions);
+    assert(result.succeeded());
     slice.insert_range(backwardSlice);
 
     // Compute and insert the forwardSlice starting from currentOp.
diff --git a/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp b/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp
index d33a17af63459..ee0d44be60b59 100644
--- a/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp
+++ b/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp
@@ -124,10 +124,13 @@ static void computeBackwardSlice(tensor::PadOp padOp,
   getUsedValuesDefinedAbove(padOp.getRegion(), padOp.getRegion(),
                             valuesDefinedAbove);
   for (Value v : valuesDefinedAbove) {
-    getBackwardSlice(v, &backwardSlice, sliceOptions);
+    auto result = getBackwardSlice(v, &backwardSlice, sliceOptions);
+    assert(result.succeeded());
   }
   // Then, add the backward slice from padOp itself.
-  getBackwardSlice(padOp.getOperation(), &backwardSlice, sliceOptions);
+  auto result =
+      getBackwardSlice(padOp.getOperation(), &backwardSlice, sliceOptions);
+  assert(result.succeeded());
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp b/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp
index 75dbe0becf80d..c5b8117283b25 100644
--- a/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp
+++ b/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp
@@ -290,8 +290,10 @@ static void getPipelineStages(
   });
   options.inclusive = true;
   for (Operation &op : forOp.getBody()->getOperations()) {
-    if (stage0Ops.contains(&op))
-      getBackwardSlice(&op, &dependencies, options);
+    if (stage0Ops.contains(&op)) {
+      auto result = getBackwardSlice(&op, &dependencies, options);
+      assert(result.succeeded());
+    }
   }
 
   for (Operation &op : forOp.getBody()->getOperations()) {
diff --git a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
index 719e2c6fa459e..273fdbfbc4ffe 100644
--- a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
+++ b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
@@ -1772,7 +1772,8 @@ checkAssumptionForLoop(Operation *loopOp, Operation *consumerOp,
   };
   llvm::SetVector<Operation *> slice;
   for (auto operand : consumerOp->getOperands()) {
-    getBackwardSlice(operand, &slice, options);
+    auto result = getBackwardSlice(operand, &slice, options);
+    assert(result.succeeded());
   }
 
   if (!slice.empty()) {
diff --git a/mlir/lib/Transforms/Utils/RegionUtils.cpp b/mlir/lib/Transforms/Utils/RegionUtils.cpp
index 4985d718c1780..70ba5e321db6d 100644
--- a/mlir/lib/Transforms/Utils/RegionUtils.cpp
+++ b/mlir/lib/Transforms/Utils/RegionUtils.cpp
@@ -1094,7 +1094,8 @@ LogicalResult mlir::moveOperationDependencies(RewriterBase &rewriter,
     return !dominance.properlyDominates(sliceBoundaryOp, insertionPoint);
   };
   llvm::SetVector<Operation *> slice;
-  getBackwardSlice(op, &slice, options);
+  auto result = getBackwardSlice(op, &slice, options);
+  assert(result.succeeded());
 
   // If the slice contains `insertionPoint` cannot move the dependencies.
   if (slice.contains(insertionPoint)) {
@@ -1159,7 +1160,8 @@ LogicalResult mlir::moveValueDefinitions(RewriterBase &rewriter,
   };
   llvm::SetVector<Operation *> slice;
   for (auto value : prunedValues) {
-    getBackwardSlice(value, &slice, options);
+    auto result = getBackwardSlice(value, &slice, options);
+    assert(result.succeeded());
   }
 
   // If the slice contains `insertionPoint` cannot move the dependencies.
diff --git a/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp b/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp
index f26058f30ad7b..041cc25b23cbd 100644
--- a/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp
+++ b/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp
@@ -154,7 +154,8 @@ void VectorizerTestPass::testBackwardSlicing(llvm::raw_ostream &outs) {
   patternTestSlicingOps().match(f, &matches);
   for (auto m : matches) {
     SetVector<Operation *> backwardSlice;
-    getBackwardSlice(m.getMatchedOperation(), &backwardSlice);
+    auto result = getBackwardSlice(m.getMatchedOperation(), &backwardSlice);
+    assert(result.succeeded());
     outs << "\nmatched: " << *m.getMatchedOperation()
          << " backward static slice: ";
     for (auto *op : backwardSlice)
diff --git a/mlir/test/lib/IR/TestSlicing.cpp b/mlir/test/lib/IR/TestSlicing.cpp
index e99d5976d6d9d..53ad9b5819986 100644
--- a/mlir/test/lib/IR/TestSlicing.cpp
+++ b/mlir/test/lib/IR/TestSlicing.cpp
@@ -41,7 +41,8 @@ static LogicalResult createBackwardSliceFunction(Operation *op,
   options.omitBlockArguments = omitBlockArguments;
   // TODO: Make this default.
   options.omitUsesFromAbove = false;
-  getBackwardSlice(op, &slice, options);
+  auto result = getBackwardSlice(op, &slice, options);
+  assert(result.succeeded());
   for (Operation *slicedOp : slice)
     builder.clone(*slicedOp, mapper);
   builder.create<func::ReturnOp>(loc);

llvmbot · 2025-05-21T21:08:20Z

@llvm/pr-subscribers-mlir-scf

Author: William Moses (wsmoses)

Changes

…crash

Full diff: https://github.com/llvm/llvm-project/pull/140961.diff

10 Files Affected:

(modified) mlir/include/mlir/Analysis/SliceAnalysis.h (+6-4)
(modified) mlir/include/mlir/Query/Matcher/SliceMatchers.h (+2-1)
(modified) mlir/lib/Analysis/SliceAnalysis.cpp (+32-20)
(modified) mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp (+3-1)
(modified) mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp (+5-2)
(modified) mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp (+4-2)
(modified) mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp (+2-1)
(modified) mlir/lib/Transforms/Utils/RegionUtils.cpp (+4-2)
(modified) mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp (+2-1)
(modified) mlir/test/lib/IR/TestSlicing.cpp (+2-1)

diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h
index 3b731e8bb1c22..192b096fd31f6 100644
--- a/mlir/include/mlir/Analysis/SliceAnalysis.h
+++ b/mlir/include/mlir/Analysis/SliceAnalysis.h
@@ -138,13 +138,15 @@ void getForwardSlice(Value root, SetVector<Operation *> *forwardSlice,
 /// Assuming all local orders match the numbering order:
 ///    {1, 2, 5, 3, 4, 6}
 ///
-void getBackwardSlice(Operation *op, SetVector<Operation *> *backwardSlice,
-                      const BackwardSliceOptions &options = {});
+LogicalResult getBackwardSlice(Operation *op,
+                               SetVector<Operation *> *backwardSlice,
+                               const BackwardSliceOptions &options = {});
 
 /// Value-rooted version of `getBackwardSlice`. Return the union of all backward
 /// slices for the op defining or owning the value `root`.
-void getBackwardSlice(Value root, SetVector<Operation *> *backwardSlice,
-                      const BackwardSliceOptions &options = {});
+LogicalResult getBackwardSlice(Value root,
+                               SetVector<Operation *> *backwardSlice,
+                               const BackwardSliceOptions &options = {});
 
 /// Iteratively computes backward slices and forward slices until
 /// a fixed point is reached. Returns an `SetVector<Operation *>` which
diff --git a/mlir/include/mlir/Query/Matcher/SliceMatchers.h b/mlir/include/mlir/Query/Matcher/SliceMatchers.h
index 1b0e4c32dbe94..6b7d10ccaed64 100644
--- a/mlir/include/mlir/Query/Matcher/SliceMatchers.h
+++ b/mlir/include/mlir/Query/Matcher/SliceMatchers.h
@@ -112,7 +112,8 @@ bool BackwardSliceMatcher<Matcher>::matches(
     }
     return true;
   };
-  getBackwardSlice(rootOp, &backwardSlice, options);
+  auto result = getBackwardSlice(rootOp, &backwardSlice, options);
+  assert(result.succeeded());
   return options.inclusive ? backwardSlice.size() > 1
                            : backwardSlice.size() >= 1;
 }
diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp
index 5aebb19e3a86e..5992aceba82d3 100644
--- a/mlir/lib/Analysis/SliceAnalysis.cpp
+++ b/mlir/lib/Analysis/SliceAnalysis.cpp
@@ -80,22 +80,25 @@ void mlir::getForwardSlice(Value root, SetVector<Operation *> *forwardSlice,
   forwardSlice->insert(v.rbegin(), v.rend());
 }
 
-static void getBackwardSliceImpl(Operation *op,
-                                 SetVector<Operation *> *backwardSlice,
-                                 const BackwardSliceOptions &options) {
+static LogicalResult getBackwardSliceImpl(Operation *op,
+                                          SetVector<Operation *> *backwardSlice,
+                                          const BackwardSliceOptions &options) {
   if (!op || op->hasTrait<OpTrait::IsIsolatedFromAbove>())
-    return;
+    return success();
 
   // Evaluate whether we should keep this def.
   // This is useful in particular to implement scoping; i.e. return the
   // transitive backwardSlice in the current scope.
   if (options.filter && !options.filter(op))
-    return;
+    return success();
+
+  bool succeeded = true;
 
   auto processValue = [&](Value value) {
     if (auto *definingOp = value.getDefiningOp()) {
       if (backwardSlice->count(definingOp) == 0)
-        getBackwardSliceImpl(definingOp, backwardSlice, options);
+        succeeded &= getBackwardSliceImpl(definingOp, backwardSlice, options)
+                         .succeeded();
     } else if (auto blockArg = dyn_cast<BlockArgument>(value)) {
       if (options.omitBlockArguments)
         return;
@@ -106,9 +109,13 @@ static void getBackwardSliceImpl(Operation *op,
       // blocks of parentOp, which are not technically backward unless they flow
       // into us. For now, just bail.
       if (parentOp && backwardSlice->count(parentOp) == 0) {
-        assert(parentOp->getNumRegions() == 1 &&
-               llvm::hasSingleElement(parentOp->getRegion(0).getBlocks()));
-        getBackwardSliceImpl(parentOp, backwardSlice, options);
+        if (parentOp->getNumRegions() == 1 &&
+            llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())) {
+          succeeded &= getBackwardSliceImpl(parentOp, backwardSlice, options)
+                           .succeeded();
+        } else {
+          succeeded = false;
+        }
       }
     } else {
       llvm_unreachable("No definingOp and not a block argument.");
@@ -133,28 +140,30 @@ static void getBackwardSliceImpl(Operation *op,
   llvm::for_each(op->getOperands(), processValue);
 
   backwardSlice->insert(op);
+  return succeess(succeeded);
 }
 
-void mlir::getBackwardSlice(Operation *op,
-                            SetVector<Operation *> *backwardSlice,
-                            const BackwardSliceOptions &options) {
-  getBackwardSliceImpl(op, backwardSlice, options);
+LogicalResult result
+mlir::getBackwardSlice(Operation *op, SetVector<Operation *> *backwardSlice,
+                       const BackwardSliceOptions &options) {
+  LogicalResult result = getBackwardSliceImpl(op, backwardSlice, options);
 
   if (!options.inclusive) {
     // Don't insert the top level operation, we just queried on it and don't
     // want it in the results.
     backwardSlice->remove(op);
   }
+  return result;
 }
 
-void mlir::getBackwardSlice(Value root, SetVector<Operation *> *backwardSlice,
-                            const BackwardSliceOptions &options) {
+LogicalResult mlir::getBackwardSlice(Value root,
+                                     SetVector<Operation *> *backwardSlice,
+                                     const BackwardSliceOptions &options) {
   if (Operation *definingOp = root.getDefiningOp()) {
-    getBackwardSlice(definingOp, backwardSlice, options);
-    return;
+    return getBackwardSlice(definingOp, backwardSlice, options);
   }
   Operation *bbAargOwner = cast<BlockArgument>(root).getOwner()->getParentOp();
-  getBackwardSlice(bbAargOwner, backwardSlice, options);
+  return getBackwardSlice(bbAargOwner, backwardSlice, options);
 }
 
 SetVector<Operation *>
@@ -170,7 +179,9 @@ mlir::getSlice(Operation *op, const BackwardSliceOptions &backwardSliceOptions,
     auto *currentOp = (slice)[currentIndex];
     // Compute and insert the backwardSlice starting from currentOp.
     backwardSlice.clear();
-    getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions);
+    auto result =
+        getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions);
+    assert(result.succeeded());
     slice.insert_range(backwardSlice);
 
     // Compute and insert the forwardSlice starting from currentOp.
@@ -193,7 +204,8 @@ static bool dependsOnCarriedVals(Value value,
   sliceOptions.filter = [&](Operation *op) {
     return !ancestorOp->isAncestor(op);
   };
-  getBackwardSlice(value, &slice, sliceOptions);
+  auto result = getBackwardSlice(value, &slice, sliceOptions);
+  assert(result.succeeded());
 
   // Check that none of the operands of the operations in the backward slice are
   // loop iteration arguments, and neither is the value itself.
diff --git a/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp b/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp
index 8b16da387457d..22df878cfe18d 100644
--- a/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp
+++ b/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp
@@ -317,7 +317,9 @@ getSliceContract(Operation *op,
     auto *currentOp = (slice)[currentIndex];
     // Compute and insert the backwardSlice starting from currentOp.
     backwardSlice.clear();
-    getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions);
+    auto result =
+        getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions);
+    assert(result.succeeded());
     slice.insert_range(backwardSlice);
 
     // Compute and insert the forwardSlice starting from currentOp.
diff --git a/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp b/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp
index d33a17af63459..ee0d44be60b59 100644
--- a/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp
+++ b/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp
@@ -124,10 +124,13 @@ static void computeBackwardSlice(tensor::PadOp padOp,
   getUsedValuesDefinedAbove(padOp.getRegion(), padOp.getRegion(),
                             valuesDefinedAbove);
   for (Value v : valuesDefinedAbove) {
-    getBackwardSlice(v, &backwardSlice, sliceOptions);
+    auto result = getBackwardSlice(v, &backwardSlice, sliceOptions);
+    assert(result.succeeded());
   }
   // Then, add the backward slice from padOp itself.
-  getBackwardSlice(padOp.getOperation(), &backwardSlice, sliceOptions);
+  auto result =
+      getBackwardSlice(padOp.getOperation(), &backwardSlice, sliceOptions);
+  assert(result.succeeded());
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp b/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp
index 75dbe0becf80d..c5b8117283b25 100644
--- a/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp
+++ b/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp
@@ -290,8 +290,10 @@ static void getPipelineStages(
   });
   options.inclusive = true;
   for (Operation &op : forOp.getBody()->getOperations()) {
-    if (stage0Ops.contains(&op))
-      getBackwardSlice(&op, &dependencies, options);
+    if (stage0Ops.contains(&op)) {
+      auto result = getBackwardSlice(&op, &dependencies, options);
+      assert(result.succeeded());
+    }
   }
 
   for (Operation &op : forOp.getBody()->getOperations()) {
diff --git a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
index 719e2c6fa459e..273fdbfbc4ffe 100644
--- a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
+++ b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
@@ -1772,7 +1772,8 @@ checkAssumptionForLoop(Operation *loopOp, Operation *consumerOp,
   };
   llvm::SetVector<Operation *> slice;
   for (auto operand : consumerOp->getOperands()) {
-    getBackwardSlice(operand, &slice, options);
+    auto result = getBackwardSlice(operand, &slice, options);
+    assert(result.succeeded());
   }
 
   if (!slice.empty()) {
diff --git a/mlir/lib/Transforms/Utils/RegionUtils.cpp b/mlir/lib/Transforms/Utils/RegionUtils.cpp
index 4985d718c1780..70ba5e321db6d 100644
--- a/mlir/lib/Transforms/Utils/RegionUtils.cpp
+++ b/mlir/lib/Transforms/Utils/RegionUtils.cpp
@@ -1094,7 +1094,8 @@ LogicalResult mlir::moveOperationDependencies(RewriterBase &rewriter,
     return !dominance.properlyDominates(sliceBoundaryOp, insertionPoint);
   };
   llvm::SetVector<Operation *> slice;
-  getBackwardSlice(op, &slice, options);
+  auto result = getBackwardSlice(op, &slice, options);
+  assert(result.succeeded());
 
   // If the slice contains `insertionPoint` cannot move the dependencies.
   if (slice.contains(insertionPoint)) {
@@ -1159,7 +1160,8 @@ LogicalResult mlir::moveValueDefinitions(RewriterBase &rewriter,
   };
   llvm::SetVector<Operation *> slice;
   for (auto value : prunedValues) {
-    getBackwardSlice(value, &slice, options);
+    auto result = getBackwardSlice(value, &slice, options);
+    assert(result.succeeded());
   }
 
   // If the slice contains `insertionPoint` cannot move the dependencies.
diff --git a/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp b/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp
index f26058f30ad7b..041cc25b23cbd 100644
--- a/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp
+++ b/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp
@@ -154,7 +154,8 @@ void VectorizerTestPass::testBackwardSlicing(llvm::raw_ostream &outs) {
   patternTestSlicingOps().match(f, &matches);
   for (auto m : matches) {
     SetVector<Operation *> backwardSlice;
-    getBackwardSlice(m.getMatchedOperation(), &backwardSlice);
+    auto result = getBackwardSlice(m.getMatchedOperation(), &backwardSlice);
+    assert(result.succeeded());
     outs << "\nmatched: " << *m.getMatchedOperation()
          << " backward static slice: ";
     for (auto *op : backwardSlice)
diff --git a/mlir/test/lib/IR/TestSlicing.cpp b/mlir/test/lib/IR/TestSlicing.cpp
index e99d5976d6d9d..53ad9b5819986 100644
--- a/mlir/test/lib/IR/TestSlicing.cpp
+++ b/mlir/test/lib/IR/TestSlicing.cpp
@@ -41,7 +41,8 @@ static LogicalResult createBackwardSliceFunction(Operation *op,
   options.omitBlockArguments = omitBlockArguments;
   // TODO: Make this default.
   options.omitUsesFromAbove = false;
-  getBackwardSlice(op, &slice, options);
+  auto result = getBackwardSlice(op, &slice, options);
+  assert(result.succeeded());
   for (Operation *slicedOp : slice)
     builder.clone(*slicedOp, mapper);
   builder.create<func::ReturnOp>(loc);

…crash

github-actions · 2025-05-21T21:20:59Z

✅ With the latest revision this PR passed the C/C++ code formatter.

ftynse

We need some sort of test. The simplest way is likely to update one of the usptream user passes to propagate failure instead of asserting, and have a test triggering that pass failure.

mlir/include/mlir/Query/Matcher/SliceMatchers.h

mlir/lib/Analysis/SliceAnalysis.cpp

ftynse · 2025-05-21T21:33:25Z

mlir/lib/Analysis/SliceAnalysis.cpp

  }
  Operation *bbAargOwner = cast<BlockArgument>(root).getOwner()->getParentOp();
-  getBackwardSlice(bbAargOwner, backwardSlice, options);
+  return getBackwardSlice(bbAargOwner, backwardSlice, options);
 }

 SetVector<Operation *>


We may want to return FailureOr<SetVector<Operation *>> here for consistency with the rest of the API.

mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp

mlir/lib/Transforms/Utils/RegionUtils.cpp

mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp

mlir/test/lib/IR/TestSlicing.cpp

ftynse

Modulo comments and reformatting.

Co-authored-by: Oleksandr "Alex" Zinenko <[email protected]>

joker-eph · 2025-05-22T09:45:03Z

Please update your PR title / description to undo the wrap-around that GitHub inserted.

Also some more info in the description could be welcome I think.

mlir/include/mlir/Analysis/SliceAnalysis.h

joker-eph · 2025-05-23T10:05:21Z

mlir/include/mlir/Analysis/SliceAnalysis.h

-void getBackwardSlice(Operation *op, SetVector<Operation *> *backwardSlice,
-                      const BackwardSliceOptions &options = {});
+/// This function returns whether the backwards slice was able to be
+/// successfully computed, and failure if it was unable to determine the slice.


That isn't a helpful comment IMO: just seeing the LogicalResult I understand that "this can fail".

What I'd expect is describe the failure condition here. What is a prerequisite of the API (which you documented in the PR description now).

Ping on this?

ah sorry for delay, fell of my stack. addressed here: #142223

…er than crash (llvm#140961)" This reverts commit 6a8dde0.

hanhanW

Where is the test for the change? I think the behavior is changed, but I don't find corresponding tests. Do I miss something?

EDIT: This breaks some integration tests in a downstream project

hanhanW · 2025-05-29T18:32:14Z

mlir/lib/Analysis/SliceAnalysis.cpp

  };

+  bool succeeded = true;


This is always true?

hanhanW · 2025-05-29T18:43:34Z

mlir/lib/Analysis/SliceAnalysis.cpp

+            if (!processValue(operand.get()).succeeded()) {
+              return WalkResult::interrupt();
+            }
        }
+        return WalkResult::advance();


This breaks some integration tests in a downstream project, and I think you should not interrupt when there is a failure. The below patch fixes our issue. I don't have a clean repro now, but I'd like to share this information first.

--- a/mlir/lib/Analysis/SliceAnalysis.cpp +++ b/mlir/lib/Analysis/SliceAnalysis.cpp @@ -127,11 +127,8 @@ static LogicalResult getBackwardSliceImpl(Operation *op, region.walk([&](Operation *op) { for (OpOperand &operand : op->getOpOperands()) { if (!descendents.contains(operand.get().getParentRegion())) - if (!processValue(operand.get()).succeeded()) { - return WalkResult::interrupt(); - } + (void)processValue(operand.get()); } - return WalkResult::advance(); }); }); }

I don't fully follow what the PR is doing, and I'll follow up when I have cycles.

hanhanW · 2025-05-29T19:04:13Z

mlir/lib/Analysis/SliceAnalysis.cpp

    }
+    return failure();


@IanWood1 shared a fix with me, and I think it may be reasonable.

The original check could be false, and it is not a failure. Should it return success() in this case?

if (parentOp && backwardSlice->count(parentOp) == 0) { assert(parentOp->getNumRegions() == 1 && llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())); getBackwardSliceImpl(parentOp, backwardSlice, options); }

EDIT: To be more specific, should we return success() in this line?

…er than crash (llvm#140961)" This reverts commit 6a8dde0.

hanhanW · 2025-05-30T01:38:10Z

I go ahead to send a fix that recovers the behavior: #142076

…crash (llvm#140961) The current implementation of getBackwardSlice will crash if an operation in the dependency chain is defined by an operation with multiple regions or blocks. Crashing is bad (and forbids many analyses from using getBackwardSlice, as well as causing existing users of getBackwardSlice to fail for IR with this property). This PR instead causes the analysis to return a failure, rather than crash in the cases it cannot compute the full slice --------- Co-authored-by: Oleksandr "Alex" Zinenko <[email protected]>

wsmoses requested a review from ftynse May 21, 2025 21:07

wsmoses requested review from grypp, dcaballe, nicolasvasilache and rengolin as code owners May 21, 2025 21:07

llvmbot added mlir:core MLIR Core Infrastructure mlir:linalg mlir:gpu mlir:affine mlir mlir:scf mlir:nvgpu labels May 21, 2025

wsmoses force-pushed the users/wsmoses/lr branch from f20f234 to c64f316 Compare May 21, 2025 21:08

[MLIR] Change getBackwardSlice to return a logicalresult rather than …

4fc8bfb

…crash

wsmoses force-pushed the users/wsmoses/lr branch from c64f316 to 4fc8bfb Compare May 21, 2025 21:17

ftynse reviewed May 21, 2025

View reviewed changes

ftynse approved these changes May 21, 2025

View reviewed changes

Apply suggestions from code review

f1f9ca6

Co-authored-by: Oleksandr "Alex" Zinenko <[email protected]>

joker-eph reviewed May 22, 2025

View reviewed changes

mlir/include/mlir/Analysis/SliceAnalysis.h Show resolved Hide resolved

wsmoses changed the title ~~[MLIR] Change getBackwardSlice to return a logicalresult rather than …~~ [MLIR] Change getBackwardSlice to return a logicalresult rather than crash May 22, 2025

wsmoses added 4 commits May 22, 2025 12:09

fix

57f9210

fmt

cec29a9

fix

a16ebc5

fmt

3ff609b

wsmoses merged commit 6a8dde0 into main May 22, 2025
9 of 10 checks passed

wsmoses deleted the users/wsmoses/lr branch May 22, 2025 19:13

joker-eph reviewed May 23, 2025

View reviewed changes

hanhanW added a commit to iree-org/llvm-project that referenced this pull request May 29, 2025

Revert "[MLIR] Change getBackwardSlice to return a logicalresult rath…

cddcc13

…er than crash (llvm#140961)" This reverts commit 6a8dde0.

hanhanW reviewed May 29, 2025

View reviewed changes

hanhanW added a commit to iree-org/llvm-project that referenced this pull request May 29, 2025

Revert "[MLIR] Change getBackwardSlice to return a logicalresult rath…

edb8c26

…er than crash (llvm#140961)" This reverts commit 6a8dde0.

wsmoses mentioned this pull request May 30, 2025

[MLIR][SliceAnalysis] Add comment and restore failure condition #142223

Open

[MLIR] Change getBackwardSlice to return a logicalresult rather than crash #140961

[MLIR] Change getBackwardSlice to return a logicalresult rather than crash #140961

Uh oh!

Conversation

wsmoses commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 21, 2025

Uh oh!

github-actions bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ftynse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ftynse May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ftynse left a comment

Choose a reason for hiding this comment

Uh oh!

joker-eph commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joker-eph May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joker-eph May 29, 2025

Choose a reason for hiding this comment

Uh oh!

wsmoses May 30, 2025

Choose a reason for hiding this comment

Uh oh!

hanhanW left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanhanW May 29, 2025

Choose a reason for hiding this comment

Uh oh!

hanhanW May 29, 2025

Choose a reason for hiding this comment

Uh oh!

hanhanW May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanhanW commented May 30, 2025

Uh oh!

Uh oh!

wsmoses commented May 21, 2025 •

edited

Loading

llvmbot commented May 21, 2025 •

edited

Loading

github-actions bot commented May 21, 2025 •

edited

Loading

joker-eph commented May 22, 2025 •

edited

Loading

joker-eph May 23, 2025 •

edited

Loading

hanhanW left a comment •

edited

Loading

hanhanW May 29, 2025 •

edited

Loading