Changes to transition to generic API for ops with weight prepacking by kimishpatel · Pull Request #35010 · pytorch/pytorch

kimishpatel · 2020-03-19T00:31:53Z

Stack from ghstack:

Changes to transition to generic API for ops with weight prepacking #35010 Changes to transition to generic API for ops with weight prepacking

semantics.

Summary:
This PR moves all the xnnpack specific interfces to a generic interface.
Accordingly removes xnnpac specific reference from API and some variable
names.
What has not yet changed:

TODO:
USE_XNNPACK is still used. This can be removed where no XNNPACK
specific things are done. e.g., RegisterOpContext.cpp and
xnnpack_rewrite.cpp.
Also the filename and structure also remains. Some of the generic class
definition can be moved non-XNNPACK specific folder.

Test Plan:
python test/test_xnnpack_integration.py

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D20526416

semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 01e11d3 Pull Request resolved: #35010

kimishpatel · 2020-03-19T00:33:27Z

To note: This PR has not yet done the following:
USE_XNNPACK is still used. This can be removed where no XNNPACK
specific things are done. e.g., RegisterOpContext.cpp and
xnnpack_rewrite.cpp.
Also the filename and structure also remains. Some of the generic class
definition can be moved non-XNNPACK specific folder.

dzhulgakov

Looks like on the right track!

Why do you have many formatting changes? Is it different version of clang-format?

dzhulgakov · 2020-03-19T01:37:33Z

-        groups,
-        {},
-        {});
+Tensor run(const ContextConv2D& context, const Tensor& input) {


maybe minor thing - but why no make it a method?

I kept how it was in the original diff TBH. Yes it can be a method and in this case it might make sense. I will just keep it a TODO for future refactoring instead of doing it in the diff.

dzhulgakov · 2020-03-19T01:38:25Z

+  return output.contiguous(input.suggest_memory_format());
+}
+
+c10::intrusive_ptr<xnnpack::Conv2dOpContext> Conv2dPrePack::operator()(


nit: you don't need this trampoline - you can register xnnpack::XNNPackConv2dOpContext::create_context directly with RegisterOps (it doesn't need to be a functor)

I tried that in the first diff itself and something I ran into that would not let me compile, cannot remember what. I will try again and see if there is a way around else report the error back here.

dzhulgakov · 2020-03-19T01:40:17Z

