-
Notifications
You must be signed in to change notification settings - Fork 645
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
[Dispatch] Disable scatter fusion with producers #19565
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this can we just disable this during dispatch region formation
Backends don't currently support scatter fusion and will silently compile incorrect code. This should be turned off in order to prevent backends from generating incorrect results. Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu>
I made the requested change. Additionally, instead of removing the tests, I modified them to check that no fusion occurs |
Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu>
@@ -651,7 +651,8 @@ isFusableWithProducer(OpOperand &operand, | |||
} | |||
|
|||
// Don't fuse attention with it's producer | |||
if (isa<IREE::LinalgExt::AttentionOp>(consumer)) { | |||
// TODO: Enable scatter fusion when supported by backends. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backends don't currently support scatter fusion and will silently compile incorrect code.
Is there an issue for the TODO or your comment in the PR description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found an issue about fusion (#19091), but not an issue for correctness. I mainly wonder if we should allocate someone from codegen side to fix the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think either @qedawkins or @MaheshRavishankar were going to look at this at some point. But I don't think there is a dedicated github issue for the correctness problem.
Backends don't currently support scatter fusion and will silently compile incorrect code. This should be turned off in order to prevent backends from generating incorrect results. I don't think any users are running into this currently, but its best to keep it off for now. Similar to #19535 but for both
indices
andupdates
.