Skip to content

[quant] conv_transpose2d_prepack/conv_transpose2d_unpack #40351

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 29 commits into from
Closed

[quant] conv_transpose2d_prepack/conv_transpose2d_unpack #40351

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

Conversation

@z-a-f
Copy link

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

Stack from ghstack:

Differential Revision: D22158983

This was referenced Jun 21, 2020
Closed
Closed
z-a-f pushed a commit that referenced this pull request Jun 21, 2020
[quant] conv_transpose2d_prepack/conv_transpose2d_unpack
7948923 
ghstack-source-id: 15a965a
Pull Request resolved: #40351
@z-a-f z-a-f added the oncall: quantization Quantization support in PyTorch label Jun 21, 2020
@dr-ci
Copy link

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

💊 CI failures summary and remediations

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

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

ci.pytorch.org: 2 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 166 times.

z-a-f
z-a-f commented Jun 21, 2020
View reviewed changes
aten/src/ATen/native/quantized/cpu/qconv_prepack.cpp Outdated
Comment on lines 235 to 240
"(weight dimensions: ",
weight.sizes(), " , transpose: ",
(transpose ? "True)." : "False)."),
"\nDEBUG INFO: ",
bias_fp32.ndimension(), ", ",
bias_fp32.size(0), ", ", out_ch
Copy link
Author

@z-a-f z-a-f Jun 21, 2020

Choose a reason for hiding this comment

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

Remove debug information

@z-a-f z-a-f mentioned this pull request Jun 21, 2020
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
Zafar added 10 commits June 23, 2020 14:53
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
88a92ac 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
a264f82 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
483d963 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
d670d37 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
4aad6fc 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
92cdc5e 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
2c97639 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
de09df2 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
53fd831 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
fe550d7 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
vkuzo
vkuzo reviewed Jul 22, 2020
View reviewed changes
aten/src/ATen/native/quantized/cpu/qconv_prepack.cpp
// Conv
m.impl("conv2d_prepack", TORCH_FN(QConvPackWeightInt8<2>::run_conv));
// ConvTranspose
m.impl("conv_transpose2d_prepack", TORCH_FN(QConvPackWeightInt8<2>::run_deconv));
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.

just curious, what's the reason to add this twice (also on line 423)?

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.

If I understand it correctly, the _ prefix is still used, and will be removed once the ops are migrated to mobile. However, before this is done, it should be registered for both (I think): #36510

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 wonder if we still need to do it? @jerryzh168

Copy link
Contributor

@vkuzo vkuzo Sep 2, 2020

Choose a reason for hiding this comment

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

maybe let's follow up on if we need this?

Copy link
Author

@z-a-f z-a-f Sep 3, 2020

Choose a reason for hiding this comment

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

I am fine either way -- the only concern is that if removed it will break the consistency with respect to the conv_Xd methods.

