[mlir][Analysis] Remove return value from getBackwardSlice
#163749
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matthias-springer
requested review from
dcaballe,
grypp,
nicolasvasilache and
rengolin
as code owners
llvmbot
added
mlir:core
|
@llvm/pr-subscribers-mlir-linalg @llvm/pr-subscribers-mlir-affine Author: Matthias Springer (matthias-springer) ChangesThis function went through a series of refactorings, most notably #140961. In the current version, there is no case where the function could return Full diff: https://github.com/llvm/llvm-project/pull/163749.diff 10 Files Affected:
diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h
index 18349d071bb2e..89f570a7c7115 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 43b699817927c..58ca187eec360 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 @@ class PredicateBackwardSliceMatcher {
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 12dff19ed31d3..ac9c96e5dd855 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 ®ion) {
@@ -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 98434357f826f..417b6bb3030d6 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 9436f1c6cd9b0..5e28306e00935 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 2a857eddbb932..efba956990697 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 29b770fb4b279..44bdfc28ef8ee 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 31ae1d1895b81..d00e137124a4e 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 2c93991386675..f26058f30ad7b 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 5a5ac450f91fb..95db70ec7a815 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);
|
|
@llvm/pr-subscribers-mlir Author: Matthias Springer (matthias-springer) ChangesThis function went through a series of refactorings, most notably #140961. In the current version, there is no case where the function could return Full diff: https://github.com/llvm/llvm-project/pull/163749.diff 10 Files Affected:
diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h
index 18349d071bb2e..89f570a7c7115 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 43b699817927c..58ca187eec360 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 @@ class PredicateBackwardSliceMatcher {
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 12dff19ed31d3..ac9c96e5dd855 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 ®ion) {
@@ -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 98434357f826f..417b6bb3030d6 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 9436f1c6cd9b0..5e28306e00935 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 2a857eddbb932..efba956990697 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 29b770fb4b279..44bdfc28ef8ee 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 31ae1d1895b81..d00e137124a4e 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 2c93991386675..f26058f30ad7b 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 5a5ac450f91fb..95db70ec7a815 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);
|
|
@llvm/pr-subscribers-mlir-scf Author: Matthias Springer (matthias-springer) ChangesThis function went through a series of refactorings, most notably #140961. In the current version, there is no case where the function could return Full diff: https://github.com/llvm/llvm-project/pull/163749.diff 10 Files Affected:
diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h
index 18349d071bb2e..89f570a7c7115 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 43b699817927c..58ca187eec360 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 @@ class PredicateBackwardSliceMatcher {
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 12dff19ed31d3..ac9c96e5dd855 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 ®ion) {
@@ -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 98434357f826f..417b6bb3030d6 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 9436f1c6cd9b0..5e28306e00935 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 2a857eddbb932..efba956990697 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 29b770fb4b279..44bdfc28ef8ee 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 31ae1d1895b81..d00e137124a4e 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 2c93991386675..f26058f30ad7b 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 5a5ac450f91fb..95db70ec7a815 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);
|
| getBackwardSliceImpl(parentOp, visited, backwardSlice, options); | ||
| } | ||
| } else { | ||
| return failure(); |
There was a problem hiding this comment.
Note that this case can never happen: An SSA value is always either an OpResult or a BlockArgument.
| return success(); | ||
| }; | ||
|
|
||
| bool succeeded = true; |
There was a problem hiding this comment.
Note: This variable is always true.
matthias-springer
requested review from
ftynse,
hanhanW,
joker-eph and
wsmoses
MaheshRavishankar
requested changes
Oct 16, 2025
|
We have to think about the conceptual aspect of this API: do we intend for this to be a "failable" operation? This is independent of the current implementation and more of a conceptual design question. |
Agreed, but this is the case right now. So I would assume people are either ignoring the error, or actually propagating the error. If we ever need to put back a return of |
|
I don't have context that when it'd fail. In the previous revision, @wsmoses pointed that it could fail. Maybe @wsmoses can provide an example that helps the decision?
The PR was landed without tests. Therefore, I'm +1 on reverting the changes, although it's been like ~0.5 year. I think we all asked for tests in other PRs, but we did not get answers. See question 0 and question 1. The landed PR and a post fix are:
I'm more comfortable if we revert the changes and reland them with proper tests. It looks like the missing test is required in the discussion. |
|
Paging all this back in, but trying to put together what's happening: so the current state of the code here will not result in the crash as it has been changed since after my original PR, it seems (though in a way that perhaps changes behavior?). Specifically the prior code was something like: the cause of the failure was the assertion above, where the blockargument's parent had more than one region. my PR changed this to instead return failure in the case that the assertion would have failed, instead looking like: The current code (prior to this PR, presumably changed between my PR and now), however, will never hit the failure, instead silently not adding anything if that assertion-case isn't hit during the blockargument, looking like as follows. so indeed presently in the implementation this isn't ever expected to fail. I'm not sufficiently familiar presently with what the analysis is doing to assess whether the move of the assertion to the if statement without failure is legal (or if it is a correctness problem). However, considering the comment of: remains, that's at least mildly concerning from a correctness standpoint. It looks like the correctness condition was accidentally removed here (#142076) and there is an unmerged PR here, to restore the correctness condition from earlier (#142223) If the correctness condition isn't needed and indeed there is no way this can fail, I'm fine removing the logicalresult. However, if it is as it appears that there are parts of backwards slice analysis marked as a TODO and unimplemented (resulting in a failure of the analysis), I would advocate for leaving the logicalresult. |
|
It's been a sufficiently long time since I've looked at this code (and wasn't an expert in it to begin with, just was trying to fix a downstream crash), that I'm fine with deferring to you all with any solution that never crashes in the analysis for valid MLIR code (including if not currently handled per that todo), and is correct. |
|
I think the next step is to figure out what to do with this TODO: // 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->contains(parentOp)) {
if (!parentOp->hasTrait<OpTrait::IsIsolatedFromAbove>() &&
parentOp->getNumRegions() == 1 &&
parentOp->getRegion(0).hasOneBlock()) {
return getBackwardSliceImpl(parentOp, visited, backwardSlice,
options);
}
}Would this indicate a potential failure condition? I'm having trouble understanding why the code is written like that today.
It feels like the handling of block argument was designed with implicit assumptions from downstream projects. Maybe the entire block argument handling should be delegated to a lambda in |
I didn't fully agree with keeping the check for
@ftynse might do a better job at explaining. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This function went through a series of refactorings, most notably #140961. In the current version, there is no case where the function could return
failure. It always returnssuccess. This PR drops the return value to avoid API bloat.Note for LLVM integration: Simply drop and ignore the return value of
getBackwardSlice.