Use CMake standard library to handle CUDA #17031

leezu · 2019-12-10T02:23:02Z

Description

Use CMake standard library to handle CUDA.
Makes our code more maintainable and may fix bugs, because we rely on code tested by all CMake users, instead of the small subset of MXNet CMake users.

Include FindCUDAToolkit that replaces the deprecated FindCUDA module as of CMake 1.17.
https://gitlab.kitware.com/cmake/cmake/merge_requests/4093

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

Refactor cuda handling in cmake build

Comments

Bump CMake minimum version requirement due to
https://gitlab.kitware.com/cmake/cmake/issues/19199
Lazy consensus for the minimum version bump: https://lists.apache.org/thread.html/82ead692d172918de3afdeb27dbd533d37e1c084aa5738c5e353bbfd%40%3Cdev.mxnet.apache.org%3E

CMakeLists.txt

yajiedesign · 2019-12-10T07:25:05Z

Consider integration #16980 and update cmake to 3.15.5.

leezu · 2019-12-10T07:26:58Z

I'll integrate the changes of #16980 once it's merged. Or would you like me to do it directly?
I'm currently not sure why edge build is failing. So #16980 may be merged first.

yajiedesign · 2019-12-10T07:32:27Z

Currently #16980 is waiting for ci to upgrade cmake. I don't know when the upgrade is complete.

yajiedesign · 2019-12-10T07:35:52Z

i see apache/mxnet-ci#17 is merged.When can upgrade complete? I'm not sure about this process.

leezu · 2019-12-10T07:40:46Z

@yajiedesign I think the change will be picked up within 7 days. But @marcoabreu may provide more details (or can force the change to be picked up earlier).

The current PR is also waiting for apache/mxnet-ci#17 to be picked up.

3rdparty/mshadow/CMakeLists.txt

larroy · 2019-12-11T19:57:54Z

i see apache/incubator-mxnet-ci#17 is merged.When can upgrade complete? I'm not sure about this process.

The change should be picked up as soon as you merge this from master in your branch.

leezu · 2019-12-12T02:17:31Z

i see apache/incubator-mxnet-ci#17 is merged.When can upgrade complete? I'm not sure about this process.

The change should be picked up as soon as you merge this from master in your branch.

What do you mean? It's for the Windows images used by the CI. When will the CI use the updated images?

leezu · 2019-12-12T06:58:02Z

@marcoabreu can you share the date when mxnet-CI will use the updated Windows AMI?

larroy · 2019-12-12T07:42:11Z

i see apache/incubator-mxnet-ci#17 is merged.When can upgrade complete? I'm not sure about this process.

The change should be picked up as soon as you merge this from master in your branch.

What do you mean? It's for the Windows images used by the CI. When will the CI use the updated images?

Oh yes I got confused sorry. We would have to update the AMIs used by CI. This needs to be done by an Amazon engineer. Marco is not involved.

leezu · 2019-12-12T07:49:15Z

Ok, thank you. I saw Marco mentioned this is done automatically every 7 days (https://github.com/apache/incubator-mxnet-ci/blob/master/services/jenkins-slave-creation-windows/README.md). Is it wrong?
@larroy could you help to get the update done? Thank you

larroy · 2019-12-12T19:13:12Z

I passed on your request to the engineering team.

larroy · 2019-12-18T00:20:26Z

Why are the dockcross images related to CMake changes??

leezu · 2019-12-18T03:03:02Z

Because the edge build is currently completely broken and only keeps on working in some special settings due to caching effects. Updating cmake on the current dockcross images is one case that will break the caching effects and thus disable the build. Updating the dockcross images fixes the edge build, and further the updated version ships with cmake 13.

It's not necessary to merge the dockcross change as part of this PR. But until the edge build is not fixed in master, the dockcross update is a necessary part of this PR.

leezu · 2019-12-23T09:24:01Z

Rebased after #17098 got merged.

leezu · 2019-12-26T11:37:43Z

Ping for review

@szha @yajiedesign or anyone willing

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.

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

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

…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

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]>

ChaiBapchya · 2020-01-13T02:59:07Z

cmake/Modules/FindCUDAToolkit.cmake

+endif()
+
+
+get_filename_component(CUDAToolkit_ROOT_DIR ${CUDAToolkit_BIN_DIR} DIRECTORY ABSOLUTE)


I understand this has been included from https://gitlab.kitware.com/cmake/cmake/merge_requests/4093
However, I'm confused between CUDAToolkit_ROOT_DIR and CUDA_TOOLKIT_ROOT_DIR that's used at rest of the places in cmake in our codebase.
Are they both pointing to same thing? @leezu Thanks

Yes, it should be insensitive to capitalization

leezu requested a review from yajiedesign December 10, 2019 02:23

leezu requested a review from szha as a code owner December 10, 2019 02:23

leezu force-pushed the cmakerefactorcuda branch 2 times, most recently from 5e35717 to 24e7209 Compare December 10, 2019 04:51

yajiedesign reviewed Dec 10, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

