Caffe2 ARM ComputeLibrary integration #2015

jerryzh168 · 2018-02-23T01:51:28Z

No description provided.

…o caffe2-acl

facebook-github-bot · 2018-02-23T01:56:54Z

@jerryzh168 has updated the pull request. View: changes

Maratyszcza · 2018-02-23T01:57:36Z

caffe2/mobile/contrib/CMakeLists.txt

@@ -1,5 +1,8 @@
 add_subdirectory(ios)
 add_subdirectory(opengl)
+if (USE_ARM_COMPUTE AND ANDROID)
+	add_subdirectory(arm-compute)


Please convert to spaces

Maratyszcza · 2018-02-23T01:59:38Z

caffe2/mobile/contrib/arm-compute/CMakeLists.txt

@@ -0,0 +1,11 @@
+if(USE_ARM_COMPUTE AND ANDROID)


Typically we just disable options on unsupported platforms, e.g. see https://github.com/caffe2/caffe2/blob/1f9e79f920a22aed1a370bf5faec1ce5e1ef73d7/cmake/Dependencies.cmake#L460-L465

Maratyszcza · 2018-02-23T02:00:26Z

caffe2/mobile/contrib/arm-compute/README.md

+brew install scons
+```
+
+use the build\_android.sh:


Which word? build_android.sh? that is probably for markdown..

Maratyszcza · 2018-02-23T02:02:57Z

caffe2/mobile/contrib/arm-compute/core/operator.cc

+
+CAFFE_DEFINE_REGISTRY(GLOperatorRegistry, OperatorBase, const OperatorDef &,
+                      Workspace *);
+CAFFE_REGISTER_DEVICE_TYPE(DeviceType::OPENGL, GLOperatorRegistry);


This probably collides with our OpenGL ES 3.0 backend

Is OpenGL ES 3.0 backend removed already?

No, its still in the source tree, and potentially even being built.

Should we change it to something else? I think DeviceType::OPENGL is a pre-defined device type.

I think we should use a different device type, similar to how we use different device type for MKL-DNN (which, albeit runs on CPU, keeps tensors in a non-standard layout)

cc @hlu1 to check if this can conflict with OpenGL ES 3.0

#ifdef CAFFE2_OPENGL_BACKEND #error Can only build one OpenGL backend at a time. #else #define CAFFE2_OPENGL_BACKEND #endif

OpenGL ES 3.0 backend does not use DeviceType. It prefixes operator names with OpenGL instead

bwasti · 2018-02-23T01:55:05Z

CMakeLists.txt

@@ -25,6 +25,7 @@ cmake_dependent_option(
    "NOT BUILD_SHARED_LIBS" OFF)
 option(BUILD_OBSERVERS "Build performance observers/loggers in caffe2/share/observers directory" OFF)
 option(BUILD_TEST "Build C++ test binaries (need gtest and gbenchmark)" ON)
+option(USE_ARM_COMPUTE "Use ARM Compute Library" ON)


lets turn this off by default, as it has a scons dependency at the moment

bwasti · 2018-02-23T02:00:20Z

.gitmodules

@@ -61,6 +61,9 @@
 [submodule "third_party/python-six"]
 	path = third_party/python-six
 	url = https://github.com/benjaminp/six.git
+[submodule "third_party/ComputeLibrary"]
+	path = third_party/ComputeLibrary
+	url = https://github.com/ARM-software/ComputeLibrary


.git for consistency

bwasti · 2018-02-23T02:01:36Z

scripts/build_android.sh


 # Disable unused dependencies
 CMAKE_ARGS+=("-DUSE_CUDA=OFF")
-CMAKE_ARGS+=("-DUSE_GFLAGS=OFF")


can you add this back?

bwasti · 2018-02-23T02:02:03Z

scripts/build_android.sh

 # Use android-cmake to build Android project from CMake.
 CMAKE_ARGS+=("-DCMAKE_TOOLCHAIN_FILE=$ANDROID_NDK/build/cmake/android.toolchain.cmake")

 # Don't build artifacts we don't need
-CMAKE_ARGS+=("-DBUILD_TEST=OFF")
+CMAKE_ARGS+=("-DBUILD_TEST=ON")


I think this is on by default, either way the comment above doesn't make sense anymore

Should we turn on the test by default?

Better keep it off. When needed, tests can be built with scripts/build_android.sh -DBUILD_TEST=ON

bwasti · 2018-02-23T02:02:35Z

scripts/build_android.sh

+CMAKE_ARGS+=($@)
+USE_ARM_COMPUTE=false
+for arg in ${CMAKE_ARGS[@]};do
+  if [ $arg == "-DUSE_ARM_COMPUTE=ON" ];then


there are a couple other ways to set things "true" in cmake, it might be good to check for all of those

-DUSE_ARM_COMPUTE=1? what else?

bwasti · 2018-02-23T02:02:54Z

scripts/build_android.sh


 cmake "$CAFFE2_ROOT" \
    -DCMAKE_INSTALL_PREFIX=../install \
    -DCMAKE_BUILD_TYPE=Release \
-    "${CMAKE_ARGS[@]}"
+    -G Ninja \


this should not be a default

some changes got lost, will revert to the master

bwasti · 2018-02-23T02:03:08Z

scripts/build_android.sh

-else
-  cmake --build . -- "-j$(nproc)"
-fi
+ninja


lets leave this bit

Maratyszcza · 2018-02-23T02:06:50Z

caffe2/CMakeLists.txt

@@ -10,6 +10,7 @@ set(Caffe2_GPU_SRCS)
 # files for CPU and GPU tests respectively.
 set(Caffe2_CPU_TEST_SRCS)
 set(Caffe2_GPU_TEST_SRCS)
+set(Caffe2_MOBILE_TEST_SRCS)


Why do we need Caffe2_MOBILE_TEST_SRCS? IIUC, Caffe2_GPU_TEST_SRCS exist because it is built with a different compiler

So that we can build only those tests when we build on mobile? I'm not sure, @bwasti what do you think?

cc @Yangqing as well

yea, we can chuck everything into CPU_TEST_SRCS. semantically, we could rename them to CC_SRCS and NVCC_SRCS huh?

agree, CC_SRCS + NVCC_SRCS would make more sense

Maratyszcza · 2018-02-23T02:09:53Z

scripts/build_android.sh


 cmake "$CAFFE2_ROOT" \
    -DCMAKE_INSTALL_PREFIX=../install \
    -DCMAKE_BUILD_TYPE=Release \
-    "${CMAKE_ARGS[@]}"
+    -G Ninja \


I like the idea of switching to ninja, but lets leave it for a separate PR

Maratyszcza · 2018-02-23T02:11:37Z

Could you keep changes to CMakeLists to a minimum?

…o caffe2-acl

facebook-github-bot · 2018-02-23T02:25:32Z

@jerryzh168 has updated the pull request. View: changes

jerryzh168 · 2018-02-23T02:41:39Z

@Maratyszcza @bwasti could you review again? it would be great if we can merge this today

facebook-github-bot · 2018-02-23T02:42:53Z

@jerryzh168 has updated the pull request. View: changes

Maratyszcza · 2018-02-23T05:59:51Z

caffe2/CMakeLists.txt

@@ -169,7 +182,7 @@ if (BUILD_TEST)
          ${test_name} ${Caffe2_MAIN_LIBS} ${Caffe2_DEPENDENCY_LIBS}
          gtest_main)
    endif()
-    if (${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION} GREATER 3.0)
+    if (${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION} GREATER 3.0 AND NOT ANDROID)


Why the change? The sources still use cxx_range_for when building for android. target_compile_features tells CMake to set appropriate flags. Without it, compiler may default to c++98.

target_compile_features is not recognized before for some reason.. resolved now.

jerryzh168 · 2018-02-23T08:06:58Z

remaining issue device_type and Caffe2_MOBILE_TEST_SRCS, but I think we can modify these in another diff as long as it doesn't break the ci?

facebook-github-bot · 2018-02-23T08:10:34Z

@jerryzh168 has updated the pull request. View: changes

Maratyszcza · 2018-02-23T09:58:48Z

CMakeLists.txt

@@ -25,6 +25,7 @@ cmake_dependent_option(
    "NOT BUILD_SHARED_LIBS" OFF)
 option(BUILD_OBSERVERS "Build performance observers/loggers in caffe2/share/observers directory" OFF)
 option(BUILD_TEST "Build C++ test binaries (need gtest and gbenchmark)" ON)
+option(USE_ARM_COMPUTE "Use ARM Compute Library" OFF)


The name is ambiguous: users may think they have to enable it for any ARM builds. Please change to USE_ACL, USE_ARMCL, or USE_ARM_COMPUTE_LIBRARY

Maratyszcza · 2018-02-23T10:01:40Z

scripts/build_android.sh

 CMAKE_ARGS+=("-DBUILD_BINARY=OFF")
 CMAKE_ARGS+=("-DBUILD_PYTHON=OFF")
 CMAKE_ARGS+=("-DBUILD_SHARED_LIBS=OFF")
+
+if $USE_ARM_COMPUTE;then
+CMAKE_ARGS+=("-DANDROID_TOOLCHAIN_NAME=arm-linux-androideabi-4.9")


This line have the same effect as -DANDROID_TOOLCHAIN=gcc. See https://android.googlesource.com/platform/ndk/+/master/build/cmake/android.toolchain.cmake#103

Maratyszcza · 2018-02-23T10:02:06Z

scripts/build_android.sh

@@ -103,7 +115,7 @@ fi
 cmake "$CAFFE2_ROOT" \
    -DCMAKE_INSTALL_PREFIX=../install \
    -DCMAKE_BUILD_TYPE=Release \
-    "${CMAKE_ARGS[@]}"
+    "${CMAKE_ARGS[@]}" \


Maratyszcza · 2018-02-23T10:05:55Z

scripts/build_android.sh

@@ -52,6 +52,13 @@ CMAKE_ARGS=()
 # target architecture and need an exact version match.
 CMAKE_ARGS+=("-DCAFFE2_CUSTOM_PROTOC_EXECUTABLE=$CAFFE2_ROOT/build_host_protoc/bin/protoc")

+CMAKE_ARGS+=($@)
+USE_ARM_COMPUTE=false


Please remove this snippet, its not needed

Maratyszcza · 2018-02-23T10:08:53Z

cmake/Dependencies.cmake

+  set (ARM_COMPUTE_CORE_LIB "${CMAKE_CURRENT_BINARY_DIR}/libarm_compute_core.a")
+  set (ARM_COMPUTE_LIBS ${ARM_COMPUTE_LIB} ${ARM_COMPUTE_CORE_LIB})
+
+  add_custom_command(


The custom command doesn't have DEPENDS, so it won't rebuild the sources e.g. after a submodule update. Hopefully, ARM CL would just add CMakeLists, and we don't have to monkey-patch it.

Maratyszcza · 2018-02-23T10:12:26Z

cmake/Dependencies.cmake

@@ -464,6 +501,14 @@ if (USE_METAL)
  endif()
 endif()

+
+if (USE_ARM_COMPUTE)


This should be above the code that imports arm compute library targets from Scons. The motivation is to check only if (USE_ARM_COMPUTE), so if we need to enable this backend on other platforms, we need to patch only one snippet.

Maratyszcza

Please do the following before merging:

Remove Caffe2_MOBILE_TEST_SRCS, use Caffe2_CPU_TEST_SRCS
Remove code that sets and uses USE_ARM_COMPUTE from scripts/build_android.sh
Replace if (USE_ARM_COMPUTE AND ANDROID) with if (USE_ARM_COMPUTE). Only the snippet that disables ARMCL on non-Android platforms should check the platform.
Rename USE_ARM_COMPUTE option to something less ambiguous. Current name may suggest that this option is required for all builds for ARM.
Check that is builds with ARM Compute Library for ARMv7 and ARM64, and with -DBUILD_TEST=ON as well.

I didn't look much at C++ code, assume @bwasti understands it.

hlu1 · 2018-02-23T23:45:26Z

caffe2/mobile/contrib/arm-compute/core/context.h

+namespace caffe2 {
+
+typedef half_float::half half;
+typedef half DataType;


Do you need to check if float16 is supported on device?

facebook-github-bot · 2018-02-23T23:55:48Z

@jerryzh168 has updated the pull request. View: changes

bwasti

Let's rename the PR to be a bit more informative?

facebook-github-bot · 2018-02-24T00:09:45Z

@jerryzh168 has updated the pull request. View: changes

facebook-github-bot

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

jerryzh168 added 3 commits February 22, 2018 14:31

Release of Caffe2 ARM Compute Library Integration

07cd973

Release of Caffe2 ARM Compute Library Integration

266197b

Merge branch 'caffe2-acl' of https://github.com/jerryzh168/caffe2 int…

364f0dc

…o caffe2-acl

jerryzh168 requested review from Maratyszcza and bwasti February 23, 2018 01:51

Merge branch 'master' into caffe2-acl

28989df

This was referenced Feb 23, 2018

[auto] caffe2-pr-2015 onnx/onnx-caffe2#283

Open

[auto] caffe2-pr-2015 onnxbot/onnx-fb-universe#783

Closed

facebook-github-bot added the CLA Signed label Feb 23, 2018

Maratyszcza reviewed Feb 23, 2018

View reviewed changes

bwasti reviewed Feb 23, 2018

View reviewed changes

Maratyszcza reviewed Feb 23, 2018

View reviewed changes

jerryzh168 added 2 commits February 22, 2018 18:24

Comments

29bbc17

Merge branch 'caffe2-acl' of https://github.com/jerryzh168/caffe2 int…

bf0cf6c

…o caffe2-acl

More options for USE_ARM_COMPUTE

7d4b25f

Maratyszcza reviewed Feb 23, 2018

View reviewed changes

Comments

4018cbe

Maratyszcza reviewed Feb 23, 2018

View reviewed changes

Maratyszcza approved these changes Feb 23, 2018

View reviewed changes

hlu1 reviewed Feb 23, 2018

View reviewed changes

Comment

850a53a

bwasti approved these changes Feb 24, 2018

View reviewed changes

bwasti changed the title ~~Caffe2 acl~~ Caffe2 ARM ComputeLibrary integration Feb 24, 2018

Merge remote-tracking branch 'upstream/master' into caffe2-acl

a0db03d

jerryzh168 merged commit f543d91 into facebookarchive:master Feb 24, 2018

facebook-github-bot reviewed Feb 24, 2018

View reviewed changes

freedomtan mentioned this pull request Feb 26, 2018

Enable OpenGL build #1995

Closed

t-vi mentioned this pull request Jul 5, 2019

Know which function is used by conv and force to use a function pytorch/pytorch#22532

Open

Caffe2 ARM ComputeLibrary integration #2015

Caffe2 ARM ComputeLibrary integration #2015

Conversation

jerryzh168 commented Feb 23, 2018

facebook-github-bot commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maratyszcza commented Feb 23, 2018

facebook-github-bot commented Feb 23, 2018

jerryzh168 commented Feb 23, 2018

facebook-github-bot commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryzh168 commented Feb 23, 2018

facebook-github-bot commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maratyszcza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 23, 2018

bwasti left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 24, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment