[Mlir-commits] [mlir] [MLIR] Change getBackwardSlice to return a logicalresult rather than crash (PR #140961)

[Han-Chung Wang]

================
@@ -80,41 +80,43 @@ 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();
 
   auto processValue = [&](Value value) {
     if (auto *definingOp = value.getDefiningOp()) {
       if (backwardSlice->count(definingOp) == 0)
-        getBackwardSliceImpl(definingOp, backwardSlice, options);
+        return getBackwardSliceImpl(definingOp, backwardSlice, options);
     } else if (auto blockArg = dyn_cast<BlockArgument>(value)) {
       if (options.omitBlockArguments)
-        return;
+        return success();
 
       Block *block = blockArg.getOwner();
       Operation *parentOp = block->getParentOp();
       // TODO: determine whether we want to recurse backward into the other
       // 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())) {
+          return getBackwardSliceImpl(parentOp, backwardSlice, options);
+        }
       }
-    } else {
-      llvm_unreachable("No definingOp and not a block argument.");
     }
+    return failure();
----------------
hanhanW wrote:

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

The original check could be false. Should it return `success()` in this case?

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

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