leezu force-pushed the cmakerefactorcuda branch from 24e7209 to 251f8d8 Compare December 11, 2019 00:36

leezu requested review from aaronmarkham and marcoabreu as code owners December 11, 2019 00:36

leezu mentioned this pull request Dec 11, 2019

Relocation truncation issues #17045

Open

larroy reviewed Dec 11, 2019

View reviewed changes

3rdparty/mshadow/CMakeLists.txt Outdated Show resolved Hide resolved

leezu force-pushed the cmakerefactorcuda branch 4 times, most recently from f1c0dbe to 37c9680 Compare December 12, 2019 06:47

leezu force-pushed the cmakerefactorcuda branch from 37c9680 to cfc2a2f Compare December 12, 2019 07:46

leezu force-pushed the cmakerefactorcuda branch 2 times, most recently from 19088f7 to 08017fe Compare December 12, 2019 15:19

leezu force-pushed the cmakerefactorcuda branch 2 times, most recently from a94bbf8 to e8163e6 Compare December 13, 2019 14:08

leezu force-pushed the cmakerefactorcuda branch 4 times, most recently from 899277f to 80d3e7a Compare December 17, 2019 14:30

leezu mentioned this pull request Dec 18, 2019

change windows build system #16980

Merged

leezu requested a review from yajiedesign December 18, 2019 03:02

leezu added the pr-awaiting-review PR is waiting for code review label Dec 18, 2019

leezu mentioned this pull request Dec 21, 2019

Memory should be completely released after an OOM happens #17126

Open

leezu force-pushed the cmakerefactorcuda branch from 80d3e7a to 6763f48 Compare December 23, 2019 09:21

leezu mentioned this pull request Dec 24, 2019

Cache CUDA modules when building Mxnet #17158

Closed

leezu force-pushed the cmakerefactorcuda branch from 6763f48 to 85f7ece Compare December 26, 2019 09:07

leezu force-pushed the cmakerefactorcuda branch from 85f7ece to d949e0c Compare December 27, 2019 12:22

szha merged commit 230ceee into apache:master Dec 30, 2019

leezu deleted the cmakerefactorcuda branch December 30, 2019 09:54

TsingWei mentioned this pull request Dec 31, 2019

When build mxnet from source via cmake, if I set USE_SSE to 0, target mshadow is still built with SSE option on. #16809

Closed

yajiedesign mentioned this pull request Jan 4, 2020

Backport #17031, #16980 to 1.6 branch #17213

Merged

ChaiBapchya reviewed Jan 13, 2020

View reviewed changes

larroy mentioned this pull request Jan 16, 2020

[Build] Add a reasonable default for CMAKE_CUDA_COMPILER in *nix #17293

Merged

olk mentioned this pull request Feb 4, 2020

C++-Package examples broken #17514

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CMake standard library to handle CUDA #17031

Use CMake standard library to handle CUDA #17031

leezu commented Dec 10, 2019 •

edited

Loading

yajiedesign commented Dec 10, 2019

leezu commented Dec 10, 2019

yajiedesign commented Dec 10, 2019

yajiedesign commented Dec 10, 2019

leezu commented Dec 10, 2019

larroy commented Dec 11, 2019 •

edited

Loading

leezu commented Dec 12, 2019

leezu commented Dec 12, 2019

larroy commented Dec 12, 2019

leezu commented Dec 12, 2019

larroy commented Dec 12, 2019

larroy commented Dec 18, 2019

leezu commented Dec 18, 2019 •

edited

Loading

leezu commented Dec 23, 2019

leezu commented Dec 26, 2019

ChaiBapchya Jan 13, 2020

leezu Jan 13, 2020

		endif()


		get_filename_component(CUDAToolkit_ROOT_DIR ${CUDAToolkit_BIN_DIR} DIRECTORY ABSOLUTE)

Use CMake standard library to handle CUDA #17031

Use CMake standard library to handle CUDA #17031

Conversation

leezu commented Dec 10, 2019 • edited Loading

Description

Checklist

Essentials

Changes

Comments

yajiedesign commented Dec 10, 2019

leezu commented Dec 10, 2019

yajiedesign commented Dec 10, 2019

yajiedesign commented Dec 10, 2019

leezu commented Dec 10, 2019

larroy commented Dec 11, 2019 • edited Loading

leezu commented Dec 12, 2019

leezu commented Dec 12, 2019

larroy commented Dec 12, 2019

leezu commented Dec 12, 2019

larroy commented Dec 12, 2019

larroy commented Dec 18, 2019

leezu commented Dec 18, 2019 • edited Loading

leezu commented Dec 23, 2019

leezu commented Dec 26, 2019

ChaiBapchya Jan 13, 2020

Choose a reason for hiding this comment

leezu Jan 13, 2020

Choose a reason for hiding this comment

leezu commented Dec 10, 2019 •

edited

Loading

larroy commented Dec 11, 2019 •

edited

Loading

leezu commented Dec 18, 2019 •

edited

Loading