[mlir][Analysis] Remove return value from `getBackwardSlice`users/matthias-springer/simplify_backward_slice

10 files changed, 58 insertions, 108 deletions

diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h
index 18349d071bb2..89f570a7c711 100644
--- a/mlir/include/mlir/Analysis/SliceAnalysis.h
+++ b/mlir/include/mlir/Analysis/SliceAnalysis.h
@@ -140,17 +140,13 @@ void getForwardSlice(Value root, SetVector<Operation *> *forwardSlice,
 /// Assuming all local orders match the numbering order:
 ///    {1, 2, 5, 3, 4, 6}
 ///
-/// This function returns whether the backwards slice was able to be
-/// successfully computed, and failure if it was unable to determine the slice.
-LogicalResult getBackwardSlice(Operation *op,
-                               SetVector<Operation *> *backwardSlice,
-                               const BackwardSliceOptions &options = {});
+void 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`.
-LogicalResult getBackwardSlice(Value root,
-                               SetVector<Operation *> *backwardSlice,
-                               const BackwardSliceOptions &options = {});
+void 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 43b699817927..58ca187eec36 100644
--- a/mlir/include/mlir/Query/Matcher/SliceMatchers.h
+++ b/mlir/include/mlir/Query/Matcher/SliceMatchers.h
@@ -113,9 +113,7 @@ bool BackwardSliceMatcher<Matcher>::matches(
     }
     return true;
   };
-  LogicalResult result = getBackwardSlice(rootOp, &backwardSlice, options);
-  assert(result.succeeded() && "expected backward slice to succeed");
-  (void)result;
+  getBackwardSlice(rootOp, &backwardSlice, options);
   return options.inclusive ? backwardSlice.size() > 1
                            : backwardSlice.size() >= 1;
 }
@@ -143,9 +141,7 @@ public:
       options.filter = [&](Operation *subOp) {
         return !filterMatcher.match(subOp);
       };
-      LogicalResult result = getBackwardSlice(rootOp, &backwardSlice, options);
-      assert(result.succeeded() && "expected backward slice to succeed");
-      (void)result;
+      getBackwardSlice(rootOp, &backwardSlice, options);
       return options.inclusive ? backwardSlice.size() > 1
                                : backwardSlice.size() >= 1;
     }
diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp
index 12dff19ed31d..ac9c96e5dd85 100644
--- a/mlir/lib/Analysis/SliceAnalysis.cpp
+++ b/mlir/lib/Analysis/SliceAnalysis.cpp
@@ -105,52 +105,47 @@ void mlir::getForwardSlice(Value root, SetVector<Operation *> *forwardSlice,
   forwardSlice->insert(v.rbegin(), v.rend());
 }
 
-static LogicalResult getBackwardSliceImpl(Operation *op,
-                                          DenseSet<Operation *> &visited,
-                                          SetVector<Operation *> *backwardSlice,
-                                          const BackwardSliceOptions &options) {
+static void getBackwardSliceImpl(Operation *op, DenseSet<Operation *> &visited,
+                                 SetVector<Operation *> *backwardSlice,
+                                 const BackwardSliceOptions &options) {
   if (!op)
-    return success();
+    return;
 
   // 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 success();
+    return;
 
   auto processValue = [&](Value value) {
     if (auto *definingOp = value.getDefiningOp()) {
       if (backwardSlice->count(definingOp) == 0 &&
-          visited.insert(definingOp).second)
-        return getBackwardSliceImpl(definingOp, visited, backwardSlice,
-                                    options);
+          visited.insert(definingOp).second) {
+        getBackwardSliceImpl(definingOp, visited, backwardSlice, options);
+        return;
+      }
 
       visited.erase(definingOp);
-    } else if (auto blockArg = dyn_cast<BlockArgument>(value)) {
-      if (options.omitBlockArguments)
-        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) {
-        if (!parentOp->hasTrait<OpTrait::IsIsolatedFromAbove>() &&
-            parentOp->getNumRegions() == 1 &&
-            parentOp->getRegion(0).hasOneBlock()) {
-          return getBackwardSliceImpl(parentOp, visited, backwardSlice,
-                                      options);
-        }
+      return;
+    }
+    auto blockArg = cast<BlockArgument>(value);
+    if (options.omitBlockArguments)
+      return;
+
+    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) {
+      if (!parentOp->hasTrait<OpTrait::IsIsolatedFromAbove>() &&
+          parentOp->getNumRegions() == 1 &&
+          parentOp->getRegion(0).hasOneBlock()) {
+        getBackwardSliceImpl(parentOp, visited, backwardSlice, options);
       }
-    } else {
-      return failure();
     }
-    return success();
   };
 
-  bool succeeded = true;
-
   if (!options.omitUsesFromAbove &&
       !op->hasTrait<OpTrait::IsIsolatedFromAbove>()) {
     llvm::for_each(op->getRegions(), [&](Region &region) {
@@ -162,44 +157,38 @@ 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();
-            }
+            processValue(operand.get());
         }
-        return WalkResult::advance();
       });
     });
   }
   llvm::for_each(op->getOperands(), processValue);
 
   backwardSlice->insert(op);
-  return success(succeeded);
 }
 
-LogicalResult mlir::getBackwardSlice(Operation *op,
-                                     SetVector<Operation *> *backwardSlice,
-                                     const BackwardSliceOptions &options) {
+void mlir::getBackwardSlice(Operation *op,
+                            SetVector<Operation *> *backwardSlice,
+                            const BackwardSliceOptions &options) {
   DenseSet<Operation *> visited;
   visited.insert(op);
-  LogicalResult result =
-      getBackwardSliceImpl(op, visited, backwardSlice, options);
+  getBackwardSliceImpl(op, visited, 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;
 }
 
-LogicalResult mlir::getBackwardSlice(Value root,
-                                     SetVector<Operation *> *backwardSlice,
-                                     const BackwardSliceOptions &options) {
+void mlir::getBackwardSlice(Value root, SetVector<Operation *> *backwardSlice,
+                            const BackwardSliceOptions &options) {
   if (Operation *definingOp = root.getDefiningOp()) {
-    return getBackwardSlice(definingOp, backwardSlice, options);
+    getBackwardSlice(definingOp, backwardSlice, options);
+    return;
   }
-  Operation *bbAargOwner = cast<BlockArgument>(root).getOwner()->getParentOp();
-  return getBackwardSlice(bbAargOwner, backwardSlice, options);
+  Operation *bbArgOwner = cast<BlockArgument>(root).getOwner()->getParentOp();
+  getBackwardSlice(bbArgOwner, backwardSlice, options);
 }
 
 SetVector<Operation *>
@@ -215,10 +204,7 @@ mlir::getSlice(Operation *op, const BackwardSliceOptions &backwardSliceOptions,
     auto *currentOp = (slice)[currentIndex];
     // Compute and insert the backwardSlice starting from currentOp.
     backwardSlice.clear();
-    LogicalResult result =
-        getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions);
-    assert(result.succeeded());
-    (void)result;
+    getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions);
     slice.insert_range(backwardSlice);
 
     // Compute and insert the forwardSlice starting from currentOp.
@@ -241,9 +227,7 @@ static bool dependsOnCarriedVals(Value value,
   sliceOptions.filter = [&](Operation *op) {
     return !ancestorOp->isAncestor(op);
   };
-  LogicalResult result = getBackwardSlice(value, &slice, sliceOptions);
-  assert(result.succeeded());
-  (void)result;
+  getBackwardSlice(value, &slice, sliceOptions);
 
   // 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 98434357f826..417b6bb3030d 100644
--- a/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp
+++ b/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp
@@ -312,10 +312,7 @@ getSliceContract(Operation *op,
     auto *currentOp = (slice)[currentIndex];
     // Compute and insert the backwardSlice starting from currentOp.
     backwardSlice.clear();
-    LogicalResult result =
-        getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions);
-    assert(result.succeeded() && "expected a backward slice");
-    (void)result;
+    getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions);
     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 9436f1c6cd9b..5e28306e0093 100644
--- a/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp
+++ b/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp
@@ -122,16 +122,10 @@ static void computeBackwardSlice(tensor::PadOp padOp,
   SetVector<Value> valuesDefinedAbove;
   getUsedValuesDefinedAbove(padOp.getRegion(), padOp.getRegion(),
                             valuesDefinedAbove);
-  for (Value v : valuesDefinedAbove) {
-    LogicalResult result = getBackwardSlice(v, &backwardSlice, sliceOptions);
-    assert(result.succeeded() && "expected a backward slice");
-    (void)result;
-  }
+  for (Value v : valuesDefinedAbove)
+    getBackwardSlice(v, &backwardSlice, sliceOptions);
   // Then, add the backward slice from padOp itself.
-  LogicalResult result =
-      getBackwardSlice(padOp.getOperation(), &backwardSlice, sliceOptions);
-  assert(result.succeeded() && "expected a backward slice");
-  (void)result;
+  getBackwardSlice(padOp.getOperation(), &backwardSlice, sliceOptions);
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp b/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp
index 2a857eddbb93..efba95699069 100644
--- a/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp
+++ b/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp
@@ -281,13 +281,9 @@ static void getPipelineStages(
     return visited->getBlock() == forOp.getBody();
   });
   options.inclusive = true;
-  for (Operation &op : forOp.getBody()->getOperations()) {
-    if (stage0Ops.contains(&op)) {
-      LogicalResult result = getBackwardSlice(&op, &dependencies, options);
-      assert(result.succeeded() && "expected a backward slice");
-      (void)result;
-    }
-  }
+  for (Operation &op : forOp.getBody()->getOperations())
+    if (stage0Ops.contains(&op))
+      getBackwardSlice(&op, &dependencies, options);
 
   for (Operation &op : forOp.getBody()->getOperations()) {
     if (!dependencies.contains(&op) && !isa<scf::YieldOp>(op))
diff --git a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
index 29b770fb4b27..44bdfc28ef8e 100644
--- a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
+++ b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
@@ -1970,11 +1970,8 @@ checkAssumptionForLoop(Operation *loopOp, Operation *consumerOp,
     return !dominanceInfo.properlyDominates(op, *firstUserOfLoop);
   };
   llvm::SetVector<Operation *> slice;
-  for (auto operand : consumerOp->getOperands()) {
-    LogicalResult result = getBackwardSlice(operand, &slice, options);
-    assert(result.succeeded() && "expected a backward slice");
-    (void)result;
-  }
+  for (auto operand : consumerOp->getOperands())
+    getBackwardSlice(operand, &slice, options);
 
   if (!slice.empty()) {
     // If consumerOp has one producer, which is also the user of loopOp.
diff --git a/mlir/lib/Transforms/Utils/RegionUtils.cpp b/mlir/lib/Transforms/Utils/RegionUtils.cpp
index 31ae1d1895b8..d00e137124a4 100644
--- a/mlir/lib/Transforms/Utils/RegionUtils.cpp
+++ b/mlir/lib/Transforms/Utils/RegionUtils.cpp
@@ -1116,9 +1116,7 @@ LogicalResult mlir::moveOperationDependencies(RewriterBase &rewriter,
     return !dominance.properlyDominates(sliceBoundaryOp, insertionPoint);
   };
   llvm::SetVector<Operation *> slice;
-  LogicalResult result = getBackwardSlice(op, &slice, options);
-  assert(result.succeeded() && "expected a backward slice");
-  (void)result;
+  getBackwardSlice(op, &slice, options);
 
   // If the slice contains `insertionPoint` cannot move the dependencies.
   if (slice.contains(insertionPoint)) {
@@ -1182,11 +1180,8 @@ LogicalResult mlir::moveValueDefinitions(RewriterBase &rewriter,
     return !dominance.properlyDominates(sliceBoundaryOp, insertionPoint);
   };
   llvm::SetVector<Operation *> slice;
-  for (auto value : prunedValues) {
-    LogicalResult result = getBackwardSlice(value, &slice, options);
-    assert(result.succeeded() && "expected a backward slice");
-    (void)result;
-  }
+  for (auto value : prunedValues)
+    getBackwardSlice(value, &slice, options);
 
   // If the slice contains `insertionPoint` cannot move the dependencies.
   if (slice.contains(insertionPoint)) {
diff --git a/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp b/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp
index 2c9399138667..f26058f30ad7 100644
--- a/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp
+++ b/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp
@@ -154,10 +154,7 @@ void VectorizerTestPass::testBackwardSlicing(llvm::raw_ostream &outs) {
   patternTestSlicingOps().match(f, &matches);
   for (auto m : matches) {
     SetVector<Operation *> backwardSlice;
-    LogicalResult result =
-        getBackwardSlice(m.getMatchedOperation(), &backwardSlice);
-    assert(result.succeeded() && "expected a backward slice");
-    (void)result;
+    getBackwardSlice(m.getMatchedOperation(), &backwardSlice);
     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 5a5ac450f91f..95db70ec7a81 100644
--- a/mlir/test/lib/IR/TestSlicing.cpp
+++ b/mlir/test/lib/IR/TestSlicing.cpp
@@ -41,9 +41,7 @@ static LogicalResult createBackwardSliceFunction(Operation *op,
   options.omitBlockArguments = omitBlockArguments;
   // TODO: Make this default.
   options.omitUsesFromAbove = false;
-  LogicalResult result = getBackwardSlice(op, &slice, options);
-  assert(result.succeeded() && "expected a backward slice");
-  (void)result;
+  getBackwardSlice(op, &slice, options);
   for (Operation *slicedOp : slice)
     builder.clone(*slicedOp, mapper);
   func::ReturnOp::create(builder, loc);