-
Notifications
You must be signed in to change notification settings - Fork 663
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
Integrate LLVM to llvm/llvm-project@3e61c1ab7f #20015
Conversation
409a354
to
40104fc
Compare
f3067dc
to
c24ba9e
Compare
Signed-off-by: Alan Li <[email protected]>
c24ba9e
to
79e983c
Compare
// CHECK: tensor.pack | ||
// CHECK: tensor.unpack | ||
// CHECK: linalg.pack | ||
// CHECK: linalg.unpack |
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.
Can you please call out large changes like these in the PR description with some context? A link to the upstream commit (llvm/llvm-project@517800e, https://github.com/llvm/llvm-project/commit/517800e37e8d3a4ee84214bef65e227612c2a98b
) would be ideal.
We don't have a PR template for LLVM integrates but here's what we usually look for:
-
Which upstream commit was chosen? This is partly in the PR title already, but you have save others some clicking by putting a link such as llvm/llvm-project@3e61c1ab7f (
https://github.com/llvm/llvm-project/commit/3e61c1ab7f
) in the description. Bonus points for also making the PR title include the full form likellvm/llvm-project@3e61c1ab7f
, so commit titles on GitHub create a link too (https://github.com/iree-org/iree/commits/main/third_party):
-
Are there any cherry-picks or reverts carried on our fork at https://github.com/iree-org/llvm-project, or is the integrate cleanly pointing to an upstream https://github.com/llvm/llvm-project commit?
-
What changes were noteworthy and needed changes to remain compatible? (at least the tensor->linalg change here)
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.
Ahh didn't know that you can save a click by putting llvm/llvm-project@3e61c1ab7f
in the title. Thanks for the tip.
I also updated the PR title and the descriptions.
// Cases of trivial pack/unpack should be handled as canonicalizations | ||
// before we get here, thus we're safe to always hoist. |
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.
Nit: this comment should be moved up to the linalg section since the pack/unpack ops are no longer in tensor.
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 even the name HoistableLinalgOpInterfaceHelper
needs to be updated as well because it is no longer linalg specific.
[MLIR] Fix rewrite of ops with vector operands to LLVM on GPU. llvm/llvm-project#127844 Previously cherry-picked in #20031 and then dropped in #20015. Needed by the just-merged #19969. Signed-off-by: Benoit Jacob <[email protected]>
This PR contains the following change: