Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions mlir/include/mlir/Analysis/SliceAnalysis.h
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ void getForwardSlice(Value root, SetVector<Operation *> *forwardSlice,
///
/// This function returns whether the backwards slice was able to be
/// successfully computed, and failure if it was unable to determine the slice.
/// This function will presently return failure if a value to process is a blockargument
/// whose parent op has more than one region, or a region with more than one block.
LogicalResult getBackwardSlice(Operation *op,
SetVector<Operation *> *backwardSlice,
const BackwardSliceOptions &options = {});
Expand Down
2 changes: 2 additions & 0 deletions mlir/lib/Analysis/SliceAnalysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ static LogicalResult getBackwardSliceImpl(Operation *op,
if (parentOp->getNumRegions() == 1 &&
llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())) {
return getBackwardSliceImpl(parentOp, backwardSlice, options);
} else {
return failure();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be tested?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, I was missing one too in the original. Thanks for updating the comment.

So this just fails due to missing support in getBackwardSlice? E.g., its difficult to determine that to slice and semantics of ops in this case and so just bail. An alternative would have been to have the filter function capture this (fail to propagate post out into such ops and record the need/failure, and handle post - less efficient than this early exit but there are just a lot of ignoring of this return now).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm somehow missed these comments/review. rebased the PR.

I'm not quite sure how to test this as the condition here is restore a broken condition from #142076 to ensure the analysis is conservative.

Happy to add a test if you know a good way to add one.

That said given #163749 it may be prudent to add this to restore correctness

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're looking for an operation which is not IsolatedFromAbove, have multiple regions or a region with multiple blocks?

}
}
} else {
Expand Down
Loading