-
Notifications
You must be signed in to change notification settings - Fork 536
Qualcomm AI Engine Direct - Support Qnn IR backend in online preparation #8876
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.
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
Qualcomm AI Engine Direct - Support Qnn IR backend in online preparation #8876
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/8876
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 8ffca64 with merge base 12ed924 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
On behalf of @DannyYuyang-quic. |
@pytorchbot label "release notes: qualcomm" |
Looks like there is CI failure |
50a3f7f
to
b0f66f8
Compare
Hi @cccclai The QnnIR-related header files in this PR, such as Sorry for not mentioning this and any inconvenience. |
Hi @cccclai , |
Can you share a bit what kind of regression does 2.30/2.31 have? |
We have tested Llama 1B with Lanai using QNN2.28, 2.30, and 2.31. |
I see. This PR bumps the qnn version in general, and we probably need to figure out how to manage these qnn versions. This PR probably will break our internal flow. Should we start an email to discuss about the versioning? |
b0f66f8
to
dfa286b
Compare
dfa286b
to
73eefc6
Compare
} | ||
return Error::Internal; | ||
} | ||
// std::vector<char> buffer(size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks for pointing that out!
@haowhsu-quic, could you please help me remove this line to trigger CI?
e05d75d
to
68365ef
Compare
68365ef
to
b6b9549
Compare
As discussed in the meetings, let's only bump the version in open source and error out when users try to run online prepare with versions older than 2.30 |
b6b9549
to
f750340
Compare
Hi @cccclai, I've pushed a new commit with the fixes, and it seems like everything is green. |
scripts/build_android_library.sh
Outdated
@@ -37,6 +37,7 @@ build_android_native_library() { | |||
cmake . -DCMAKE_INSTALL_PREFIX="${CMAKE_OUT}" \ | |||
-DCMAKE_TOOLCHAIN_FILE="${ANDROID_NDK}/build/cmake/android.toolchain.cmake" \ | |||
-DANDROID_ABI="${ANDROID_ABI}" \ | |||
-DANDROID_NATIVE_API_LEVEL=30 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use https://developer.android.com/ndk/guides/cmake#android_platform? (line below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you for your suggestion.
@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Can you add this change?
Also, I'm getting this error
Where does this file come from? |
@@ -70,6 +70,7 @@ endif() | |||
|
|||
include_directories( | |||
BEFORE ${_common_include_directories} ${QNN_SDK_ROOT}/include/QNN | |||
${QNN_SDK_ROOT}/share/QNN/converter/jni |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'm getting this error
In file included from fbcode/executorch/backends/qualcomm/runtime/backends/QnnContextCommon.cpp:10: buck-out/v2/gen/fbcode/5d832762563ef7a9/executorch/backends/qualcomm/runtime/__runtime__/buck-headers/executorch/backends/qualcomm/runtime/backends/QnnDlcManager.h:15:10: fatal error: 'QnnWrapperUtils.hpp' file not found 15 | #include "QnnWrapperUtils.hpp" | ^~~~~~~~~~~~~~~~~~~~~ 1 error generated.
Where does this file come from?
The QnnWrapperUtils.hpp
file is located under the path ${QNN_SDK_ROOT}/share/QNN/converter/jni.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Find it, looks like we miss some files for the internal build. I added a target for the files inside /share/QNN/converter/jni
, and now run into this error
ld.lld: error: undefined symbol: qnn_wrapper_api::strnDup(char const*, unsigned long)
>>> referenced by QnnWrapperUtils.cpp:75 (./third-party/qualcomm/qnn/qnn-2.28/share/QNN/converter/jni/QnnWrapperUtils.cpp:75)
>>> buck-out/v2/gen/fbsource/7d5d1c564400faae/third-party/qualcomm/qnn/qnn-2.28/__app_sources__/__objects__/share/QNN/converter/jni/QnnWrapperUtils.cpp.pic.o:(qnn_wrapper_api::deepCopyQnnTensors(Qnn_Tensor_t&, Qnn_Tensor_t&))
>>> referenced by QnnModel.cpp:403 (./third-party/qualcomm/qnn/qnn-2.28/share/QNN/converter/jni/QnnModel.cpp:403)
>>> buck-out/v2/gen/fbsource/7d5d1c564400faae/third-party/qualcomm/qnn/qnn-2.28/__app_sources__/__objects__/share/QNN/converter/jni/QnnModel.cpp.pic.o:(qnn_wrapper_api::getGraphInfoFromModels(qnn_wrapper_api::QnnModel*, unsigned int, qnn_wrapper_api::GraphInfo***))
Looks like
char *strnDup(const char *source, size_t maxlen);
is defined inside QnnModelPal.hpp
, where is the source function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah find it, nvm.
Do you know how much size increase it will add to the android? Also, is it for x86 only or both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ld.lld: error: undefined symbol: qnn_wrapper_api::strnDup(char const*, unsigned long) >>> referenced by QnnWrapperUtils.cpp:75 (./third-party/qualcomm/qnn/qnn-2.28/share/QNN/converter/jni/QnnWrapperUtils.cpp:75) >>> buck-out/v2/gen/fbsource/7d5d1c564400faae/third-party/qualcomm/qnn/qnn-2.28/__app_sources__/__objects__/share/QNN/converter/jni/QnnWrapperUtils.cpp.pic.o:(qnn_wrapper_api::deepCopyQnnTensors(Qnn_Tensor_t&, Qnn_Tensor_t&)) >>> referenced by QnnModel.cpp:403 (./third-party/qualcomm/qnn/qnn-2.28/share/QNN/converter/jni/QnnModel.cpp:403) >>> buck-out/v2/gen/fbsource/7d5d1c564400faae/third-party/qualcomm/qnn/qnn-2.28/__app_sources__/__objects__/share/QNN/converter/jni/QnnModel.cpp.pic.o:(qnn_wrapper_api::getGraphInfoFromModels(qnn_wrapper_api::QnnModel*, unsigned int, qnn_wrapper_api::GraphInfo***))
We don't need to include the QnnWrapperUtils.cpp
file; we only use the macro inside QnnWrapperUtils.hpp
.
Do you know how much size increase it will add to the android? Also, is it for x86 only or both?
Regarding the size increase, libqnn_executorch_backend.so will grow from 11.79MB to 12.19MB on android in total, based on a comparison between the mainline and this PR.
This is required for both x86 and android.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, hmm, do you mean I just need to add some files? I currently add a dependency for the buck target like this
cxx_library(
name = "app_sources",
srcs = glob([
"share/QNN/converter/jni/*.cpp",
]) + select({
"DEFAULT": glob([
"share/QNN/converter/jni/linux/*.cpp",
]),
"ovr_config//os:linux": glob([
"share/QNN/converter/jni/linux/*.cpp",
]),
"ovr_config//os:windows": glob([
"share/QNN/converter/jni/windows/*.cpp",
]),
}),
headers = glob([
"share/QNN/converter/jni/*.hpp",
]),
header_namespace = "",
exported_headers = subdir_glob([
("share/QNN/converter/jni", "*.hpp"),
]),
visibility = [
"PUBLIC",
],
deps = [
":api",
],
)
Can you help me understand what is required and what is not? If you have a better name, that's better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we consider making it optional in the future? For production, the runtime size budget can be limited sometimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, hmm, do you mean I just need to add some files? I currently add a dependency for the buck target like this
cxx_library(
name = "qnn_converter_sources",
exported_headers = subdir_glob([
("share/QNN/converter/jni", "QnnWrapperUtils.hpp"),
]),
visibility = [
"PUBLIC",
],
deps = [
":api",
],
)
Yes, we only need QnnWrapperUtils.hpp
, so I think our dependency can just be like this~ ?
Can we consider making it optional in the future? For production, the runtime size budget can be limited sometimes.
I see. Ideally, it would be great to make it optional, I will have the corresponding PR on this.
Can you also update these
|
Summary: pytorch#8876 add dependency on the QnnWrapperUtils.hpp, add the buck file here. Differential Revision: D73452937
Summary: pytorch#8876 add dependency on the QnnWrapperUtils.hpp, add the buck file here. Reviewed By: kirklandsign Differential Revision: D73452937
Summary: Pull Request resolved: pytorch#10370 pytorch#8876 add dependency on the QnnWrapperUtils.hpp, add the buck file here. Reviewed By: kirklandsign Differential Revision: D73452937
5d224d7
to
eaf22c9
Compare
@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
hmm seems like there is a merge conflict, can you rebase? |
eac8893
to
8611a4b
Compare
Hi @cccclai, |
I'm out of office and don't have access for now. @kirklandsign can you help a bit? |
@kirklandsign has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Hi @haowhsu-quic seems that you still need to rebase it. |
@kirklandsign has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Seems that we are very close. Just android ci.
Is that related to bumping android sdk version |
5334d52
to
4b4f6ce
Compare
- Support Qnn IR backend - Replace QCir with Dlc in online prepare flow - Add config for Saver backend - Block online preparation if the QNN version is below 2.30. - Fix SDK version checking - quant/dequant op breakage fix - Upgrade ANDROID_NATIVE_API_LEVEL from 23 to 30 - Add comments for qat_training_data/passes_job
4b4f6ce
to
8ffca64
Compare
Hi @kirklandsign, |
@kirklandsign has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Thank you @DannyYuyang-quic !! Let's import and do another round of internal CI! |
Summary
Test plan