Fork QNNPACK into aten/src/ATen/native/quantized/cpu/qnnpack #25500

AshkanAliabadi · 2019-08-30T22:26:20Z

The motivation for this move, and our long-term commitment to maintaining and integrating this code into ATen is described in the issue below:

#25621

AshkanAliabadi · 2019-08-30T22:28:01Z

aten/src/ATen/native/quantized/cpu/qnnpack/CMakeLists.txt

+INCLUDE(GNUInstallDirs)
+
+# ---[ Project and semantic versioning.
+PROJECT(PYTORCH_QNNPACK C CXX ASM)


Changed project name to PYTORCH_QNNPACK.

AshkanAliabadi · 2019-08-30T22:29:12Z

aten/src/ATen/native/quantized/cpu/qnnpack/include/pytorch_qnnpack.h

+/**
+ * @brief Status code for any QNNPACK function call.
+ */
+enum pytorch_qnnp_status {


All symbols prefixed with pytorch to avoid collision.

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

AshkanAliabadi · 2019-08-31T01:26:44Z

cmake/Dependencies.cmake

+      set_property(TARGET pytorch_qnnpack PROPERTY POSITION_INDEPENDENT_CODE ON)
+      set_property(TARGET pthreadpool PROPERTY POSITION_INDEPENDENT_CODE ON)
+      set_property(TARGET cpuinfo PROPERTY POSITION_INDEPENDENT_CODE ON)
+    endif()


Please double check all changes in this file.

dzhulgakov

Have we tried to compile it with C++? It might be easier that way as we can use namespaces.

Also, is the plan to just blindly pull in all of qnnpack (like in this PR) or be more selective? cc @ajtulloch for advice too

dzhulgakov · 2019-09-03T04:46:32Z

aten/src/ATen/native/quantized/cpu/qnnpack/CMakeLists.txt

+INCLUDE(GNUInstallDirs)
+
+# ---[ Project and semantic versioning.
+PROJECT(PYTORCH_QNNPACK C CXX ASM)


you can also just drop it as we'd be compiling it inline (but not sure)

Yes, that is true. My goal was to minimize modifications to make sure I am not breaking anything in the process inadvertently since different flags may be used between PyTorch and QNNPACK for compilation and linking, but I can definitely try that if that's what you prefer.

Have we tried to compile it with C++? It might be easier that way as we can use namespaces.

I actually asked the same question about namespace offline :) I don't know how much work to recompile with c++ though. In worst case we can change it back to namespace when things are more clear?

OK so I tried compiling with C++, but it's a good deal of work. The compiler is complaining about jumps (use of goto to be exact) bypassing variable initializations, and use of 'restrict' (as it's not a C++ keyword.) All of this can be worked around (using __restrict for instance instead of restrict and initializing all these variables to null / zero prior at them being jumped over), but it's a good deal of work. Making these changes is actually one thing and not terribly difficult, but making sure they do not introduce any hidden bugs is a whole another can of worms that I would rather not open at this point.

https://gist.github.com/AshkanAliabadi/d7e78ce6c0ea6e9b092d71a9bb96b9de

It would be nice to compile with C++ for new files. I am planning on adding new APIs to make QNNPACK more functional and it would be a bit clunky if they are written in C as well. That can be part of a different PR though.

We shouldn't have any problems adding new C++ files to QNNPack. QNNPack already has C++ files in the form of tests and benchmarks. It is just that modifying the C files to make them C++-conformant is more work that we can take on right now.

dzhulgakov · 2019-09-03T04:48:26Z

aten/src/ATen/native/quantized/cpu/qnnpack/deps/clog/include/clog.h

