Skip to content
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

Add pattern to convert generic conv ops to IGEMM #19798

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

nirvedhmeshram
Copy link
Contributor

@nirvedhmeshram nirvedhmeshram commented Jan 23, 2025

This PR removes the named op patterns to convert convs to IGEMM and replaces them with a generic pattern that works for all supported convs. A new utility function that populates the shared details required for setting lowering config and doing the IGEMM computation is added.
The PR is currently using a default true flag iree-gpu-use-tile-and-fuse-generic-convolution . The idea is that since a lot more convolutions will go down the IGEMM path with this PR if any of them run into issues we can turn the flag off by default rather then needing to revert the whole PR. If after some time we find that there are no issues then we can drop the flag and have this happening always.

@nirvedhmeshram nirvedhmeshram force-pushed the generic_conv_igemm_final branch 2 times, most recently from 22ed0c0 to c94c8b6 Compare January 24, 2025 02:24
Copy link
Contributor

@Max191 Max191 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looked at the tests so far, but nice work!

@nirvedhmeshram nirvedhmeshram force-pushed the generic_conv_igemm_final branch 3 times, most recently from f70dc62 to afcd4e5 Compare January 28, 2025 22:25
@nirvedhmeshram nirvedhmeshram marked this pull request as ready for review January 28, 2025 22:43
@nirvedhmeshram nirvedhmeshram force-pushed the generic_conv_igemm_final branch from afcd4e5 to 5f8ffad Compare January 28, 2025 23:27
@nirvedhmeshram
Copy link
Contributor Author

@Max191 @jerryyin This PR is now ready for review.

@nirvedhmeshram nirvedhmeshram requested a review from Max191 January 28, 2025 23:32
Copy link
Contributor

@Max191 Max191 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great, nice work Nirvedh!

Mostly just some nits/small suggestions.

@nirvedhmeshram nirvedhmeshram force-pushed the generic_conv_igemm_final branch from 80af6ec to b86e844 Compare January 29, 2025 22:25
@nirvedhmeshram nirvedhmeshram merged commit 50a7087 into iree-org:main Jan 29, 2025
41 checks passed
ita9naiwa pushed a commit to ita9naiwa/iree that referenced this pull request Feb 4, 2025
This PR removes the named op patterns to convert convs to IGEMM and
replaces them with a generic pattern that works for all supported convs.
A new utility function that populates the shared details required for
setting lowering config and doing the IGEMM computation is added.
The PR is currently using a default true flag
`iree-gpu-use-tile-and-fuse-generic-convolution` . The idea is that
since a lot more convolutions will go down the IGEMM path with this PR
if any of them run into issues we can turn the flag off by default
rather then needing to revert the whole PR. If after some time we find
that there are no issues then we can drop the flag and have this
happening always.

---------

Signed-off-by: Nirvedh Meshram <[email protected]>
Signed-off-by: Hyunsung Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants