Skip to content
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

Fix some link errors about NNPACK. #2824

Merged
merged 2 commits into from
Jul 18, 2017

Conversation

hedaoyuan
Copy link
Contributor

No description provided.

@hedaoyuan hedaoyuan requested a review from Xreki July 12, 2017 12:19
Copy link
Contributor

@Xreki Xreki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is almost LGTM, just with some doubts.

@@ -7,10 +7,24 @@ set(NNPACK_ROOT $ENV{NNPACK_ROOT} CACHE PATH "Folder contains NNPACK")
find_path(NNPACK_INC_DIR nnpack.h PATHS ${NNPACK_ROOT}/include)
find_library(NNPACK_LIB NAMES nnpack PATHS ${NNPACK_ROOT}/lib)
find_library(PTHREADPOOL_LIB NAMES pthreadpool PATHS ${NNPACK_ROOT}/lib)
find_library(NNPACK_UKERNELS_LIB NAMES nnpack_ukernels PATHS ${NNPACK_ROOT}/lib)
find_library(NNPACK_CPUFEATURES_LIB NAMES cpufeatures PATHS ${NNPACK_ROOT}/lib)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cpufeatures is a library from ndk, and is special for Android. Just reminding here that we may have a better way to locate it in future.


set(NNPACK_LIBS)
list(APPEND NNPACK_LIBS ${NNPACK_LIB} ${PTHREADPOOL_LIB})
if (NNPACK_UKERNELS_LIB)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what case there is nnpack_ukernels, and what case there isn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Android, the nnpack compilation generates the libnnpack_ukernels.a file. If not, there is no such file, all *.o files have been packaged into the libnnpack.a file.

VLOG(3) << "Number of threads "
<< pthreadpool_get_threads_count(threadpool_);
}
create_nnpack_threadpool();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this way, we cannot destroy the threadpool_ explicitly, and leave the work to OS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this may not very good. However, the previous version, each NNPACKConvFunction object has a threadpool_ is a bug. When the program running, it will lead to creating many of threads. So, I changed threadpool_ to a static variable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can define threadpool like this:

template <DeviceType Device>
class NNPACKConvFunction : public ConvFunctionBase {
  ...
private:
  struct ThreadPool {
    ThreadPool() : threadpool_(nullptr) {}
    ~ThreadPool() {
      if (threadpool_) {
        pthreadpool_destroy(threadpool_);
      }
    }
    void create(int num_threads) {
      if (num_threads > 0 && threadpool_ == nullptr) {
        threadpool_ = pthreadpool_create(num_threads);		
        VLOG(3) << "Number of threads "		
                      << pthreadpool_get_threads_count(threadpool_);		
      }
    }

    pthreadpool_t threadpool_;
  };

private:
  static ThreadPool pool_;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is better, can you fix it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

@hedaoyuan hedaoyuan merged commit f146b03 into PaddlePaddle:develop Jul 18, 2017
@Xreki Xreki added the Android label Sep 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants