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

Fix CUDNN detection for CMake build #17019

Merged
merged 2 commits into from
Dec 10, 2019
Merged

Fix CUDNN detection for CMake build #17019

merged 2 commits into from
Dec 10, 2019

Conversation

leezu
Copy link
Contributor

@leezu leezu commented Dec 9, 2019

Description

  • Use FindCUDNN.cmake instead of the previous macro
  • Previous macro did not detect CUDNN correctly on my system (Deep Learning AMI
    Ubuntu 18.04) and did not generate any warnings that cudnn could not be found.

With FindCUDNN.cmake introduced here, based on https://cmake.org/cmake/help/latest/module/FindPackageHandleStandardArgs.html, we will use the "standard" outputting methods to signify if cuDNN is found or missing.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Fix CUDNN detection for CMake build

Comments

CC @szha @yajiedesign @junrushao1994

@leezu leezu requested a review from szha as a code owner December 9, 2019 07:09
@leezu leezu force-pushed the cmakefixcudnn branch 2 times, most recently from 7a0b009 to 19af332 Compare December 9, 2019 07:34
cmake/Modules/FindCUDNN.cmake Outdated Show resolved Hide resolved
@leezu leezu force-pushed the cmakefixcudnn branch 2 times, most recently from a21e4a9 to 5df2368 Compare December 9, 2019 10:48
- Use FindCUDNN.cmake instead of the previous macro
- Previous macro did not detect CUDNN correctly on my system (Deep Learning AMI
  Ubuntu 18.04)
- We now explicitly fail if the user does not provide CUDNN and hasn't manually
  set -DUSE_CUDNN=0
@leezu
Copy link
Contributor Author

leezu commented Dec 9, 2019

If the lazy consensus for bumping cmake minimum version requirement on the mailing list passes, I'll open another PR tomorrow removing all custom Cuda handling logic. Only the FindCUDNN.cmake file introduced in this PR will remain.

@leezu leezu merged commit 8645b9a into apache:master Dec 10, 2019
@leezu leezu deleted the cmakefixcudnn branch December 10, 2019 02:16
@@ -502,13 +502,15 @@ add_subdirectory(${GTEST_ROOT})
find_package(GTest REQUIRED)

# cudnn detection
if(USE_CUDNN AND USE_CUDA)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This AND USE_CUDA should not have been removed inside this PR. It assumes cmake_dependent_option(USE_CUDNN "Build with cudnn support" ON "USE_CUDA" OFF) from #17018 is used. Unfortunately our tests don't test USE_CUDA=0 on a machine with CUDNN available, so the passed.

@yajiedesign I suggest either to either merge #17018 quickly or I'll create another PR to add back the AND USE_CUDA

yajiedesign pushed a commit to yajiedesign/mxnet that referenced this pull request Jan 5, 2020
…ranch

Fix CUDNN detection for CMake build (apache#17019)

Replace mxnet_option macro with standard CMAKE_DEPENDENT_OPTION (apache#17018)

Switch to modern CMake CUDA handling (apache#17031)

Introduce unified MXNET_CUDA_ARCH option to specify cuda architectures.
Previously cuda architecture setting was partially broken and different options
were applied to different parts of the build (CUDA_ARCH_NAME CUDA_ARCH_BIN
CUDA_ARCH_PTX and CUDA_ARCH_LIST).

Include FindCUDAToolkit from CMake 3.17, which replaces the deprecated FindCUDA
functionality for finding the cuda toolkit include directories and libraries.

Workaround for DLL size limitation on Windows (apache#16980)

* change windows build system.
add gen_warp cpp version
add add_custom_command to run warp_gen
add download cmake
add option
change option
add dynamic read mxnet dll
ptrendx pushed a commit that referenced this pull request Jan 5, 2020
Fix CUDNN detection for CMake build (#17019)

Replace mxnet_option macro with standard CMAKE_DEPENDENT_OPTION (#17018)

Switch to modern CMake CUDA handling (#17031)

Introduce unified MXNET_CUDA_ARCH option to specify cuda architectures.
Previously cuda architecture setting was partially broken and different options
were applied to different parts of the build (CUDA_ARCH_NAME CUDA_ARCH_BIN
CUDA_ARCH_PTX and CUDA_ARCH_LIST).

Include FindCUDAToolkit from CMake 3.17, which replaces the deprecated FindCUDA
functionality for finding the cuda toolkit include directories and libraries.

Workaround for DLL size limitation on Windows (#16980)

* change windows build system.
add gen_warp cpp version
add add_custom_command to run warp_gen
add download cmake
add option
change option
add dynamic read mxnet dll

Co-authored-by: Leonard Lausen <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants