[AMD] getBackwardSlice variant with handling for op regions #4993
Conversation
karthik-man
force-pushed
the
backward_slice
branch
3 times, most recently
from
54ebd43 to
307d809
Compare
mlir::getBackwardSlice does not handle op regions. This can cause the backward slice to not be in topological order and this can result in the reordering pass moving a value's use before its def. This is a temporary local fix until these changes are upstreamed to mlir.
karthik-man
force-pushed
the
backward_slice
branch
from
307d809 to
0b962d9
Compare
| @@ -24,6 +25,73 @@ static bool isLocalLoadOrDotLayoutConversion(Operation *op) { | |||
| return false; | |||
| } | |||
|
|
|||
| // Copy of mlir::getBackwardSlice with changes to handle nested regions. | |||
| // This is a temporary local fix until these changes are upstreamed to mlir. | |||
| static void getDeepBackwardSlice(Operation *op, | |||
There was a problem hiding this comment.
Consider making this fix to the MLIR code base, perhaps through BackwardSliceOptions?
There was a problem hiding this comment.
Yeah BackwardSliceOptions will be a good place to handle this variant.
There was a problem hiding this comment.
Yeah we should consider landing such general changes to upstream mlir. Then we can update llvm submodule to bring it in.
| @@ -24,6 +25,73 @@ static bool isLocalLoadOrDotLayoutConversion(Operation *op) { | |||
| return false; | |||
| } | |||
|
|
|||
| // Copy of mlir::getBackwardSlice with changes to handle nested regions. | |||
| // This is a temporary local fix until these changes are upstreamed to mlir. | |||
| static void getDeepBackwardSlice(Operation *op, | |||
There was a problem hiding this comment.
Yeah BackwardSliceOptions will be a good place to handle this variant.
| }; | ||
|
|
||
| // collect all the values used in the nested regions of this op | ||
| // SetVector<Region*> nestedRegions; |
There was a problem hiding this comment.
Should this line be removed?
Also it will be good to mark which part is different from mlir::getBackwardSlice.
There was a problem hiding this comment.
Thanks! I think the improvement should happen in upstream mlir first. Also, we are looking at to avoid doing complicated reordering with #4881; so this might be resolved the other simpler way.
| @@ -24,6 +25,73 @@ static bool isLocalLoadOrDotLayoutConversion(Operation *op) { | |||
| return false; | |||
| } | |||
|
|
|||
| // Copy of mlir::getBackwardSlice with changes to handle nested regions. | |||
| // This is a temporary local fix until these changes are upstreamed to mlir. | |||
| static void getDeepBackwardSlice(Operation *op, | |||
There was a problem hiding this comment.
Yeah we should consider landing such general changes to upstream mlir. Then we can update llvm submodule to bring it in.
|
Closing this given superseded by #5363. |
mlir::getBackwardSlice does not traverse through op regions. This can cause the backward slice to not be in topological order and this can result in the reordering pass moving a value's use before its def.
Example:
%0 op0 ...
%1 scf.if %arg0
{
%2 op1 %0, ...
}
%3 op2 %0,
%4 tt.load %1, %3
while gathering the backward slice of the tt.load, if op1's use of %0 is not seen during the post-order walk in mlir::getBackwardSlice, then the backward slice can have op0 after the scf.if. Global load reordering relies on the load's backward set being topologically sorted to move the load and its transitive dependencies.
This PR adds a getBackwardSlice variant with handling for op regions. This is a temporary fix until these changes are upstreamed to mlir::getBackwardSlice.
The core Triton is a small number of people, and we receive many PRs (thank
you!). To help us review your code more quickly, if you are a new
contributor (less than 3 PRs merged) we ask that you complete the following
tasks and include the filled-out checklist in your PR description.
Complete the following tasks before sending your PR, and replace
[ ]with[x]to indicate you have done them.I am not making a trivial change, such as fixing a typo in a comment.
I have written a PR description following these
rules.
I have run
pre-commit run --from-ref origin/main --to-ref HEAD.Select one of the following.
/testforlittests/unittestfor C++ tests/python/testfor end-to-end testsFILL THIS IN.Select one of the following.
littests.littests I have added follow these best practices,including the "tests should be minimal" section. (Usually running Python code
and using the instructions it generates is not minimal.)