-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Include all mkldnn headers in CD builds #18355
Conversation
Hey @leezu , Thanks for submitting the PR
CI supported jobs: [centos-gpu, windows-gpu, unix-cpu, miscellaneous, website, unix-gpu, sanity, centos-cpu, clang, windows-cpu, edge] Note: |
set(DNNL_ENABLE_CONCURRENT_EXEC ON CACHE INTERNAL "" FORCE) | ||
|
||
if(NOT USE_OPENMP) | ||
function(load_mkldnn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why need put them into a function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functions have their own scope in cmake. We don't want to change any of the settings outside of this function (such as the install target name for the include files)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's really about setting the CMAKE_INSTALL_INCLUDEDIR
differently for mkldnn which is introduced in this PR
include_directories(3rdparty/mkldnn/include) | ||
include_directories(${PROJECT_BINARY_DIR}/3rdparty/mkldnn/include) | ||
add_definitions(-DMXNET_USE_MKLDNN=1) | ||
list(APPEND mxnet_LINKER_LIBS dnnl) | ||
set(INSTALL_MKLDNN ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it's removed? @yzhliu FYI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable isn't used anywhere but in this line
* Fix cmake mkldnn install target. Previously mkldnn headers are installed to CMAKE_INSTALL_INCLUDEDIR instead of CMAKE_INSTALL_INCLUDEDIR/mkldnn * Fix pypi_package.sh pip/setup.py for mkldnn builds
* Fix cmake mkldnn install target. Previously mkldnn headers are installed to CMAKE_INSTALL_INCLUDEDIR instead of CMAKE_INSTALL_INCLUDEDIR/mkldnn * Fix pypi_package.sh pip/setup.py for mkldnn builds
* cherry-pick: Fix missing MKLDNN headers (#18310) * Include all mkldnn headers in CD builds (#18355) * Fix cmake mkldnn install target. Previously mkldnn headers are installed to CMAKE_INSTALL_INCLUDEDIR instead of CMAKE_INSTALL_INCLUDEDIR/mkldnn * Fix pypi_package.sh pip/setup.py for mkldnn builds * Set CMAKE_CUDA_COMPILER in aarch64-linux-gnu-toolchain.cmake (#18713) CMAKE_CUDA_HOST_COMPILER will be reset if CMAKE_CUDA_COMPILER is not set as of cmake 3.17.3 See https://gitlab.kitware.com/cmake/cmake/-/issues/20826 Co-authored-by: Leonard Lausen <lausen@amazon.com>
* cherry-pick: Fix missing MKLDNN headers (apache#18310) * Include all mkldnn headers in CD builds (apache#18355) * Fix cmake mkldnn install target. Previously mkldnn headers are installed to CMAKE_INSTALL_INCLUDEDIR instead of CMAKE_INSTALL_INCLUDEDIR/mkldnn * Fix pypi_package.sh pip/setup.py for mkldnn builds * Set CMAKE_CUDA_COMPILER in aarch64-linux-gnu-toolchain.cmake (apache#18713) CMAKE_CUDA_HOST_COMPILER will be reset if CMAKE_CUDA_COMPILER is not set as of cmake 3.17.3 See https://gitlab.kitware.com/cmake/cmake/-/issues/20826 Co-authored-by: Leonard Lausen <lausen@amazon.com>
* Cherry-pick #18310 #18355 (#18608) * cherry-pick: Fix missing MKLDNN headers (#18310) * Include all mkldnn headers in CD builds (#18355) * Fix cmake mkldnn install target. Previously mkldnn headers are installed to CMAKE_INSTALL_INCLUDEDIR instead of CMAKE_INSTALL_INCLUDEDIR/mkldnn * Fix pypi_package.sh pip/setup.py for mkldnn builds * Set CMAKE_CUDA_COMPILER in aarch64-linux-gnu-toolchain.cmake (#18713) CMAKE_CUDA_HOST_COMPILER will be reset if CMAKE_CUDA_COMPILER is not set as of cmake 3.17.3 See https://gitlab.kitware.com/cmake/cmake/-/issues/20826 Co-authored-by: Leonard Lausen <lausen@amazon.com> * remove linux-gputoolchain Co-authored-by: MoisesHer <50716238+MoisesHer@users.noreply.github.com> Co-authored-by: Leonard Lausen <lausen@amazon.com>
Fixes #18120 specifically #18120 (comment)