vkuzo
vkuzo reviewed Jul 22, 2020
View reviewed changes
aten/src/ATen/native/quantized/cpu/qconv_unpack.cpp
class QConvOutputPadding final {
public:
static torch::List<int64_t> run(
const c10::intrusive_ptr<ConvPackedParamsBase<kSpatialDim>>& packed_weight) {
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.

nit: packed_params, since it's actually not the weight that's being passed in?

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.

Although we call it ConvPackedParams, I think on python level it is still called packed_weight. I can change it to the params -- I am just concerned if this would be equally confusing?

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 can review again after serialization is clear

jerryzh168
jerryzh168 reviewed Jul 23, 2020
View reviewed changes
aten/src/ATen/native/quantized/cpu/qconv_unpack.cpp
Comment on lines +267 to +274
m.impl("conv_transpose2d_unpack", TORCH_FN(QConvUnpackWeightsInt8<2>::run));

m.impl("conv_transpose2d_stride", TORCH_FN(QConvStride<2>::run));
m.impl("conv_transpose2d_padding", TORCH_FN(QConvPadding<2>::run));
m.impl("conv_transpose2d_output_padding", TORCH_FN(QConvOutputPadding<2>::run));
m.impl("conv_transpose2d_dilation", TORCH_FN(QConvDilation<2>::run));
m.impl("conv_transpose2d_groups", TORCH_FN(QConvGroups<2>::run));
m.impl("conv_transpose2d_transpose", TORCH_FN(QConvTranspose<2>::run));
Copy link
Contributor

@jerryzh168 jerryzh168 Jul 23, 2020

Choose a reason for hiding this comment

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

why do we need different names?

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.

You mean in addition to the conv2d_*? If so, this is just for consistency. They call the same functions, but because we have conv2d and conv_transpose2d, I thought it would be confusing to have conv2d_*, but not the conv_transpose2d*. LMK if I should change it

Copy link
Contributor

@vkuzo vkuzo Sep 2, 2020

Choose a reason for hiding this comment

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

maybe we can avoid the duplicate methods and add a comment?

Copy link
Author

@z-a-f z-a-f Sep 3, 2020

Choose a reason for hiding this comment

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

Same as my previous comment -- I am fine removing it, the only convern is the symmetry with respect to the conv_Xd methods.

@z-a-f z-a-f requested review from jerryzh168 and vkuzo July 30, 2020 01:48
Zafar added 5 commits July 30, 2020 14:47
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
55481d5 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
8b6139d 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
010d209 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
f3e6da7 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[ghstack-poisoned]
Update on "[quant] conv_transpose2d_prepack/conv_transpose2d_unpack"
4eb04a1 
Differential Revision: [D22158983](https://our.internmc.facebook.com/intern/diff/D22158983)

[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
aten/src/ATen/native/quantized/cpu/qconv_prepack.cpp
torch::List<int64_t> dilation,
int64_t groups) {
return _run(weight, bias, stride, padding, output_padding, dilation, groups,
/*transpose=*/true);
Copy link
Contributor

@vkuzo vkuzo Sep 2, 2020

Choose a reason for hiding this comment

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

optional nit, would be good to avoid /*....*/, this removes the ability to easily comment out blocks of code for debugging

aten/src/ATen/native/quantized/cpu/qconv_prepack.cpp
// Conv
m.impl("conv2d_prepack", TORCH_FN(QConvPackWeightInt8<2>::run_conv));
// ConvTranspose
m.impl("conv_transpose2d_prepack", TORCH_FN(QConvPackWeightInt8<2>::run_deconv));
Copy link
Contributor

@vkuzo vkuzo Sep 2, 2020

Choose a reason for hiding this comment

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

maybe let's follow up on if we need this?

aten/src/ATen/native/quantized/cpu/qconv_unpack.cpp
Comment on lines +267 to +274
m.impl("conv_transpose2d_unpack", TORCH_FN(QConvUnpackWeightsInt8<2>::run));

m.impl("conv_transpose2d_stride", TORCH_FN(QConvStride<2>::run));
m.impl("conv_transpose2d_padding", TORCH_FN(QConvPadding<2>::run));
m.impl("conv_transpose2d_output_padding", TORCH_FN(QConvOutputPadding<2>::run));
m.impl("conv_transpose2d_dilation", TORCH_FN(QConvDilation<2>::run));
m.impl("conv_transpose2d_groups", TORCH_FN(QConvGroups<2>::run));
m.impl("conv_transpose2d_transpose", TORCH_FN(QConvTranspose<2>::run));
Copy link
Contributor

@vkuzo vkuzo Sep 2, 2020

Choose a reason for hiding this comment

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

maybe we can avoid the duplicate methods and add a comment?

@codecov
Copy link

codecov bot commented Sep 3, 2020 •
edited
Loading

Codecov Report

Merging #40351 into gh/z-a-f/39/base will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                Coverage Diff                @@
##           gh/z-a-f/39/base   #40351   +/-   ##
=================================================
  Coverage             69.34%   69.35%           
=================================================
  Files                   381      381           
  Lines                 47170    47178    +8     
=================================================
+ Hits                  32712    32721    +9     
+ Misses                14458    14457    -1
Impacted Files Coverage Δ
torch/testing/_internal/hypothesis_utils.py 82.63% <100.00%> (+0.87%) ⬆️
torch/testing/_internal/expecttest.py 78.57% <0.00%> (+1.02%) ⬆️

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 582205a...d0b783a. 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 69e3882.

@facebook-github-bot facebook-github-bot deleted the gh/z-a-f/39/head branch September 7, 2020 14:17
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

@raghuramank100 raghuramank100 Awaiting requested review from raghuramank100

@paulshaoyuqiao paulshaoyuqiao Awaiting requested review from paulshaoyuqiao

@jerryzh168 jerryzh168 Awaiting requested review from jerryzh168

Assignees

No one assigned

Labels

Merged oncall: quantization Quantization support in PyTorch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

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