-
Notifications
You must be signed in to change notification settings - Fork 609
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
Collapse dims when producer is unpack op #17725
Conversation
Abbreviated Benchmark Summary@ commit 793e4095b4558c71409842768805334c6cf56f1d (no previous benchmark results to compare) Data-Tiling Comparison TableClick to show
Raw Latencies
[Top 3 out of 92 results showed] No improved or regressed compilation metrics 🏖️ For more information: |
b0942ff
to
307e134
Compare
I haven't looked at the code yet, but it does address the compilation issue for #17530. Thanks for pushing on this! |
@hanhanW do you mean the large dispatch sizes? I'm not sure currently since the benchmarks are having issues If you can point me to something reproducible locally I can check it out |
There are no issues. This PR is good to me because it fixes my issues. :) I need to check benchmark result though, but at least we can compile all the models in my prototype. |
I think your PR breaks some models targeting gfx90a: https://github.com/iree-org/iree/actions/runs/9665068317/job/26661780089?pr=17725 The model and input files and be found at https://github.com/nod-ai/SHARK-TestSuite/tree/main/iree_tests/pytorch/models/sdxl-vae-decode-tank |
Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu>
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.
LGTM, just one question
@@ -507,51 +507,6 @@ util.func public @input_broadcast(%arg0: tensor<4x8xf32>, %arg1: tensor<4xf32>) | |||
|
|||
// ----- | |||
|
|||
// Do nothing if the dispatch is not a single elementwise op (with tensor.empty/linalg.fill producers) |
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.
What's going on in the test? Why do we delete it?
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.
It was there to make sure that collapsing only happened when there was one operation in the dispatch region. I removed it because I was planning on being able to collapse this, but had to disable collapsing when there is a producer generic.
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.
Okay, added this test back. Will remove again in a later pr that actually performs collapse here
@@ -200,15 +227,6 @@ findRootGenericOp(DispatchRegionOp regionOp) { | |||
} | |||
} | |||
|
|||
// Check that the operands of the generic op are defined outside the dispatch. |
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.
[no action required]: I don't understand why we had the check. But the changes that you made look reasonable to me.
Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu>
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.
LGTM, thanks a lot!
#17594 --------- Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu>
iree-org#17594 --------- Signed-off-by: Ian Wood <ianwood2024@u.northwestern.edu> Signed-off-by: Lubo Litchev <lubol@google.com>
#17594