+
+class LinearOpContext : public torch::jit::CustomClassHolder {
+ protected:
+  Tensor orig_weight_;


this duplicates all weights for serialization, I guess xnnpack doesn't have "unpack" method, does it?

This is correct. My plan was to have the weights set to null inside setstate conditioned on some flags such as lite_interpreter run etc., that ensure we will not be serializing this. Is that the right approach?

@dzhulgakov, so I think we dont need to use trampoline. However, given that we are doing all this to make a generic API, it makes sense to hide backend specific context creation behind something, which in this PR turns out to be just a trampoline. However I am gonna stack a PR on top of this which will check global context to see which engine we want to use for packing (for now xnnpack) and direct context creation appropriately. I will refactor this code so that both setstate method and PrePack op are directed to a common method for context creaton.

I'm not sure what the right approach is here. If we set the weights to null, that means we can't re-save the model, right?

Yes, but if we know we will not need to, e.g. when we know we are doing inference in lite interpreter, it should ok, right?

Yeah, but what about people using the full JIT. This is okay for now, but we should keep thinking about better solutions.

Keeping the weights around is fine, we can gate freeing them with mobile interpreter flag indeed (we kind of do that for qnnpack on mobile I think).

dr-ci · 2020-03-19T04:30:21Z

💊 CircleCI build failures summary and remediations

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

✅ None of the build failures appear to be your fault 💚

2/2 broken upstream at merge base a100cf5 since Mar 21
Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:
```
git fetch https://github.com/pytorch/pytorch viable/strict
git rebase --onto FETCH_HEAD $(git merge-base origin/master HEAD)
```
If your commit is older than viable/strict:
```
git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD
```
Check out the recency history of this "viable master" tracking branch.

🚧 2 upstream failures:

These were probably caused by upstream breakages:

pytorch_windows_vs2019_py36_cuda10.1_test2 since Mar 21
pytorch_linux_xenial_py3_clang5_asan_test since Mar 21

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.

This comment has been revised 31 times.

kimishpatel · 2020-03-19T15:08:33Z

Why do you have many formatting changes? Is it different version of clang-format?

Possibly. I ended apply clang-format on all the files that I touched.

…repacking" semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20526416](https://our.internmc.facebook.com/intern/diff/D20526416) [ghstack-poisoned]

semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 2143c10 Pull Request resolved: #35010

…repacking" semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20526416](https://our.internmc.facebook.com/intern/diff/D20526416) [ghstack-poisoned]

dreiss

Formatting changes need to be in a separate diff. This is impossible to review asis.

kimishpatel · 2020-03-19T19:20:14Z

Formatting changes need to be in a separate diff. This is impossible to review asis.

I shouldnt have run clang-format on this. This is going to take a while to fix.

…repacking" semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20526416](https://our.internmc.facebook.com/intern/diff/D20526416) [ghstack-poisoned]

semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 3cdde9d Pull Request resolved: #35010

…repacking" semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20526416](https://our.internmc.facebook.com/intern/diff/D20526416) [ghstack-poisoned]

semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: b8aaea5 Pull Request resolved: #35010

dreiss · 2020-03-20T02:22:46Z

+
+class LinearOpContext : public torch::jit::CustomClassHolder {
+ protected:
+  Tensor orig_weight_;


Yeah, but what about people using the full JIT. This is okay for now, but we should keep thinking about better solutions.

…repacking" semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20526416](https://our.internmc.facebook.com/intern/diff/D20526416) [ghstack-poisoned]

semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Also had to add BC test changes to whitelist the ops added. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 5ca10a0 Pull Request resolved: #35010

…repacking" semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20526416](https://our.internmc.facebook.com/intern/diff/D20526416) [ghstack-poisoned]

semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Also had to add BC test changes to whitelist the ops added. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 9d05d87 Pull Request resolved: #35010

…repacking" semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20526416](https://our.internmc.facebook.com/intern/diff/D20526416) [ghstack-poisoned]

semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Also had to add BC test changes to whitelist the ops added. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: b55a94d Pull Request resolved: #35010

…repacking" semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D20526416](https://our.internmc.facebook.com/intern/diff/D20526416) [ghstack-poisoned]

semantics. Summary: This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Also had to add BC test changes to whitelist the ops added. Test Plan: python test/test_xnnpack_integration.py Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: b75cb64 Pull Request resolved: #35010

facebook-github-bot · 2020-03-22T18:09:40Z

This pull request has been merged in e1c092f.

facebook-github-bot · 2020-03-22T18:10:11Z

This pull request has been merged in e1c092f.

jerryzh168 · 2020-03-23T16:54:37Z

What's the plan? are we going to use this for quantized ops as well?

kimishpatel · 2020-03-23T16:56:21Z

What's the plan? are we going to use this for quantized ops as well?

I don't think so. Changes are mainly to abstract away xnnpack specific namespace from ops and make it generic. For example if we want to use gpu as the backend instead of xnnpack.

dreiss · 2020-03-23T17:01:12Z

What's the plan? are we going to use this for quantized ops as well?

Not my plan. Quantized ops need to have output scale/zero as part of the interface, so IMO they should follow this same pattern but be separate.

jerryzh168 · 2020-03-23T17:03:42Z

folding quantized prepack is pretty much the same I think: #35077

So is this for the non quantized use case only?

jerryzh168 · 2020-03-23T17:09:26Z

Oh sorry, it might be confusing, I'm just talking about prepack folding. Found this PR when after I rebase my PRs and see the name of FoldPrepackingOps is changed

kimishpatel · 2020-03-23T17:19:56Z

Oh sorry, it might be confusing, I'm just talking about prepack folding. Found this PR when after I rebase my PRs and see the name of FoldPrepackingOps is changed

Not sure what you mean by FoldPrepackingOps name changed. Also the rest of the op registration namespace changed but that should not impact any JIT passes.

jerryzh168 · 2020-03-23T17:29:10Z

+        n->kind() == Symbol::fromQualString("prepacked::conv2d_clamp_prepack"));
  };
-  FoldPrePackingOps(m, filter_fn, "xnnpack_prepack_folding");
+  PrePackingOpsFolder(m, filter_fn, "prepack_folding");


I'm talking about the function name is changed from FoldPrePackingOps to PrePackingOpsFolder, which leads me to the PR, but it is not important. I'm just trying to understand what is the use case this function is targeting, is this only for non-quantized prepacking ops?

Aah thanks for pointing out :). Shouldn't have changed that as part of this PR. But to answer your question, no. It should be generic as we had discussed originally when the PR for this pass was under review. It should still be a generally usable pass that is can be applied after freezing, regardless. However the naming either FoldPrePackingOps or PrePackingOpsFolder does suggest that it is for prepacking ops, including quantized. But I understand the concern, if that is the concern, that the naming may suggest that it is applied to ops that are in prepacked namespace even though that is not the case.

Yeah the implementation is applicable to quantized ops as well. Do we want to do use the same function for quantized prepacking ops as well(since now it is changed to a generic function)?

I think that was the original plan, right? I dont think anything about that has changed, except naming. So I would say yes we should.

I mean yeah, I can use PrePackingOpsFolder to implement a pass to fold quantized ops.
My question is should we merge that to the current new FoldPrePackingOps here, or is this still function still just for xnnpack?

resolved in chat, this is prepack folding for fp32 ops.

…ytorch#35010) Summary: Pull Request resolved: pytorch#35010 semantics. This PR moves all the xnnpack specific interfces to a generic interface. Accordingly removes xnnpac specific reference from API and some variable names. What has not yet changed: TODO: USE_XNNPACK is still used. This can be removed where no XNNPACK specific things are done. e.g., RegisterOpContext.cpp and xnnpack_rewrite.cpp. Also the filename and structure also remains. Some of the generic class definition can be moved non-XNNPACK specific folder. Test Plan: python test/test_xnnpack_integration.py Imported from OSS Differential Revision: D20526416 fbshipit-source-id: 2e1725345c44bbb26bdc448097a7384eca121387

kimishpatel requested a review from apaszke as a code owner March 19, 2020 00:31

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Mar 19, 2020

kimishpatel requested review from AshkanAliabadi, dreiss and dzhulgakov March 19, 2020 00:32

dzhulgakov reviewed Mar 19, 2020

View reviewed changes

dreiss requested changes Mar 19, 2020

View reviewed changes

dreiss approved these changes Mar 20, 2020

View reviewed changes

dreiss approved these changes Mar 21, 2020

View reviewed changes

facebook-github-bot closed this in e1c092f Mar 22, 2020

facebook-github-bot added the merged label Mar 22, 2020

jerryzh168 reviewed Mar 23, 2020

View reviewed changes

facebook-github-bot deleted the gh/kimishpatel/5/head branch March 26, 2020 14:16

This was referenced Apr 14, 2020

Generic API for ops that are require context, such as packed weights. #34913

Closed

Add a centralized TORCH_LIBRARY declaration for quantized and xnnpack ops. #36520

Closed

XNNPACK operators are not actually registered under xnnpack namespace #36517

Open

mruberry added the Merged label Oct 28, 2020

Conversation

kimishpatel commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimishpatel commented Mar 19, 2020

Uh oh!

dzhulgakov 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!

dr-ci Bot commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CircleCI build failures summary and remediations

🚧 2 upstream failures:

Uh oh!

kimishpatel commented Mar 19, 2020

Uh oh!

dreiss left a comment

Choose a reason for hiding this comment

Uh oh!

kimishpatel commented Mar 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Mar 22, 2020

Uh oh!

facebook-github-bot commented Mar 22, 2020

Uh oh!

jerryzh168 commented Mar 23, 2020

Uh oh!

kimishpatel commented Mar 23, 2020

Uh oh!

dreiss commented Mar 23, 2020

Uh oh!

jerryzh168 commented Mar 23, 2020

Uh oh!

jerryzh168 commented Mar 23, 2020

Uh oh!

kimishpatel commented Mar 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryzh168 Mar 23, 2020 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

kimishpatel commented Mar 19, 2020 •

edited

Loading

dr-ci Bot commented Mar 19, 2020 •

edited

Loading

jerryzh168 Mar 23, 2020 •

edited

Loading