[MLIR] getBackwardSlice: don't bail on ops that are IsolatedFromAbove #158135
Conversation
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
|
@llvm/pr-subscribers-mlir Author: Ian Wood (IanWood1) ChangesOps with the Full diff: https://github.com/llvm/llvm-project/pull/158135.diff 1 Files Affected:
diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp
index 7037fa644c7be..d0e10626589ce 100644
--- a/mlir/lib/Analysis/SliceAnalysis.cpp
+++ b/mlir/lib/Analysis/SliceAnalysis.cpp
@@ -109,7 +109,7 @@ static LogicalResult getBackwardSliceImpl(Operation *op,
DenseSet<Operation *> &visited,
SetVector<Operation *> *backwardSlice,
const BackwardSliceOptions &options) {
- if (!op || op->hasTrait<OpTrait::IsIsolatedFromAbove>())
+ if (!op)
return success();
// Evaluate whether we should keep this def.
|
|
Tests are here https://github.com/llvm/llvm-project/blob/6ab2b8745156269024de9098a4a6495ef19d546e/mlir/test/Transforms/move-operation-deps.mlir but I don't know of a good op to use in a testcase. I don't think its possible to give unregistered ops traits. |
|
Are you able to use some Test ops? E.g., llvm-project/mlir/test/lib/Dialect/Test/TestOps.td Lines 553 to 559 in 2f9a458 |
There was a problem hiding this comment.
This was intended as a recursion stopping condition. When getBackwardSliceImpl hits an isolated-from-above op, it doesn't need to go any further because there cannot be any values in the backward slice due to isolation. I see that this unintentionally prevents this from computing the slice of an operation that is itself isolated from above but may have operands. We shouldn't remove the original behavior though, but move the check to happen before recursive calls and not enter the recursion instead.
If it is isolated we should still add it to the slice and traverse the operands. However, the trait tells us that the op's regions don't access any values outside. So, do you mean the check should be moved here: |
|
I was thinking here (but not here IIUC) so we don't recurse into this function for an isolated-from-above parent of the current, but we include the isolated-from-above defining op and continue to its operands.Incidentally, we shouldn't be recursing on the use-def chain because those tend to be long and stack tends to be small, but that is a follow-up. |
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
|
Does isolated from above carry any other meaning than the ops regions don't use values outside of the region? For example if you assume %0 = op
%1 = op_with_region %0 {
^bb0(%in):
%2 = op_in_body %in
}I think that it would make sense to include all 3 ops even if it |
|
I think the issue is this https://github.com/llvm/llvm-project/pull/158135/files#diff-6e5b7f6ec0083b327b4c9960d81df86b74c89d2b51d91784d6d979c0c6031c78R200 . Because of that, the operation could be either Maybe we just check for |
This is a correct analysis of MLIR semantics: whether it is a block argument of implicitly captured shouldn't make a difference. |
Strictly speaking, it does not. Now, the interpretation of that for the purpose of backward slice may differ. The interpretation adopted so far is that since values from outside the region cannot be used inside the region, they cannot directly affect any value/operation inside the region, and therefore should not be included in the slice. This is based on the This PR seems to adopt a different interpretation and try to account for a potential forwarding of the operands of the operation with the
Exactly. What I suggest is to at least parameterize the behavior of (b). I'd be also curious to see the practical use case this comes from, e.g., an isolated operation where we want to see isolated values in the slice somehow. What are you doing with them? I'd just take the slice of the isolated operation...
This code goes down, not up. |
Or, rather, the operation that defines |
|
I agree with, lets "silo" the use of backward slice in the case of hitting block arguments, but I think Ian's fix here is the right one. When you encounter a block argument whether you hit it from within an operation that is isolated from above or not should not make a difference. So this PR seems fine as is. |
Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
I think case about block arguments is a bit of a side-issue to my goal for this PR. I'm just trying to fix |
| if (!options.omitUsesFromAbove) { | ||
| if (!options.omitUsesFromAbove && | ||
| !op->hasTrait<OpTrait::IsIsolatedFromAbove>()) { | ||
| llvm::for_each(op->getRegions(), [&](Region ®ion) { |
There was a problem hiding this comment.
Note for reviewers: this line change is an NFC since if the op is isolated from above there can't be any uses defined above.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/18719 Here is the relevant piece of the build log for the reference |
#22088) This can now be landed after llvm/llvm-project#158135 has been integrated. This was reverted due to #21889 Original PR #21894 Fixes #21889 Signed-off-by: Ian Wood <ianwood@u.northwestern.edu>
Ops with the
IsIsolatedFromAbovetrait should be captured by the backward slice.