Skip to content

Add better performing versions for groupwise and depthwise convolutions #22869

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
wants to merge 1 commit into from
Closed

Add better performing versions for groupwise and depthwise convolutions #22869

wants to merge 1 commit into from

Conversation

@dskhudia
Copy link
Contributor

Summary: Groupwise and depthwise convolutions become faster with this diff

Differential Revision: D16264144

@pytorchbot pytorchbot added module: nn Related to torch.nn module: operators oncall: quantization Quantization support in PyTorch labels Jul 15, 2019
@twmht
Copy link

twmht commented Jul 16, 2019 •
edited
Loading

@dskhudia

Did you benchmark this PR?

raghuramank100
raghuramank100 suggested changes Jul 16, 2019
View reviewed changes
Copy link
Contributor

@raghuramank100 raghuramank100 left a comment

Choose a reason for hiding this comment

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

Please add a benchmark test for the kernels and add the benchmarking results before and after the change as part of documentation for this PR. Thanks!

jerryzh168
jerryzh168 reviewed Jul 16, 2019
View reviewed changes
torch/nn/quantized/modules/conv.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

an unrelated question: do we have unpack for conv?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jerryzh168
We made sure to add unpack for conv. Details in pytorch/FBGEMM#103, pytorch/FBGEMM#105, pytorch/FBGEMM#106, pytorch/FBGEMM@815139b, pytorch/FBGEMM@64a2c73

Copy link
Contributor

Choose a reason for hiding this comment

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

is this ready or still ongoing?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I saw qconv_unpack, Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is ready. I might have to change your qat modules if they have landed.

@dskhudia
Copy link
Contributor Author

@dskhudia

Did you benchmark this PR?

@twmht
I'll add some benchmarking results soon. BTW, this is for convolutions on int8 data rather than fp32.

@dskhudia
Copy link
Contributor Author

Please add a benchmark test for the kernels and add the benchmarking results before and after the change as part of documentation for this PR. Thanks!

Sure. I'll add benchmarking results in the summary. I have a followup PR that adds benchmarking. #22895

@dskhudia
Copy link
Contributor Author

@raghuramank100 Please see summary for before and after results.

@jerryzh168 jerryzh168 modified the milestone: 1.2 Jul 18, 2019
raghuramank100
raghuramank100 reviewed Jul 23, 2019
View reviewed changes
jianyuh
jianyuh reviewed Jul 23, 2019
View reviewed changes
aten/src/ATen/native/quantized/cpu/qconv.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Still keep this for debugging purpose? Do we have printPackedMatrix function in the general Conv interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't. Let me remove this.

