-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
[MLIR] Generalize expand_shape to take shape as explicit input #69267
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.
Ok, I think this is not looking too bad. Adding the helper build method here would make the diff much smaller I think.
11c8940
to
e8ac533
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
e8ac533
to
3961b9a
Compare
Hey, I left a few comments. Let me know when this is ready for review. |
3961b9a
to
3f96df1
Compare
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.
Next rounds of comments. Thanks!
dec4859
to
f9d0f60
Compare
@MaheshRavishankar Thanks for reviewing, have addressed all the comments, and is ready for re-review. |
f9d0f60
to
2e27433
Compare
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.
ok, this looks good to me. Lets test this with IREE. Ill let you know how to do that.
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.
Thanks for getting this across the finish line! LGTM
2e27433
to
273d71d
Compare
273d71d
to
24ee8aa
Compare
*DO NOT SUBMIT* This commit is to test and fix the bugs introduced by importing tensor.expand_shape changes in IREE. (check here llvm/llvm-project#69267) Signed-Off-by: Gaurav Shukla <[email protected]>
@MaheshRavishankar Could you please re-review it? There is still one failure in reshape_fusion(as discussed), meanwhile I can fix that. Thanks :) |
I added a comment that has not been addressed yet? |
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.
Thanks! Left a few mostly minor comments. Nothing that I see as blocker for landing.
76caac7
to
f352a94
Compare
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.
Ok, looks ogod to me. Left a single comment but it is not a blocker and is OK to do it after this PR as well since that part needs to be fixed after this PR lands.
yes please please do as follow-up and not lose the progress on (very slow) CI here :-) |
Windows CI failed with
Probably unrelated to this PR, but also not completely impossible that something about this PR increases compiler memory usage and pushes it over a threshold, so we need to get a green CI run before merging. Unfortunately I don't see a "retrigger" button here. Can you push an empty commit to this PR to cause a retrigger maybe? |
This patch generalizes tensor.expand_shape and memref.expand_shape to consume the output shape as a list of SSA values. This enables us to implement generic reshape operations with dynamic shapes using collapse_shape/expand_shape pairs. The output_shape input to expand_shape follows the static/dynamic representation that's also used in `tensor.extract_slice`. Differential Revision: https://reviews.llvm.org/D140821
f352a94
to
5ea9b9a
Compare
I think this broke the bot: https://lab.llvm.org/buildbot/#/builders/61/builds/57064 |
Reverted in #89540 |
…69267) This patch generalizes tensor.expand_shape and memref.expand_shape to consume the output shape as a list of SSA values. This enables us to implement generic reshape operations with dynamic shapes using collapse_shape/expand_shape pairs. The output_shape input to expand_shape follows the static/dynamic representation that's also used in `tensor.extract_slice`. Differential Revision: https://reviews.llvm.org/D140821 Co-authored-by: Ramiro Leal-Cavazos <[email protected]>
…t" (llvm#89540) Reverts llvm#69267 this broke some bots.
This patch generalizes tensor.expand_shape and memref.expand_shape to consume the output shape as a list of SSA values. This enables us to implement generic reshape operations with dynamic shapes using collapse_shape/expand_shape pairs.
The output_shape input to expand_shape follows the static/dynamic representation that's also used in
tensor.extract_slice
.Differential Revision: https://reviews.llvm.org/D140821