Use NNPACK for strided convolutions. #29084
Conversation
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@AshkanAliabadi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@AshkanAliabadi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
aten/src/ATen/native/NNPACK.cpp
Outdated
There was a problem hiding this comment.
I didn't trace into the details but looks like nnp_convolution_output() might be able to do better batching than for-loop, or it's equivalent?
Guess it's not a big deal for mobile use case as batch size is usually small (1)?
There was a problem hiding this comment.
Good point, I'll modify.
aten/src/ATen/native/NNPACK.cpp
Outdated
There was a problem hiding this comment.
Right, good catch.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@AshkanAliabadi is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Use NNPACK for strided convolutions. Pull Request resolved: pytorch/pytorch#29084 Differential Revision: D18286473 Pulled By: AshkanAliabadi fbshipit-source-id: accdfafa2c247f2750208a7af84c9e2c0374920b
|
This PR broke backcompat tests :( : https://app.circleci.com/jobs/github/pytorch/pytorch/3549012 |
|
@houseroad what's the best thing to do here? Revert? Whitelist this op? |
|
@AshkanAliabadi merged this pull request in 5ba9209. |
|
Let's first unloand it to make sure we either make it backward compatible, or no existing serialized models used this op. |
|
This PR gives a 20-ish% performance boost for ResNet50 on mobile. NNPACK does not provide a backwards path for strided convolutions so this is the only way I am aware of that this can be implemented so backwards compatibility may not be possible. How should I check this is not used in any serialized model? |
houseroad
left a comment
houseroad
left a comment
There was a problem hiding this comment.
Sure, I am not saying this PR is not important. My point is that breaking CI is not good. Especially, IMO, there is a way to make the new API to be compatible with existing API. If we break the BC CI, 1) exiting models may break, 2) others may be confused/blocked by the noise from the broken CI.
| use_c10_dispatcher: full | ||
|
|
||
| - func: _nnpack_spatial_convolution(Tensor input, Tensor weight, Tensor? bias, int[2] padding) -> Tensor | ||
| - func: _nnpack_spatial_convolution(Tensor input, Tensor weight, Tensor? bias, int[2] stride, int[2] padding) -> Tensor |
There was a problem hiding this comment.
you are changing the signature in a backward incompatible way.
Can you change it in the following way?
_nnpack_spatial_convolution(Tensor input, Tensor weight, Tensor? bias, int[2] padding, int[2]? stride=None) -> Tensor
The new API is backward compatible with the existing API.
There was a problem hiding this comment.
Definitely. Thanks for your feedback. Just to be sure, should I unland this CR first, and update this PR to contain that fix and resubmit, or, create a new patch that contains your proposed fix only and apply that to master?
There was a problem hiding this comment.
I have already unlanded the PR and fixed the BC CI.
Please resubmit the PR and make the new aten function backward compatible. If you need any help on BC, feel free to ping me.
6 participants
Use NNPACK for strided convolutions.