-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[mlir][linalg] Implement Conv2D using Winograd Conv2D algorithm #96181
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
Merged
+943
−0
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
4240341
[mlir][linalg] Implement Conv2D using Winograd Conv2D algorithm
Hsiangkai bbb6542
Address ftynse's comments
Hsiangkai db8e7e7
Address Max191's comments
Hsiangkai f018ec0
Add more tests in Linalg/roundtrip.mlir and Linalg/invalid.mlir
Hsiangkai 11a4ee2
Address more comments
Hsiangkai 65883da
Consider dynamic shapes in verify functions
Hsiangkai 97329fa
use ShapedType::kDynamic
Hsiangkai 4a46b7b
Merge branch 'main' into users/hsiangkai/winograd-ops
Hsiangkai File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
These verifiers will not work for dynamic shapes. Can you support dynamic cases? The transform is only supported for static shapes right now, but shapes can become dynamic when tiling.
You can create an expected output shape from the input, allowing dynamic dims, and compare with the actual output shape. This helper may be useful:
llvm-project/mlir/lib/IR/TypeUtilities.cpp
Line 58 in 5861145
This way will also make it easy to check that the batch/channel dimensions match for the input and output.
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 we support static shapes in the upstream first? I added a TODO for dynamic cases.
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 has a tendency to not get done and not having the shape handling can mask other issues. Usually it's better to do it right, do it once.
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 your review. I updated the
verify()
functions to consider dynamic shapes. I also added test cases for dynamic shapes.