facebook-github-bot
facebook-github-bot reviewed Jul 25, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@dskhudia is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@dskhudia @facebook-github-bot
Add better performing versions for groupwise and depthwise convolutio…
2484c80 
…ns (pytorch#22869)

Summary:
Groupwise and depthwise convolutions become faster with this diff
Pull Request resolved: pytorch#22869

Test Plan:
buck test mode/dev caffe2/test:quantized -- 'test_qconv'  --print-passing-details

```
Running 2 tests
Started new test run: https://our.intern.facebook.com/intern/testinfra/testrun/562950091484224
      ✓ caffe2/test:quantized - test_qconv (test_quantized.TestQuantizedConv) 2.731 1/2 (passed)
Test output:
> test_qconv (test_quantized.TestQuantizedConv) ... ok
>
> ----------------------------------------------------------------------
> Ran 1 test in 2.732s
>
> OK
      ✓ caffe2/test:quantized - test_qconv_unpack (test_quantized.TestQuantizedConv) 5.187 2/2 (passed)
Test output:
> test_qconv_unpack (test_quantized.TestQuantizedConv) ... ok
>
> ----------------------------------------------------------------------
> Ran 1 test in 5.188s
>
> OK
Finished test run: https://our.intern.facebook.com/intern/testinfra/testrun/562950091484224
Summary (total time 15.66s):
  PASS: 2
  FAIL: 0
  SKIP: 0
  FATAL: 0
  TIMEOUT: 0
  OMIT: 0

```

buck test mode/dev caffe2/test:quantized -- 'test_conv_api'
```
Running 2 tests
Started new test run: https://our.intern.facebook.com/intern/testinfra/testrun/3940649676010406
      ✓ caffe2/test:quantized - test_conv_api (test_nn_quantized.ModuleAPITest) 0.040 1/2 (passed)
      ✓ caffe2/test:quantized - test_conv_api (test_quantized_conv.FunctionalAPITest) 5.402 2/2 (passed)
Finished test run: https://our.intern.facebook.com/intern/testinfra/testrun/3940649676010406
Summary (total time 11.83s):
  PASS: 2
  FAIL: 0
  SKIP: 0
  FATAL: 0
  TIMEOUT: 0
  OMIT: 0
```

Differential Revision: D16264144

Pulled By: dskhudia

fbshipit-source-id: 959417212f99d3789c2b48361dd36cb864db3594
zdevito pushed a commit to zdevito/ATen that referenced this pull request Jul 26, 2019
@dskhudia @facebook-github-bot
Add better performing versions for groupwise and depthwise convolutio…
269eb1a 
…ns (#22869)

Summary:
Groupwise and depthwise convolutions become faster with this diff
Pull Request resolved: pytorch/pytorch#22869

Test Plan:
buck test mode/dev caffe2/test:quantized -- 'test_qconv'  --print-passing-details

```
Running 2 tests
Started new test run: https://our.intern.facebook.com/intern/testinfra/testrun/562950091484224
      ✓ caffe2/test:quantized - test_qconv (test_quantized.TestQuantizedConv) 2.731 1/2 (passed)
Test output:
> test_qconv (test_quantized.TestQuantizedConv) ... ok
>
> ----------------------------------------------------------------------
> Ran 1 test in 2.732s
>
> OK
      ✓ caffe2/test:quantized - test_qconv_unpack (test_quantized.TestQuantizedConv) 5.187 2/2 (passed)
Test output:
> test_qconv_unpack (test_quantized.TestQuantizedConv) ... ok
>
> ----------------------------------------------------------------------
> Ran 1 test in 5.188s
>
> OK
Finished test run: https://our.intern.facebook.com/intern/testinfra/testrun/562950091484224
Summary (total time 15.66s):
  PASS: 2
  FAIL: 0
  SKIP: 0
  FATAL: 0
  TIMEOUT: 0
  OMIT: 0

```

buck test mode/dev caffe2/test:quantized -- 'test_conv_api'
```
Running 2 tests
Started new test run: https://our.intern.facebook.com/intern/testinfra/testrun/3940649676010406
      ✓ caffe2/test:quantized - test_conv_api (test_nn_quantized.ModuleAPITest) 0.040 1/2 (passed)
      ✓ caffe2/test:quantized - test_conv_api (test_quantized_conv.FunctionalAPITest) 5.402 2/2 (passed)
Finished test run: https://our.intern.facebook.com/intern/testinfra/testrun/3940649676010406
Summary (total time 11.83s):
  PASS: 2
  FAIL: 0
  SKIP: 0
  FATAL: 0
  TIMEOUT: 0
  OMIT: 0
```

Differential Revision: D16264144

Pulled By: dskhudia

fbshipit-source-id: 32fa43e5c3d97c8aaa6e0858327a2ac0aef8df5c
@facebook-github-bot
Copy link
Contributor

@dskhudia merged this pull request in 6a8c275.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jianyuh jianyuh jianyuh left review comments

@jerryzh168 jerryzh168 jerryzh168 left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

+1 more reviewer

@raghuramank100 raghuramank100 raghuramank100 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged module: nn Related to torch.nn oncall: quantization Quantization support in PyTorch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@dskhudia @twmht @facebook-github-bot @jianyuh @jerryzh168 @raghuramank100 @pytorchbot @mruberry