-
Notifications
You must be signed in to change notification settings - Fork 505
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 non-unit groups to aten::convolution #858
Conversation
90b6ef8
to
fb5b479
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.
Looking good! I have a few initial comments
0edf54c
to
f811ecd
Compare
e15c836
to
f6fb86b
Compare
f6fb86b
to
4129aa9
Compare
Okay, most recent commit is confirmed working with my new op in LLVM. Once that lands and we can push torch-mlir's LLVM version up this should be good to go. |
Adds my grouped conv2d op for #858
20e097e
to
56f69bd
Compare
Now that the LLVM op has landed, here's the updated PR using the new logic. I spoke with Mahesh about adding a depthwise case and my concerns that it won't be possible with dynamic tensors because I won't be able to do the necessary calculation at compile time. He suggested that the optimization to depthwise convolution could be done on the backend (i.e. IREE) side. If you think I should add a specific case for depthwise static tensors then I can do that, otherwise this is complete. |
I think we should support the special case when enough static info is available (the whole shape doesn't have to be static for us to detect it). It is a very important pattern in practice and we don't want all backends replicating that code. |
Okay, I've put in a PR at LLVM for the new op here: https://reviews.llvm.org/D128575. It's only for the case where the multiplier is 1, which covers Mobilenet. I can add the non-unit multiplier case if you think it's relevant, but for now I'm going to check for multiplier of 1 and fall through to the grouped op otherwise. |
1f030db
to
fa86ecb
Compare
Testing my patch with other models (i.e. not Mobilenet) exposed an issue in the ordering of the op I wrote in upstream. This latest commit fixes the issue, although it does require a change to the upstream op which I have put up at https://reviews.llvm.org/D128880. |
Adds my grouped conv2d op for llvm#858
fa86ecb
to
fd27ca8
Compare
Both LLVM patches have landed, now this is just waiting for an LLVM bump before it's good to go. |
Adds my grouped conv2d op for llvm#858
14e2710
to
8215edc
Compare
8215edc
to
c3d1e7a
Compare
…llvm#858) * shape inference Signed-off-by: Tong Chen <[email protected]> * modify test Signed-off-by: Tong Chen <[email protected]> * lower compiled Signed-off-by: Tong Chen <[email protected]> * neagive Signed-off-by: Tong Chen <[email protected]> * add backend test Signed-off-by: Tong Chen <[email protected]> * format Signed-off-by: Tong Chen <[email protected]> * polish Signed-off-by: Tong Chen <[email protected]> * try Signed-off-by: Tong Chen <[email protected]> * modify dynamic Signed-off-by: Tong Chen <[email protected]> * check dim Signed-off-by: Tong Chen <[email protected]> * dynamic axes Signed-off-by: Tong Chen <[email protected]> * noop_empty_axes Signed-off-by: Tong Chen <[email protected]> * lit test Signed-off-by: Tong Chen <[email protected]> * fix test Signed-off-by: Tong Chen <[email protected]> * format Signed-off-by: Tong Chen <[email protected]> * restrict constant Signed-off-by: Tong Chen <[email protected]> Co-authored-by: Tung D. Le <[email protected]>
Adds support for group sizes > 1 for at::convolution (and by extension the ops that decompose into it).