@@ -0,0 +1,123 @@
+/*


have we tried to recompile qnnpack code with c++ - then we can drop this and instead rely on main PyTorch's logging

We can definitely do that but we are under somewhat of a time constraint here. What was supposed to be a simple change (add support for runtime quantization) has already taken a long time because of all this back and forth, but I can definitely make this change if you have a strong preference.

Let's try to make this PR a simple copy and do meaningful changes in separate PRs.
Is it a simple compiler change or do we need fix syntax / measure perf impact / etc?
Sounds like something nice to have - I feel if we have new features to be written in c++ then it's more urgent change, otherwise we can try it after finishing other critical perf work?

Please refer to my comment above:

OK so I tried compiling with C++, but it's a good deal of work. The compiler is complaining about jumps (use of goto to be exact) bypassing variable initializations, and use of 'restrict' (as it's not a C++ keyword.) All of this can be worked around (using __restrict for instance instead of restrict and initializing all these variables to null / zero prior at them being jumped over), but it's a good deal of work. Making these changes is actually one thing and not terribly difficult, but making sure they do not introduce any hidden bugs is a whole another can of worms that I would rather not open at this point.

https://gist.github.com/AshkanAliabadi/d7e78ce6c0ea6e9b092d71a9bb96b9de

ezyang · 2019-09-04T00:14:57Z

The PR description on this change is insufficient. In private communication you have given more information about why this is being done, but this needs to be recorded in the PR itself, so that people can easily find the information after the fact.

ezyang · 2019-09-04T00:30:02Z

My questions are primarily procedural:

What is the long term plan for this fork? Based on Users/ashkan/requantize QNNPACK#66 it doesn't look like you intend to ever upstream this as a compile time option. QNNPACK seems to be a low maintenance library, but judging from the commit history, maintenance does happen. Are upstream fixes going to make it back to this fork, or is this fork just striking it out on its own? If you strike it out on your own, we are responsible for the long term maintenance of this copy. Do we have the appropriate knowledge and bandwidth to do so? (Standard litmus test: you need to get a code review on a functional change to this fork of QNNPACK, in this repository. Who can you get it from in a timely manner? Is this @ajtulloch?)
You haven't deleted the old copy of qnnpack. Based on @ajtulloch's comment in the old PR, it sounds like you want to end up in a place where this version of qnnpack is closely integrated with facilities in PyTorch. When can the old copy of qnnpack be deleted? Or if it can't be deleted, why not?
I am a random developer who happens to need to interact with our integration with qnnpack. At some point, I realize that we actually have two copies of qnnpack in the project: pytorch_qnnpack and qnnpack. Where can I find out what the difference is? How do I know which one is appropriate for my use? There appears to be zero documentation about what is going on here. (To make matters worse: this fork of qnnpack appears to already come with the functional changes proposed from Users/ashkan/requantize QNNPACK#66 ; so I cannot even run a git diff to see what the changes are.)
What is the testing strategy?

AshkanAliabadi · 2019-09-04T01:41:42Z

Thank you for your comments @ezyang. We synched offline but for the record, the motivation for this move, and our long-term commitment to maintaining and integrating this code into ATen and PyTorch is described in the issue below:

#25621

ljk53

Let's land it to keep the ball rolling.

facebook-github-bot

@AshkanAliabadi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@AshkanAliabadi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: The motivation for this move, and our long-term commitment to maintaining and integrating this code into ATen is described in the issue below: pytorch/pytorch#25621 Pull Request resolved: pytorch/pytorch#25500 Test Plan: QNNPack unit tests, as follows: OSS: x86: mkdir build; cd build; cmake ..; make all -j16 && make test All 26 unit tests pass, both when built with ADD_DEFINITIONS(-DPYTORCH_QNNPACK_RUNTIME_QUANTIZATION=0) and ADD_DEFINITIONS(-DPYTORCH_QNNPACK_RUNTIME_QUANTIZATION=1) ARM: Make sure you have an android device available to adb either through one world or directly connected. To compile and push do $> adb shell mkdir /data/qnnpack && ./scripts/build-android-arm64.sh && adb push ./build/android/arm64-v8a/*-test /data/qnnpack To execute tests, first $> adb shell to login into the device, then run all the tests by $> for t in $(ls /data/qnnpack); do /data/qnnpack/$t; done Repeat the exact same process with ADD_DEFINITIONS(-DPYTORCH_QNNPACK_RUNTIME_QUANTIZATION=0), and ADD_DEFINITIONS(-DPYTORCH_QNNPACK_RUNTIME_QUANTIZATION=1) Repeat the exact same process with ./scripts/build-android-armv7.sh for AARCH32. Reviewed By: ljk53 Differential Revision: D17194732 Pulled By: AshkanAliabadi fbshipit-source-id: 9e627338ebd63aa917a36b717618c0643ccf40c8

facebook-github-bot · 2019-09-08T01:04:17Z

@AshkanAliabadi merged this pull request in 825f471.

AshkanAliabadi requested review from supriyar, dzhulgakov and ljk53 August 30, 2019 22:26

AshkanAliabadi commented Aug 30, 2019

View reviewed changes

supriyar requested a review from ajtulloch August 30, 2019 22:32

supriyar reviewed Aug 30, 2019

View reviewed changes

aten/src/ATen/native/quantized/cpu/qadd.cpp Outdated Show resolved Hide resolved

AshkanAliabadi commented Aug 31, 2019

View reviewed changes

dzhulgakov reviewed Sep 3, 2019

View reviewed changes

ezyang self-requested a review September 4, 2019 00:14

ljk53 approved these changes Sep 4, 2019

View reviewed changes

facebook-github-bot reviewed Sep 4, 2019

View reviewed changes

facebook-github-bot reviewed Sep 5, 2019

View reviewed changes

Ashkan Aliabadi added 2 commits September 5, 2019 22:06

Fork QNNPACK into aten/src/ATen/native/quantized/cpu/qnnpack

f4599da

Subtract quantization zero point at runtime.

251cad9

facebook-github-bot closed this in 825f471 Sep 7, 2019

facebook-github-bot added the merged label Sep 8, 2019

mruberry added the Merged label Oct 28, 2020

Fork QNNPACK into aten/src/ATen/native/quantized/cpu/qnnpack #25500

Fork QNNPACK into aten/src/ATen/native/quantized/cpu/qnnpack #25500

Uh oh!

Conversation

AshkanAliabadi commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AshkanAliabadi Aug 30, 2019 • 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!

Uh oh!

Choose a reason for hiding this comment

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!

ezyang commented Sep 4, 2019

Uh oh!

ezyang commented Sep 4, 2019

Uh oh!

AshkanAliabadi commented Sep 4, 2019

Uh oh!

ljk53 left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 8, 2019

Uh oh!

Uh oh!

AshkanAliabadi commented Aug 30, 2019 •

edited

Loading

AshkanAliabadi Aug 30, 2019 •

edited

Loading