-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[MLIR][SliceAnalysis] Add comment and restore failure condition #142223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
90167b5
1a578c9
0c81ad6
d81f2df
0c08827
1bf1cb4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be tested? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
Uh oh!
There was an error while loading. Please reload this page.