Skip to content

Refactor qconv to reduce allocations. #42007

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.

Sign up for GitHub

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

Jump to bottom
Closed
kimishpatel wants to merge 20 commits into from
Closed

Refactor qconv to reduce allocations. #42007

kimishpatel wants to merge 20 commits into from

Conversation

@kimishpatel
Copy link
Contributor

@kimishpatel kimishpatel commented Jul 24, 2020 •
edited
Loading

Stack from ghstack:

Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D22726972

@kimishpatel
Refactor qconv to reduce allocations.
0f04b4b 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

[ghstack-poisoned]
This was referenced Jul 24, 2020
Closed
Closed
Closed
@dr-ci
Copy link

dr-ci bot commented Jul 24, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit d1cd111 (more details on the Dr. CI page):

  • 2/2 failures possibly* introduced in this PR
    • 2/2 non-CircleCI failure(s)

Extra GitHub checks: 1 failed

ci.pytorch.org: 1 failed

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 77 times.

@kimishpatel
Update on "Refactor qconv to reduce allocations."
022ed3b 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel kimishpatel mentioned this pull request Jul 28, 2020
Closed
@kimishpatel
Update on "Refactor qconv to reduce allocations."
5a8c011 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
93e441e 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
51a5dbb 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
11378a0 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel kimishpatel mentioned this pull request Aug 11, 2020
Closed
@kimishpatel
Update on "Refactor qconv to reduce allocations."
f6c0c8f 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
1c908e3 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
92b9371 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
cdad591 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
42cc357 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
510abb6 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
9e2b8f2 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
f9c417f 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
26f3e02 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel kimishpatel requested a review from supriyar August 18, 2020 16:08
AshkanAliabadi
AshkanAliabadi approved these changes Aug 18, 2020
View reviewed changes
aten/src/ATen/native/quantized/cpu/qnnpack/include/conv_utils.h
pytorch_qnnp_log_error(
"Per channel quantized weights are not supported for XZP kernels");
assert("Failed to initialize QNNPACK conv_param_t struct.");
}
Copy link
Contributor

@AshkanAliabadi AshkanAliabadi Aug 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Can this cause unexpected behavior if someone uses the XZP kernels? I know we are not currently using it but it would be safer to either remove it completely or leave it functional rather than an in-between state.

Copy link
Contributor Author

@kimishpatel kimishpatel Aug 18, 2020

Choose a reason for hiding this comment

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

Thanks for the catch. Strange I removed this. Will reinstate.

aten/src/ATen/native/quantized/cpu/qnnpack/src/conv-run.cc Outdated
@@ -1,4 +1,5 @@
#include <conv_utils.h>
#include <conv_utils.h>
Copy link
Contributor

@AshkanAliabadi AshkanAliabadi Aug 18, 2020

Choose a reason for hiding this comment

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

Redundant include

aten/src/ATen/native/quantized/cpu/qnnpack/src/conv-run.cc
input_pixel_stride,
output,
output_pixel_stride,
threadpool);
Copy link
Contributor

@AshkanAliabadi AshkanAliabadi Aug 18, 2020

Choose a reason for hiding this comment

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

Is the logic removed to the left entirely contained in pytorch_qnnp_setup_convolution2d_nhwc_q8?

Copy link
Contributor Author

@kimishpatel kimishpatel Aug 18, 2020

Choose a reason for hiding this comment

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

Yes.

aten/src/ATen/native/quantized/cpu/qnnpack/src/conv-run.cc
const uint32_t cr = pytorch_qnnp_params.q8dw9.cr;
const size_t group_stride = (groups + (cr - 1)) & -cr;

if (any_padding) {
Copy link
Contributor

@AshkanAliabadi AshkanAliabadi Aug 18, 2020

Choose a reason for hiding this comment

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

Can you explain why this block is removed? From 376 to 414.

aten/src/ATen/native/quantized/cpu/qnnpack/src/deconv-run.cc
const void** indirection_buffer = (const void**)realloc(
deconvolution->indirection_buffer, indirection_buffer_size);
if (indirection_buffer == NULL) {
pytorch_qnnp_status status = pytorch_qnnp_setup_deconvolution2d_nhwc_q8(
Copy link
Contributor

@AshkanAliabadi AshkanAliabadi Aug 18, 2020

Choose a reason for hiding this comment

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

So again, all of the logic removed to the left is replaced with pytorch_qnnp_setup_deconvolution2d_nhwc_q8?

aten/src/ATen/native/quantized/cpu/qnnpack/test/convolution-operator-tester.h
const bool any_padding = (conv_p.padding[0]| conv_p.padding[1]
|conv_p.padding[2] | conv_p.padding[3]) != 0;

if (any_padding) {
Copy link
Contributor

@AshkanAliabadi AshkanAliabadi Aug 18, 2020

Choose a reason for hiding this comment

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

OK I see, it is moved here.

@kimishpatel
Update on "Refactor qconv to reduce allocations."
34db564 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
fedc37e 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
088f80e 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
16b720d 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@kimishpatel
Update on "Refactor qconv to reduce allocations."
d1cd111 
Summary:
zero buffer and indirection pointers are allocatoed on every iterations.
With this refactor we create op once for qnnpackconv struct and keep
repopulating indirection pointer as necessary.

For deconv moved much of op creation outside so that we can avoid creating and
destroying ops every time.

Test Plan:
CI quantization tests.
deconvolution-test

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D22726972](https://our.internmc.facebook.com/intern/diff/D22726972)

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 22, 2020

This pull request has been merged in 04aa42a.

@facebook-github-bot facebook-github-bot deleted the gh/kimishpatel/36/head branch August 25, 2020 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dreiss dreiss Awaiting requested review from dreiss

@supriyar supriyar Awaiting requested review from supriyar

1 more reviewer

@AshkanAliabadi AshkanAliabadi AshkanAliabadi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kimishpatel @facebook-github-bot @AshkanAliabadi @mruberry