Bob/cmake (#1118)

* fix mkldnn rpath

* fix compile warning

* close backends when exiting

* set backend output directory of backends to the ngraph output directory

* Aprocter/patch patch (#1119)

* Move more rpath stuff inside if(NOT APPLE)

* fix repatch problem with mkldnn library

* add updated patch command for older versions of cmake

CMakeLists.txt

@@ -194,6 +194,8 @@ endif()
 # External projects install directory
 #-----------------------------------------------------------------------------------------------
 
+set(NGRAPH_BUILD_DIR ${CMAKE_CURRENT_BINARY_DIR}/src/ngraph)
+
 set(EXTERNAL_INSTALL_DIR ${CMAKE_BINARY_DIR}/external)
 
 if(NOT DEFINED EXTERNAL_PROJECTS_ROOT)

cmake/external_mkldnn.cmake

@@ -91,6 +91,12 @@ if(${CMAKE_VERSION} VERSION_LESS 3.2)
         GIT_TAG ${MKLDNN_GIT_TAG}
         UPDATE_COMMAND ""
         CONFIGURE_COMMAND
+        # Patch gets mad if it applied for a second time so:
+        #    --forward tells patch to ignore if it has already been applied
+        #    --reject-file tells patch to not right a reject file
+        #    || exit 0 changes the exit code for the PATCH_COMMAND to zero so it is not an error
+        # I don't like it, but it works
+        PATCH_COMMAND patch ${EXTERNAL_PROJECTS_ROOT}/mkldnn/src/src/CMakeLists.txt --forward --reject-file=- -i ${CMAKE_SOURCE_DIR}/cmake/mkldnn.diff || exit 0
         # Uncomment below with any in-flight MKL-DNN patches
         # PATCH_COMMAND patch -p1 < ${CMAKE_SOURCE_DIR}/third-party/patches/mkldnn-cmake-openmp.patch
         CMAKE_ARGS
@@ -116,6 +122,12 @@ else()
         GIT_REPOSITORY ${MKLDNN_GIT_REPO_URL}
         GIT_TAG ${MKLDNN_GIT_TAG}
         UPDATE_COMMAND ""
+        # Patch gets mad if it applied for a second time so:
+        #    --forward tells patch to ignore if it has already been applied
+        #    --reject-file tells patch to not right a reject file
+        #    || exit 0 changes the exit code for the PATCH_COMMAND to zero so it is not an error
+        # I don't like it, but it works
+        PATCH_COMMAND patch ${EXTERNAL_PROJECTS_ROOT}/mkldnn/src/src/CMakeLists.txt --forward --reject-file=- -i ${CMAKE_SOURCE_DIR}/cmake/mkldnn.diff || exit 0
         # Uncomment below with any in-flight MKL-DNN patches
         # PATCH_COMMAND patch -p1 < ${CMAKE_SOURCE_DIR}/third-party/patches/mkldnn-cmake-openmp.patch
         CMAKE_ARGS
@@ -145,6 +157,11 @@ ExternalProject_Add_Step(
     DEPENDERS configure
     )
 
+add_custom_command(TARGET ext_mkldnn POST_BUILD
+    COMMAND ${CMAKE_COMMAND} -E copy_directory ${EXTERNAL_PROJECTS_ROOT}/mkldnn/lib ${NGRAPH_BUILD_DIR}
+    COMMENT "Move mkldnn libraries to ngraph build directory"
+)
+
 add_library(libmkldnn INTERFACE)
 target_include_directories(libmkldnn SYSTEM INTERFACE ${EXTERNAL_PROJECTS_ROOT}/mkldnn/include)
 target_link_libraries(libmkldnn INTERFACE

cmake/mkldnn.diff

+diff --git a/mkldnn/src/src/CMakeLists.txt b/../../CMakeLists.txt
+index 95fb94ab..9ffa8947 100755
+--- a/mkldnn/src/src/CMakeLists.txt
++++ b/../../CMakeLists.txt
+@@ -56,6 +56,9 @@ add_library(${TARGET_NAME} SHARED ${HEADERS} ${SOURCES})
+ #Add mkldnn.dll to execution PATH
+ set(CTESTCONFIG_PATH "${CTESTCONFIG_PATH}\;${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_BUILD_TYPE}" PARENT_SCOPE)
+ target_link_libraries(${TARGET_NAME} ${${TARGET_NAME}_LINKER_LIBS} ${EXTRA_LIBS})
++if(NOT APPLE)
++  set_property(TARGET ${TARGET_NAME} APPEND_STRING PROPERTY LINK_FLAGS " -Wl,--rpath,$ORIGIN")
++endif()
+ set_property(TARGET ${TARGET_NAME} PROPERTY CXX_STANDARD 11)
+ set_property(TARGET ${TARGET_NAME} PROPERTY CXX_STANDARD_REQUIRED ON)
+ set_property(TARGET ${TARGET_NAME} PROPERTY VERSION "${PROJECT_VERSION}.0")

src/ngraph/CMakeLists.txt

@@ -179,22 +179,9 @@ add_definitions("-DLIBRARY_VERSION=\"${NGRAPH_VERSION}\"")
 set_target_properties(ngraph PROPERTIES VERSION ${NGRAPH_VERSION} SOVERSION ${NGRAPH_API_VERSION})
 target_link_libraries(ngraph PUBLIC libjson)
 
-if (${NGRAPH_CPU_ENABLE})
-    get_target_property(BIN cpu_backend BINARY_DIR)
-    set_property(TARGET ngraph APPEND_STRING PROPERTY LINK_FLAGS " -Wl,--rpath,${BIN}")
-endif()
-
-if (${NGRAPH_GPU_ENABLE})
-    get_target_property(BIN gpu_backend BINARY_DIR)
-    set_property(TARGET ngraph APPEND_STRING PROPERTY LINK_FLAGS " -Wl,--rpath,${BIN}")
-endif()
-
-if (${NGRAPH_INTERPRETER_ENABLE})
-    get_target_property(BIN interpreter_backend BINARY_DIR)
-    set_property(TARGET ngraph APPEND_STRING PROPERTY LINK_FLAGS " -Wl,--rpath,${BIN}")
-endif()
-
 if (NOT APPLE)
+    set_property(TARGET ngraph APPEND_STRING PROPERTY LINK_FLAGS " -Wl,--rpath,$ORIGIN")
+
     # NGraph links against one or more libraries (ex. LLVM) but we don't want to
     # export these symbols as part of the DSO. This is a GNU ld (and derivatives) specific
     # option so making this portable is still an open issue. As a note for the future,

src/ngraph/function.cpp

@@ -130,7 +130,7 @@ void Function::set_temporary_pool_size(size_t size)
     m_temporary_pool_size = size;
 }
 
-std::ostream& ngraph::operator<<(std::ostream& out, const Function& f)
+std::ostream& operator<<(std::ostream& out, const Function& f)
 {
     out << "Function(" << f.get_name() << ")";
     return out;

src/ngraph/runtime/backend.cpp

@@ -24,6 +24,8 @@
 using namespace std;
 using namespace ngraph;
 
+std::unordered_map<string, void*> runtime::Backend::s_open_backends;
+
 bool runtime::Backend::register_backend(const string& name, shared_ptr<Backend> backend)
 {
     get_backend_map().insert({name, backend});
@@ -38,6 +40,10 @@ unordered_map<string, shared_ptr<runtime::Backend>>& runtime::Backend::get_backe
 
 runtime::Backend::~Backend()
 {
+    for (auto& p : s_open_backends)
+    {
+        dlclose(p.second);
+    }
 }
 
 void* runtime::Backend::open_shared_library(string type)
@@ -65,8 +71,10 @@ void* runtime::Backend::open_shared_library(string type)
         }
         else
         {
+            dlclose(handle);
             throw runtime_error("Failed to find create_backend function in library '" + name + "'");
         }
+        s_open_backends.insert({name, handle});
     }
     else
     {

src/ngraph/runtime/backend.hpp

@@ -85,6 +85,7 @@ namespace ngraph
         private:
             static void* open_shared_library(std::string type);
             static std::unordered_map<std::string, std::shared_ptr<Backend>>& get_backend_map();
+            static std::unordered_map<std::string, void*> s_open_backends;
         };
     }
 }

