Refactor qconv_prepack and qconv_unpack to support conv3d (#146) #28481

xiaomengy · 2019-10-23T00:40:41Z

Summary:
Pull Request resolved: pytorch/FBGEMM#146

Refactor qconv_prepack and qconv_unpack to support conv3d

Test Plan: buck test mode/dev-nosan caffe2/test:quantized -- "Conv"

Reviewed By: dskhudia

Differential Revision: D18023651

facebook-github-bot · 2019-10-23T00:41:01Z

This pull request was exported from Phabricator. Differential Revision: D18023651

xiaomengy · 2019-10-23T00:41:42Z

link to #28480

xiaomengy · 2019-10-23T00:44:21Z

link to pytorch/FBGEMM#146

facebook-github-bot · 2019-10-23T03:34:12Z

This pull request was exported from Phabricator. Differential Revision: D18023651

facebook-github-bot · 2019-10-23T06:09:05Z

This pull request was exported from Phabricator. Differential Revision: D18023651

facebook-github-bot · 2019-10-23T17:21:33Z

This pull request was exported from Phabricator. Differential Revision: D18023651

facebook-github-bot · 2019-10-23T21:42:14Z

This pull request was exported from Phabricator. Differential Revision: D18023651

facebook-github-bot · 2019-10-23T22:20:55Z

This pull request was exported from Phabricator. Differential Revision: D18023651

jamesr66a

Generally looks OK. I'd like to defer to Vitaly to verify the memory layout changes

jamesr66a · 2019-10-23T22:41:26Z

aten/src/ATen/native/quantized/cpu/qconv_prepack.cpp

Hmm, I don't really like the naming inconsistency here (conv_prepack vs conv3d_prepack, i'd prefer conv2d_prepack). It would be BC breaking but quantization is experimental so it's probably okay

The problem is conv_prepack op is already there and if we change the api name, there may be some backward compatibility issues.

jamesr66a · 2019-10-23T22:42:03Z

aten/src/ATen/native/quantized/cpu/qconv_unpack.cpp

Same comment on naming inconsistency as above

Same as the comments above, because of backward compatibility tests, we cannot change the existing op names and args

What I’m saying above is we probably should break backwards compatibility, otherwise we’re in a state that doesn’t make sense. We can defer this to another PR though for now

jamesr66a · 2019-10-23T22:45:45Z

test/test_quantized.py

Naming the function arguments qconv_prepack and qconv_unpack can be confusing for readers. Can you name them something like unpack_fn and pack_fn to distinguish them from the ops?

jamesr66a · 2019-10-23T22:47:54Z

c10/core/MemoryFormat.h

cc @VitalyFedyunin does this look OK?

jamesr66a · 2019-10-23T22:48:18Z

aten/src/ATen/native/quantized/cpu/qconv_unpack.cpp

:'( any plan to support it in the future?

I guess there is? Actually I am not quite sure about that part.

jamesr66a · 2019-10-23T22:55:24Z

aten/src/ATen/native/quantized/cpu/qconv_prepack.cpp

for this conv_param_t invocation and the one below, can we getp aram name comments (e.g. like

pytorch/aten/src/ATen/native/quantized/cpu/qlinear.cpp

Line 170 in d83389d

/*packA=*/packA,

)

facebook-github-bot · 2019-10-23T23:04:27Z

This pull request was exported from Phabricator. Differential Revision: D18023651

facebook-github-bot · 2019-10-25T21:40:48Z

This pull request was exported from Phabricator. Differential Revision: D18023651

facebook-github-bot · 2019-10-25T21:42:21Z

This pull request was exported from Phabricator. Differential Revision: D18023651

facebook-github-bot · 2019-10-26T16:17:18Z

This pull request was exported from Phabricator. Differential Revision: D18023651

facebook-github-bot · 2019-10-27T15:54:17Z

This pull request was exported from Phabricator. Differential Revision: D18023651

facebook-github-bot · 2019-10-27T16:07:41Z

This pull request was exported from Phabricator. Differential Revision: D18023651

VitalyFedyunin

LGTG for me in terms of memory formats. Took a couple of important notes for me on how to implement 3d channels last.

VitalyFedyunin · 2019-10-27T16:01:03Z

aten/src/ATen/native/quantized/cpu/fbgemm_utils.cpp

This is nice candidate to be public function similar to empty_strided

VitalyFedyunin · 2019-10-27T16:02:27Z

aten/src/ATen/native/quantized/cpu/fbgemm_utils.cpp

No need to check is_contiguous, src.contiguous() will provide same functionality internally.

) Summary: Pull Request resolved: pytorch#28481 Refactor qconv_prepack and qconv_unpack to support conv3d Test Plan: buck test mode/dev-nosan caffe2/test:quantized -- "Conv" Reviewed By: dskhudia Differential Revision: D18023651 fbshipit-source-id: 4c6be0e3ebf9d995e442c5be63d4316944fa9440

facebook-github-bot · 2019-10-27T16:34:59Z

This pull request was exported from Phabricator. Differential Revision: D18023651

