Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Add option to choose between OMP implementations #15808

Closed
wants to merge 4 commits into from
Closed

Add option to choose between OMP implementations #15808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
583a2bb
Add option to choose between OMP implementations
larroy Aug 21, 2019
c7fd3a8
Switch submodule for CI
larroy Aug 29, 2019
80d2216
remove ws
larroy Sep 6, 2019
27a2a12
retrigger ci
larroy Sep 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitmodules
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[submodule "3rdparty/dmlc-core"]
path = 3rdparty/dmlc-core
url = https://github.com/dmlc/dmlc-core.git
url = https://github.com/larroy/dmlc-core.git
[submodule "3rdparty/ps-lite"]
path = 3rdparty/ps-lite
url = https://github.com/dmlc/ps-lite
Expand Down
2 changes: 1 addition & 1 deletion 3rdparty/dmlc-core
Open in desktop
Submodule dmlc-core updated 29 files
+12 −0 .travis.yml
+26 −8 CMakeLists.txt
+4 −0 README.md
+13 −0 cmake/Modules/FindASan.cmake
+13 −0 cmake/Modules/FindLSan.cmake
+13 −0 cmake/Modules/FindTSan.cmake
+58 −0 cmake/Sanitizer.cmake
+1 −1 cmake/build_config.h.in
+1 −1 cmake/gtest_cmake.in
+4 −0 include/dmlc/base.h
+5 −5 include/dmlc/build_config_default.h
+7 −6 include/dmlc/endian.h
+1 −1 include/dmlc/omp.h
+1 −1 include/dmlc/parameter.h
+62 −32 include/dmlc/threadediter.h
+1 −0 include/dmlc/type_traits.h
+6 −6 scripts/packages.mk
+28 −0 scripts/travis/s390x/Dockerfile
+9 −0 scripts/travis/s390x/build_via_cmake.sh
+86 −0 scripts/travis/s390x/ci_build.sh
+43 −0 scripts/travis/s390x/entrypoint.sh
+22 −1 scripts/travis/travis_script.sh
+4 −4 src/config.cc
+1 −1 test/logging_test.cc
+1 −1 test/unittest/CMakeLists.txt
+9 −0 test/unittest/unittest_inputsplit.cc
+23 −0 test/unittest/unittest_serializer.cc
+0 −2 test/unittest/unittest_thread_group.cc
+1 −1 tracker/dmlc_tracker/mpi.py
19 changes: 11 additions & 8 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ mxnet_option(USE_CUDA "Build with CUDA support" ON)
mxnet_option(USE_OLDCMAKECUDA "Build with old cmake cuda" OFF)
mxnet_option(USE_NCCL "Use NVidia NCCL with CUDA" OFF)
mxnet_option(USE_OPENCV "Build with OpenCV support" ON)
mxnet_option(USE_OPENMP "Build with Openmp support" ON)
mxnet_option(USE_OPENMP "Build with Openmp support" ON) # OFF | ON | PLATFORM | BUNDLED
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between ON and the rest? Can you add some comments here?

mxnet_option(USE_CUDNN "Build with cudnn support" ON) # one could set CUDNN_ROOT for search path
mxnet_option(USE_SSE "Build with x86 SSE instruction support" ON IF NOT ARM)
mxnet_option(USE_F16C "Build with x86 F16C instruction support" ON) # autodetects support if ON
Expand Down Expand Up @@ -432,14 +432,13 @@ endif()

# ---[ OpenMP
if(USE_OPENMP)
find_package(OpenMP REQUIRED)
# This should build on Windows, but there's some problem and I don't have a Windows box, so
# could a Windows user please fix?
if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/3rdparty/openmp/CMakeLists.txt
AND SYSTEM_ARCHITECTURE STREQUAL "x86_64"
AND NOT MSVC
AND NOT CMAKE_CROSSCOMPILING)

if(USE_OPENMP STREQUAL "BUNDLED" AND EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/3rdparty/openmp/CMakeLists.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

The first check and the others should be separate to throw appropriate error messages

Copy link
Contributor Author

@larroy larroy Sep 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your review. Can you explain what you mean exactly? A concrete request would help, as this is a followup on previous review requests that might be conflicting.

Copy link
Contributor

Choose a reason for hiding this comment

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

If someone uses the option "bundled" but the "exists" check fails, they'd receive an error message that they selected an invalid mode.

AND SYSTEM_ARCHITECTURE STREQUAL "x86_64"
AND NOT MSVC
AND NOT CMAKE_CROSSCOMPILING)
message("Using bundlded LLVM OpenMP from 3rdparty")
# Intel/llvm OpenMP: https://github.com/llvm-mirror/openmp
set(OPENMP_STANDALONE_BUILD TRUE)
set(LIBOMP_ENABLE_SHARED TRUE)
Expand All @@ -452,14 +451,18 @@ if(USE_OPENMP)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${OpenMP_C_FLAGS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}")
add_definitions(-DMXNET_USE_OPENMP=1)
else()
elseif(USE_OPENMP STREQUAL "PLATFORM" OR USE_OPENMP STREQUAL "ON")
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between on and platform

Copy link
Contributor Author

@larroy larroy Sep 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

They are the same, for backwards compatibility, meaning if someone had USE_OPENMP=ON will continue working with the OMP implementation which has been shown to be most stable.

find_package(OpenMP REQUIRED)
message("Using platform provided OpenMP")
if(OPENMP_FOUND)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${OpenMP_C_FLAGS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${OpenMP_EXE_LINKER_FLAGS}")
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${OpenMP_EXE_LINKER_FLAGS}")
add_definitions(-DMXNET_USE_OPENMP=1)
endif()
else()
message(FATAL_ERROR "USE_OPENMP takes values [PLATFORM, BUNDLED, OFF]")
Copy link
Contributor

Choose a reason for hiding this comment

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

"USE_OPENMP takes values [PLATFORM, BUNDLED, OFF, ON]")?

endif()
elseif(UNIX AND NOT ANDROID)
list(APPEND mxnet_LINKER_LIBS pthread)
Expand Down