[DispatchCreation] Extend multi-use producer fusion #18551
Conversation
Fuse even in cases where the most dominant op isn't fusable but other operations would be legal to fuse. Signed-off-by: Ian Wood <[email protected]>
|
Maybe that could be added upstream, but assuming worst case scenario works (only really effects gather ops) |
Signed-off-by: Ian Wood <[email protected]>
Signed-off-by: Ian Wood <[email protected]>
IanWood1
force-pushed
the
multi_use_fusion
branch
from
fbfcb84
to
fbbcd12
Compare
IanWood1
requested review from
hanhanW,
MaheshRavishankar and
ScottTodd
as code owners
compiler/src/iree/compiler/DispatchCreation/FuseMultiUseElementwiseProducer.cpp
Outdated
Show resolved
Hide resolved
| llvm::SetVector<Operation *> slice; | ||
| getBackwardSlice(op, &slice, options); | ||
|
|
||
| // `getBackwardSlice` doesnt track uses from within an ops region, so make |
There was a problem hiding this comment.
Not sure I follow this comment. Can you explain more?
There was a problem hiding this comment.
%0 = linalg.generic
%1 = linalg.generic %0
%2 = linalg.generic %1 ins(%0 : tensor<...>) {
^bb0(%in: i64, %out: i64):
%extracted = tensor.extract %1 [...]
linalg.yield %extracted : f16
} -> tensor<...>In the above, the backward slice starting at %2 would only include %2 (but not %1 because it isn't a direct operand). Which makes it seem like %2 can be moved before %1. Just looking at the slice doesn't tell you if there is a dependency between the ops.
This was causing issues with the open llama regression tests since (i think) there are multiple gathers
There was a problem hiding this comment.
Do we need to use a slice of the values that are captured from above to make sure that there is no dependence?
There was a problem hiding this comment.
Yeah I think so. Maybe this could be a option of getBackwardsSlice?
There was a problem hiding this comment.
Maybe... lets leave this this way for now.
There was a problem hiding this comment.
If I am seeing the test added in this PR correctly there wasnt a test added for this? The failure in #18879 seems related to this.
There was a problem hiding this comment.
@nirvedhmeshram There were several other tests that failed related to the backward slice problem (regression tests), but none were because the consumer was using values defined above which is why this slipped through. I'll make sure to add more testing when fixing this
Oh I think I forgot to update the description. There is a commit here originally from #18341 but since thats waiting for bangtian's multi reduction pipeline change and this was easily seperatable I just moved it's own PR. #18341 allows more movement of reshape ops which changed the order of some of the consumers. For example: Before:%producer = linalg.generic
%fusableConsumer = linalg.generic
%unfusableConsumer = expand_shape %producerAfter:%producer = linalg.generic
%unfusableConsumer = expand_shape %producer
%fusableConsumer = linalg.genericThis caused regressions in dispatch count because |
Signed-off-by: Ian Wood <[email protected]>
Signed-off-by: Ian Wood <[email protected]>
| llvm::SetVector<Operation *> slice; | ||
| getBackwardSlice(op, &slice, options); | ||
|
|
||
| // `getBackwardSlice` doesnt track uses from within an ops region, so make |
There was a problem hiding this comment.
Maybe... lets leave this this way for now.
The reverted commit does not handle when the "consumer" uses a value defined above. See #18879 for the original issue. This is causing issue with ~15 onnx models. I have a PR (#18855) to fix this by including values used in an ops region in the backwards slice, but It is waiting on upstream changes to `getBackwardSlice`. Currently, the PR is using a wrapper around `getBackwardSlice` to acheive the same effect, but this will be updated once the upstream change lands (llvm/llvm-project#113478) Reverts #18551 --------- Signed-off-by: Ian Wood <[email protected]>
The reverted commit does not handle when the "consumer" uses a value defined above. See #18879 for the original issue. This is causing issue with ~15 onnx models. I have a PR (#18855) to fix this by including values used in an ops region in the backwards slice, but It is waiting on upstream changes to `getBackwardSlice`. Currently, the PR is using a wrapper around `getBackwardSlice` to acheive the same effect, but this will be updated once the upstream change lands (llvm/llvm-project#113478) Reverts #18551 --------- Signed-off-by: Ian Wood <[email protected]> Signed-off-by: Elias Joseph <[email protected]>
Fuse even in cases where the most dominant op isn't fusable but other operations would be legal to fuse.