jianyuh

LGTM! @dskhudia has made the comments and approved in the internal tools. Please make sure all OSS tests pass.

facebook-github-bot · 2019-10-27T22:06:19Z

This pull request has been merged in d5afd97.

Summary: Pull Request resolved: pytorch/pytorch#28481 Refactor qconv_prepack and qconv_unpack to support conv3d Test Plan: buck test mode/dev-nosan caffe2/test:quantized -- "Conv" Reviewed By: dskhudia Differential Revision: D18023651 fbshipit-source-id: 8cbc9fe68f93bc4b247a4f41423c6d8c30a5ef90

Summary: Changelog: - Re-enable multinomial sampling when the probability tensor has `dtype == torch.half`. It seems to have been missed in #28481. Fixes #29211 Pull Request resolved: #29266 Differential Revision: D18619105 Pulled By: ezyang fbshipit-source-id: 1f87e5183e75de5c5e0ffde862fc72d040b32864

xiaomengy requested review from dskhudia and dzhulgakov October 23, 2019 00:41

xiaomengy self-assigned this Oct 23, 2019

xiaomengy mentioned this pull request Oct 23, 2019

Refactor qconv_prepack and qconv_unpack to support conv3d pytorch/FBGEMM#146

Closed

xiaomengy force-pushed the export-D18023651 branch from e4cd874 to 9ceba6e Compare October 23, 2019 03:34

xiaomengy force-pushed the export-D18023651 branch from 9ceba6e to 46bd15d Compare October 23, 2019 06:09

dzhulgakov requested a review from jamesr66a October 23, 2019 06:37

xiaomengy force-pushed the export-D18023651 branch from 46bd15d to ad241eb Compare October 23, 2019 17:21

xiaomengy force-pushed the export-D18023651 branch from ad241eb to 843687f Compare October 23, 2019 21:42

xiaomengy force-pushed the export-D18023651 branch from 843687f to 6bb69ca Compare October 23, 2019 22:20

jamesr66a reviewed Oct 23, 2019

View reviewed changes

xiaomengy force-pushed the export-D18023651 branch from 6bb69ca to 4427107 Compare October 23, 2019 23:04

xiaomengy force-pushed the export-D18023651 branch from 4427107 to f814542 Compare October 23, 2019 23:19

xiaomengy force-pushed the export-D18023651 branch from 66db06f to 7597d7e Compare October 25, 2019 21:40

xiaomengy force-pushed the export-D18023651 branch from 7597d7e to 8d8fb86 Compare October 25, 2019 21:42

xiaomengy force-pushed the export-D18023651 branch from 8d8fb86 to 7c61ae1 Compare October 26, 2019 16:17

xiaomengy force-pushed the export-D18023651 branch from 7c61ae1 to 6b06a62 Compare October 27, 2019 15:54

xiaomengy force-pushed the export-D18023651 branch from 6b06a62 to a4a222c Compare October 27, 2019 16:07

VitalyFedyunin reviewed Oct 27, 2019

View reviewed changes

xiaomengy force-pushed the export-D18023651 branch from a4a222c to 22e8a18 Compare October 27, 2019 16:34

jianyuh approved these changes Oct 27, 2019

View reviewed changes

jamesr66a approved these changes Oct 27, 2019

View reviewed changes

facebook-github-bot closed this in d5afd97 Oct 27, 2019

facebook-github-bot added the merged label Oct 27, 2019

xiaomengy deleted the export-D18023651 branch October 27, 2019 22:36

vishwakftw mentioned this pull request Nov 6, 2019

Enable multinomial for torch.half #29266

Closed

mruberry added the Merged label Oct 28, 2020

Refactor qconv_prepack and qconv_unpack to support conv3d (#146) #28481

Refactor qconv_prepack and qconv_unpack to support conv3d (#146) #28481

Uh oh!

Conversation

xiaomengy commented Oct 23, 2019

Uh oh!

facebook-github-bot commented Oct 23, 2019

Uh oh!

xiaomengy commented Oct 23, 2019

Uh oh!

xiaomengy commented Oct 23, 2019

Uh oh!

facebook-github-bot commented Oct 23, 2019

Uh oh!

facebook-github-bot commented Oct 23, 2019

Uh oh!

facebook-github-bot commented Oct 23, 2019

Uh oh!

facebook-github-bot commented Oct 23, 2019

Uh oh!

facebook-github-bot commented Oct 23, 2019

Uh oh!

jamesr66a left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 23, 2019

Uh oh!

facebook-github-bot commented Oct 25, 2019

Uh oh!

facebook-github-bot commented Oct 25, 2019

Uh oh!

facebook-github-bot commented Oct 26, 2019

Uh oh!

facebook-github-bot commented Oct 27, 2019

Uh oh!

facebook-github-bot commented Oct 27, 2019

Uh oh!

VitalyFedyunin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 27, 2019

Uh oh!

jianyuh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 27, 2019

Uh oh!

Reviewers

Assignees

jianyuh left a comment •

edited

Loading