Skip to content

[quant] Prep for conv_transpose packing #39714

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
z-a-f wants to merge 33 commits into from
Closed

[quant] Prep for conv_transpose packing #39714

z-a-f wants to merge 33 commits into from

Conversation

@z-a-f
Copy link

@z-a-f z-a-f commented Jun 9, 2020 •
edited
Loading

Stack from ghstack:

Differential Revision: D22087071

@z-a-f z-a-f requested a review from apaszke as a code owner June 9, 2020 07:46
z-a-f pushed a commit that referenced this pull request Jun 9, 2020
[quant] Prep for conv_transpose packing
2e25708 
ghstack-source-id: 9148871
Pull Request resolved: #39714
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jun 9, 2020
@dr-ci
Copy link

dr-ci bot commented Jun 9, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 582205a (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 184 times.

z-a-f pushed a commit that referenced this pull request Jun 9, 2020
[quant] Prep for conv_transpose packing
492b635 
ghstack-source-id: c454ab9
Pull Request resolved: #39714
z-a-f pushed a commit that referenced this pull request Jun 11, 2020
[quant] Prep for conv_transpose packing
9c64c8c 
ghstack-source-id: 8a3d039
Pull Request resolved: #39714
z-a-f pushed a commit that referenced this pull request Jun 11, 2020
[quant] Prep for conv_transpose packing
08e431f 
ghstack-source-id: 931913f
Pull Request resolved: #39714
@z-a-f z-a-f added the oncall: quantization Quantization support in PyTorch label Jun 11, 2020
@z-a-f z-a-f requested review from jamesr66a and jerryzh168 June 11, 2020 08:25
jerryzh168
jerryzh168 reviewed Jun 11, 2020
View reviewed changes
jamesr66a
jamesr66a reviewed Jun 11, 2020
View reviewed changes
z-a-f pushed a commit that referenced this pull request Jun 12, 2020
[quant] Prep for conv_transpose packing
88d20eb 
ghstack-source-id: 2c7d3eb
Pull Request resolved: #39714
@z-a-f z-a-f requested a review from raghuramank100 June 12, 2020 22:36
@z-a-f z-a-f mentioned this pull request Jun 20, 2020
Closed
z-a-f pushed a commit that referenced this pull request Jun 20, 2020
[quant] Prep for conv_transpose packing
de14d4a 
ghstack-source-id: 939b91c
Pull Request resolved: #39714
This was referenced Jun 21, 2020
Closed
Closed
@z-a-f z-a-f mentioned this pull request Jun 22, 2020
Closed
@z-a-f z-a-f mentioned this pull request Jun 22, 2020
Closed
vkuzo
vkuzo reviewed Jul 22, 2020
View reviewed changes
Copy link
Contributor

@vkuzo vkuzo left a comment

Choose a reason for hiding this comment

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

looks reasonable, ideally we could review this for real when the serialization story is agreed upon

aten/src/ATen/native/quantized/cpu/qconv_prepack.cpp Outdated
kSpatialDim,
"D convolution.");
const int output_channels = weight.size(0);
const int output_channels_idx = transpose ? 1 : 0;
Copy link
Contributor

@vkuzo vkuzo Jul 22, 2020

Choose a reason for hiding this comment

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

optional: if you want these to be more readable, you could define an enum ConvType which can be ConvType::CONV | ConvType::CONV_TRANSPOSE and pass that around instead

Copy link
Author

@z-a-f z-a-f Jul 30, 2020

Choose a reason for hiding this comment

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

I love this solution -- I was originally thinking of casting boolean to int (as in int(transpose)), but thought that would be confusing when used as a channel index

aten/src/ATen/native/quantized/cpu/qnnpack_utils.h
{(uint32_t)dilation_[1], (uint32_t)dilation_[0]},
{(uint32_t)padding_[0], (uint32_t)padding_[1],
(uint32_t)padding_[0], (uint32_t)padding_[1]},
/*adjustment=*/{0, 0},
Copy link
Contributor

@vkuzo vkuzo Jul 22, 2020

Choose a reason for hiding this comment

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

what does "adjustment" mean here / is it ok to replace with output padding?

Copy link
Author

@z-a-f z-a-f Jul 30, 2020

Choose a reason for hiding this comment

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

Yea, the terminology sucks -- technically, this is adjustment to the output size, which is the same as the output_padding. The only difference is that adjustment refers to the size in general, while padding refers to the padding to every side separately.

Basically, I am using asjustment here because the qnnpack uses this as terminology, while pytorch uses "padding" -- should I change it?

@z-a-f z-a-f requested review from jamesr66a, jerryzh168 and vkuzo July 30, 2020 01:41
Zafar added 5 commits July 30, 2020 14:47
Update on "[quant] Prep for conv_transpose packing"
9508ff2 
Differential Revision: [D22087071](https://our.internmc.facebook.com/intern/diff/D22087071)

[ghstack-poisoned]
Update on "[quant] Prep for conv_transpose packing"
c6f96a4 
Differential Revision: [D22087071](https://our.internmc.facebook.com/intern/diff/D22087071)

[ghstack-poisoned]
Update on "[quant] Prep for conv_transpose packing"
1cda316 
Differential Revision: [D22087071](https://our.internmc.facebook.com/intern/diff/D22087071)

[ghstack-poisoned]
Update on "[quant] Prep for conv_transpose packing"
e1ba4a8 
Differential Revision: [D22087071](https://our.internmc.facebook.com/intern/diff/D22087071)

[ghstack-poisoned]
Update on "[quant] Prep for conv_transpose packing"
eed89b0 
Differential Revision: [D22087071](https://our.internmc.facebook.com/intern/diff/D22087071)

[ghstack-poisoned]
@z-a-f z-a-f mentioned this pull request Aug 12, 2020
Closed
@z-a-f z-a-f mentioned this pull request Aug 18, 2020
Closed
@z-a-f z-a-f mentioned this pull request Aug 18, 2020
Merged
vkuzo
vkuzo approved these changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@vkuzo vkuzo left a comment

Choose a reason for hiding this comment

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

lg if CI passes

@codecov
Copy link

codecov bot commented Sep 3, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (gh/z-a-f/35/base@70e8e7c). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                 Coverage Diff                 @@
##             gh/z-a-f/35/base   #39714   +/-   ##
===================================================
  Coverage                    ?   69.34%           
===================================================
  Files                       ?      381           
  Lines                       ?    47170           
  Branches                    ?        0           
===================================================
  Hits                        ?    32712           
  Misses                      ?    14458           
  Partials                    ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70e8e7c...582205a. Read the comment docs.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 4, 2020

@z-a-f merged this pull request in e05fa2f.

@facebook-github-bot facebook-github-bot deleted the gh/z-a-f/35/head branch September 7, 2020 14:17
@vkuzo vkuzo mentioned this pull request Sep 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vkuzo vkuzo vkuzo approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@raghuramank100 raghuramank100 Awaiting requested review from raghuramank100

@jamesr66a jamesr66a Awaiting requested review from jamesr66a

@jerryzh168 jerryzh168 Awaiting requested review from jerryzh168

Assignees

No one assigned

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue oncall: quantization Quantization support in PyTorch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@z-a-f @facebook-github-bot @vkuzo @jamesr66a @jerryzh168 @mruberry