src/ngraph/runtime/cpu/CMakeLists.txt

@@ -104,6 +104,7 @@ if (NGRAPH_CPU_ENABLE)
 
     target_link_libraries(cpu_backend PUBLIC ngraph codegen libmkldnn libeigen libjson libtbb)
     target_include_directories(cpu_backend SYSTEM PUBLIC libmkldnn)
+    set_target_properties(cpu_backend PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${NGRAPH_BUILD_DIR})
 
     install(TARGETS cpu_backend LIBRARY DESTINATION ${NGRAPH_INSTALL_LIB})
 endif()

src/ngraph/runtime/gpu/CMakeLists.txt

@@ -56,6 +56,7 @@ if (NGRAPH_GPU_ENABLE)
         ${CUDA_LIBRARIES}
         ${CUDA_CUBLAS_LIBRARIES}
         ${CUDNN_LIBRARIES})
+    set_target_properties(gpu_backend PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${NGRAPH_BUILD_DIR})
 
     install(TARGETS gpu_backend LIBRARY DESTINATION ${NGRAPH_INSTALL_LIB})
 endif()

src/ngraph/runtime/interpreter/CMakeLists.txt

@@ -26,6 +26,7 @@ if (NGRAPH_INTERPRETER_ENABLE)
     add_library(interpreter_backend SHARED int_backend.cpp)
     set_target_properties(interpreter_backend PROPERTIES VERSION ${NGRAPH_VERSION} SOVERSION ${NGRAPH_API_VERSION})
     target_link_libraries(interpreter_backend PUBLIC ngraph)
+    set_target_properties(interpreter_backend PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${NGRAPH_BUILD_DIR})
 
     install(TARGETS interpreter_backend LIBRARY DESTINATION ${NGRAPH_INSTALL_LIB})
 endif()

test/CMakeLists.txt

@@ -119,6 +119,7 @@ target_link_libraries(unit-test ${CMAKE_DL_LIBS})
 if (NGRAPH_CPU_ENABLE)
     # The INTERPRETER backend is required for graph_partition, convolution, and backwards unit tests
     target_link_libraries(unit-test cpu_backend interpreter_backend)
+    target_link_libraries(unit-test libmkldnn)
 endif()
 
 if (NGRAPH_TBB_ENABLE)