[DispatchCreation] Extend multi-use producer fusion#18551
[DispatchCreation] Extend multi-use producer fusion#18551IanWood1 merged 7 commits intoiree-org:mainfrom
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 <ianwood2024@u.northwestern.edu>
|
Maybe that could be added upstream, but assuming worst case scenario works (only really effects gather ops) |
Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu>
Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu>
IanWood1
force-pushed
the
multi_use_fusion
branch
from
fbfcb84 to
fbbcd12
Compare
IanWood1
requested review from
MaheshRavishankar,
ScottTodd and
hanhanW
as code owners
MaheshRavishankar
left a comment
MaheshRavishankar
left a comment
There was a problem hiding this comment.
Could you provide some more context on this change. Just trying to place what this is addressing.
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 <ianwood2024@u.northwestern.edu>
Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu>
| 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 <ianwood2024@u.northwestern.edu>
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 <ianwood2024@u.northwestern.edu> Signed-off-by: Elias Joseph <eljoseph@amd.com>
…g#18917) The reverted commit does not handle when the "consumer" uses a value defined above. See iree-org#18879 for the original issue. This is causing issue with ~15 onnx models. I have a PR (iree-org#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 iree-org#18551 --------- Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu> Signed-off-by: Giacomo Serafini <179146510+giacs-epic@users.noreply.github.com>
4 participants
Fuse even in cases where the most dominant op isn't fusable but other operations would be legal to fuse.