[mlir] Recover the behavior of SliceAnaylsis for llvm-project@6a8dde04a07 #142076
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
In llvm@6a8dde0, it changes the method to return LogicalFailure, so callers can handle the failure instead of crashing, if I read the intention correctly. However, it changes the behavior of the implementation. Before the change, processValue does not treat it as a failure if the check below TODO has a false condition. However, with the new change, it starts treating it as a failure. The revision updates the final `else` branch to return a failure, and return success at the end; the behavior is recovered. ```cpp auto processValue = [&](Value value) { if (auto *definingOp = value.getDefiningOp()) { if (backwardSlice->count(definingOp) == 0) getBackwardSliceImpl(definingOp, backwardSlice, options); } else if (auto blockArg = dyn_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) { assert(parentOp->getNumRegions() == 1 && llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())); getBackwardSliceImpl(parentOp, backwardSlice, options); } } else { llvm_unreachable("No definingOp and not a block argument."); } ``` No additional tests are added like the previous commit. This revision is mostly a post-fix for llvm@6a8dde0 Signed-off-by: hanhanW <[email protected]>
|
@llvm/pr-subscribers-mlir Author: Han-Chung Wang (hanhanW) ChangesIn 6a8dde0, it changes the method to return LogicalFailure, so callers can handle the failure instead of crashing, if I read the intention correctly. However, it changes the behavior of the implementation. Before the change, processValue does not treat it as a failure if the check below TODO has a false condition. However, with the new change, it starts treating it as a failure. The revision updates the final auto processValue = [&](Value value) {
if (auto *definingOp = value.getDefiningOp()) {
if (backwardSlice->count(definingOp) == 0)
getBackwardSliceImpl(definingOp, backwardSlice, options);
} else if (auto blockArg = dyn_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) {
assert(parentOp->getNumRegions() == 1 &&
llvm::hasSingleElement(parentOp->getRegion(0).getBlocks()));
getBackwardSliceImpl(parentOp, backwardSlice, options);
}
} else {
llvm_unreachable("No definingOp and not a block argument.");
}No additional tests are added, like the previous commit. This revision is mostly a post-fix for 6a8dde0 Full diff: https://github.com/llvm/llvm-project/pull/142076.diff 1 Files Affected:
diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp
index c4eda71c42f3e..4cdf47a405c01 100644
--- a/mlir/lib/Analysis/SliceAnalysis.cpp
+++ b/mlir/lib/Analysis/SliceAnalysis.cpp
@@ -111,8 +111,10 @@ static LogicalResult getBackwardSliceImpl(Operation *op,
return getBackwardSliceImpl(parentOp, backwardSlice, options);
}
}
+ } else {
+ return failure();
}
- return failure();
+ return success();
};
bool succeeded = true;
|
|
yeah fair catch -- and yes that should've been a commit which didn't change behavior. |
wsmoses
approved these changes
May 30, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/13875 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/35/builds/10510 Here is the relevant piece of the build log for the reference |
WillFroom
reviewed
May 30, 2025
| @@ -111,8 +111,10 @@ static LogicalResult getBackwardSliceImpl(Operation *op, | |||
| return getBackwardSliceImpl(parentOp, backwardSlice, options); | |||
| } | |||
There was a problem hiding this comment.
Should this else return a failure? (to reflect the original assert?)
|
Yeah looking at this again this patch is wrong and indeed it should return a failure there |
|
fix in #142223 |
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…4a07 (llvm#142076) In llvm@6a8dde0, it changes the method to return LogicalFailure, so callers can handle the failure instead of crashing, if I read the intention correctly. However, it changes the behavior of the implementation; it breaks several integratino tests in downstream projects (e.g., IREE). Before the change, processValue does not treat it as a failure if the check below TODO has a false condition. However, with the new change, it starts treating it as a failure. The revision updates the final `else` branch (i.e., `llvm_unreachable` line) to return a failure, and return success at the end; the behavior is recovered. ```cpp auto processValue = [&](Value value) { if (auto *definingOp = value.getDefiningOp()) { if (backwardSlice->count(definingOp) == 0) getBackwardSliceImpl(definingOp, backwardSlice, options); } else if (auto blockArg = dyn_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) { assert(parentOp->getNumRegions() == 1 && llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())); getBackwardSliceImpl(parentOp, backwardSlice, options); } } else { llvm_unreachable("No definingOp and not a block argument."); } ``` No additional tests are added, like the previous commit. This revision is mostly a post-fix for llvm@6a8dde0 Co-authored-by: Ian Wood <[email protected]> Signed-off-by: hanhanW <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In 6a8dde0, it changes the method to return LogicalFailure, so callers can handle the failure instead of crashing, if I read the intention correctly. However, it changes the behavior of the implementation; it breaks several integratino tests in downstream projects (e.g., IREE).
Before the change, processValue does not treat it as a failure if the check below TODO has a false condition. However, with the new change, it starts treating it as a failure.
The revision updates the final
elsebranch (i.e.,llvm_unreachableline) to return a failure, and return success at the end; the behavior is recovered.No additional tests are added, like the previous commit. This revision is mostly a post-fix for 6a8dde0
Co-authored-by: Ian Wood [email protected]