[pytorch] remove pthreadpool dependency in aten/CMake#25894
Closed
ljk53 wants to merge 2 commits intogh/ljk53/38/basefrom
Closed
[pytorch] remove pthreadpool dependency in aten/CMake#25894ljk53 wants to merge 2 commits intogh/ljk53/38/basefrom
ljk53 wants to merge 2 commits intogh/ljk53/38/basefrom
Conversation
Summary: NNPack/QNNPack both depend on a third-party library "pthreadpool". There are two versions of "pthreadpool" implementation, one is the default implementation under third-party/pthreadpool, the other is caffe2 custom implementation under caffe2/utils/threadpool. Both implementations share the same interface (as defined by pthreadpool headers). Usually only one version of pthreadpool should be linked into libtorch. If QNNPACK_CUSTOM_THREADPOOL/NNPACK_CUSTOM_THREADPOOL are set to true, then QNNPack/NNPack will not link third-party/pthreadpool - they will expect the caller (libtorch) to link correct version of pthreadpool; otherwise they will bring in the default pthreadpool implementation. Looks like libtorch cmake already sets both macros to true in Dependencies.cmake and External/nnpack.cmake. And currently libtorch mobile build includes the caffe2/utils/threadpool pthreadpool implementation. So it shouldn't try to explicitly link default pthreadpool target in aten/CMake in this AT_NNPACK_ENABLED section. Test Plan: - Before this diff, libtorch.so links libpthreadpool.a: ``` LINK_LIBRARIES = lib/libc10.so lib/libqnnpack.a lib/libnnpack.a lib/libcpuinfo.a -llog -ldl -lm lib/libnnpack.a lib/libpthreadpool.a lib/libcpuinfo.a lib/libclog.a -llog -latomic -lm ``` - After this diff, libtorch.so no longer links libpthreadpool.a: ``` LINK_LIBRARIES = lib/libc10.so lib/libqnnpack.a lib/libnnpack.a lib/libcpuinfo.a -llog -ldl -lm lib/libnnpack.a lib/libcpuinfo.a lib/libclog.a -llog -latomic -lm ``` - Tried the following combinations to make sure things work as expected: * remove caffe2/utils/threadpool, remove libpthreadpool: link error; * keep caffe2/utils/threadpool, remove libpthreadpool: no link error; * remove caffe2/utils/threadpool, add back libpthreadpool: no link error;
This was referenced Sep 10, 2019
Closed
ljk53
added a commit
that referenced
this pull request
Sep 10, 2019
Summary: NNPack/QNNPack both depend on a third-party library "pthreadpool". There are two versions of "pthreadpool" implementation, one is the default implementation under third-party/pthreadpool, the other is caffe2 custom implementation under caffe2/utils/threadpool. Both implementations share the same interface (as defined by pthreadpool headers). Usually only one version of pthreadpool should be linked into libtorch. If QNNPACK_CUSTOM_THREADPOOL/NNPACK_CUSTOM_THREADPOOL are set to true, then QNNPack/NNPack will not link third-party/pthreadpool - they will expect the caller (libtorch) to link correct version of pthreadpool; otherwise they will bring in the default pthreadpool implementation. Looks like libtorch cmake already sets both macros to true in Dependencies.cmake and External/nnpack.cmake. And currently libtorch mobile build includes the caffe2/utils/threadpool pthreadpool implementation. So it shouldn't try to explicitly link default pthreadpool target in aten/CMake in this AT_NNPACK_ENABLED section. Test Plan: - Before this diff, libtorch.so links libpthreadpool.a: ``` LINK_LIBRARIES = lib/libc10.so lib/libqnnpack.a lib/libnnpack.a lib/libcpuinfo.a -llog -ldl -lm lib/libnnpack.a lib/libpthreadpool.a lib/libcpuinfo.a lib/libclog.a -llog -latomic -lm ``` - After this diff, libtorch.so no longer links libpthreadpool.a: ``` LINK_LIBRARIES = lib/libc10.so lib/libqnnpack.a lib/libnnpack.a lib/libcpuinfo.a -llog -ldl -lm lib/libnnpack.a lib/libcpuinfo.a lib/libclog.a -llog -latomic -lm ``` - Tried the following combinations to make sure things work as expected: * remove caffe2/utils/threadpool, remove libpthreadpool: link error; * keep caffe2/utils/threadpool, remove libpthreadpool: no link error; * remove caffe2/utils/threadpool, add back libpthreadpool: no link error; ghstack-source-id: 96b4a62 Pull Request resolved: #25894
Summary: NNPack/QNNPack both depend on a third-party library "pthreadpool". There are two versions of "pthreadpool" implementation, one is the default implementation under third-party/pthreadpool, the other is caffe2 custom implementation under caffe2/utils/threadpool. Both implementations share the same interface (as defined by pthreadpool headers). Usually only one version of pthreadpool should be linked into libtorch. If QNNPACK_CUSTOM_THREADPOOL/NNPACK_CUSTOM_THREADPOOL are set to true, then QNNPack/NNPack will not link third-party/pthreadpool - they will expect the caller (libtorch) to link correct version of pthreadpool; otherwise they will bring in the default pthreadpool implementation. Looks like libtorch cmake already sets both macros to true in Dependencies.cmake and External/nnpack.cmake. And currently libtorch mobile build includes the caffe2/utils/threadpool pthreadpool implementation. So it shouldn't try to explicitly link default pthreadpool target in aten/CMake in this AT_NNPACK_ENABLED section. Test Plan: - Before this diff, libtorch.so links libpthreadpool.a: ``` LINK_LIBRARIES = lib/libc10.so lib/libqnnpack.a lib/libnnpack.a lib/libcpuinfo.a -llog -ldl -lm lib/libnnpack.a lib/libpthreadpool.a lib/libcpuinfo.a lib/libclog.a -llog -latomic -lm ``` - After this diff, libtorch.so no longer links libpthreadpool.a: ``` LINK_LIBRARIES = lib/libc10.so lib/libqnnpack.a lib/libnnpack.a lib/libcpuinfo.a -llog -ldl -lm lib/libnnpack.a lib/libcpuinfo.a lib/libclog.a -llog -latomic -lm ``` - Tried the following combinations to make sure things work as expected: * remove caffe2/utils/threadpool, remove libpthreadpool: link error; * keep caffe2/utils/threadpool, remove libpthreadpool: no link error; * remove caffe2/utils/threadpool, add back libpthreadpool: no link error; Pull Request resolved: #25894 Differential Revision: [D17279723](https://our.internmc.facebook.com/intern/diff/D17279723)
ezyang
approved these changes
Sep 10, 2019
zdevito
pushed a commit
to zdevito/ATen
that referenced
this pull request
Sep 10, 2019
Summary: Pull Request resolved: pytorch/pytorch#25894 NNPack/QNNPack both depend on a third-party library "pthreadpool". There are two versions of "pthreadpool" implementation, one is the default implementation under third-party/pthreadpool, the other is caffe2 custom implementation under caffe2/utils/threadpool. Both implementations share the same interface (as defined by pthreadpool headers). Usually only one version of pthreadpool should be linked into libtorch. If QNNPACK_CUSTOM_THREADPOOL/NNPACK_CUSTOM_THREADPOOL are set to true, then QNNPack/NNPack will not link third-party/pthreadpool - they will expect the caller (libtorch) to link correct version of pthreadpool; otherwise they will bring in the default pthreadpool implementation. Looks like libtorch cmake already sets both macros to true in Dependencies.cmake and External/nnpack.cmake. And currently libtorch mobile build includes the caffe2/utils/threadpool pthreadpool implementation. So it shouldn't try to explicitly link default pthreadpool target in aten/CMake in this AT_NNPACK_ENABLED section. Test Plan: - Before this diff, libtorch.so links libpthreadpool.a: ``` LINK_LIBRARIES = lib/libc10.so lib/libqnnpack.a lib/libnnpack.a lib/libcpuinfo.a -llog -ldl -lm lib/libnnpack.a lib/libpthreadpool.a lib/libcpuinfo.a lib/libclog.a -llog -latomic -lm ``` - After this diff, libtorch.so no longer links libpthreadpool.a: ``` LINK_LIBRARIES = lib/libc10.so lib/libqnnpack.a lib/libnnpack.a lib/libcpuinfo.a -llog -ldl -lm lib/libnnpack.a lib/libcpuinfo.a lib/libclog.a -llog -latomic -lm ``` - Tried the following combinations to make sure things work as expected: * remove caffe2/utils/threadpool, remove libpthreadpool: link error; * keep caffe2/utils/threadpool, remove libpthreadpool: no link error; * remove caffe2/utils/threadpool, add back libpthreadpool: no link error; Pull Request resolved: pytorch/pytorch#25894 Reviewed By: dreiss Differential Revision: D17279723 Pulled By: ljk53 fbshipit-source-id: ae5aa7ca7283a276ecf1e2140bad0a6af3efdb3a
Contributor
ljk53
added a commit
to ljk53/pytorch
that referenced
this pull request
Sep 17, 2019
This reverts commit 4bd9ddb.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
Summary:
NNPack/QNNPack both depend on a third-party library "pthreadpool". There
are two versions of "pthreadpool" implementation, one is the default
implementation under third-party/pthreadpool, the other is caffe2 custom
implementation under caffe2/utils/threadpool. Both implementations share
the same interface (as defined by pthreadpool headers).
Usually only one version of pthreadpool should be linked into libtorch.
If QNNPACK_CUSTOM_THREADPOOL/NNPACK_CUSTOM_THREADPOOL are set to true,
then QNNPack/NNPack will not link third-party/pthreadpool - they will
expect the caller (libtorch) to link correct version of pthreadpool;
otherwise they will bring in the default pthreadpool implementation.
Looks like libtorch cmake already sets both macros to true in
Dependencies.cmake and External/nnpack.cmake. And currently libtorch
mobile build includes the caffe2/utils/threadpool pthreadpool
implementation. So it shouldn't try to explicitly link default
pthreadpool target in aten/CMake in this AT_NNPACK_ENABLED section.
Test Plan:
Pull Request resolved: #25894
Differential Revision: D17279723