Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kokkos_launch_compiler + CUDA auto-detect arch #3770

Merged
merged 4 commits into from
Feb 3, 2021

Conversation

jrmadsen
Copy link
Contributor

@jrmadsen jrmadsen commented Feb 1, 2021

- enables CUDA as a language to auto-detect architecture
- compiler launcher is used so it should validate auto-detection worked
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Is there any problem with always doing this and removing the first run?

@jrmadsen
Copy link
Contributor Author

jrmadsen commented Feb 1, 2021

Is there any problem with always doing this and removing the first run?

I honestly cannot think of any other than it might cause users unfamiliar with the Kokkos build system to see CUDA getting enabled as a language and lead them to believe we are actually compiling things with CMAKE_CUDA_COMPILER

@jrmadsen
Copy link
Contributor Author

jrmadsen commented Feb 1, 2021

As far as I know there is no way to suppress this from getting emitted when the enable_language(CUDA) is done:

-- The CUDA compiler identification is NVIDIA 11.0.221
-- Detecting CUDA compiler ABI info
-- Detecting CUDA compiler ABI info - done
-- Check for working CUDA compiler: /usr/local/cuda/bin/nvcc - skipped
-- Detecting CUDA compile features
-- Detecting CUDA compile features - done

@jrmadsen
Copy link
Contributor Author

jrmadsen commented Feb 1, 2021

Even though I can't think of why it may cause the auto-detect to emit something different, at least with the current implementation, if the users sets CMAKE_CXX_COMPILER to nvcc_wrapper or mpicxx + MPICH_CXX=nvcc_wrapper (or whatever the env variable is), we know the compiler being used to detect the auto-arch will be the compiler used when compiling the code. Supercomputers have a lot of toolchains and I don't feel like I can say with 100% certainty that if cmake picks up nvcc from cuda 9.2, it will correctly detect the Ampere arch. To me, keeping the way it currently is will produce the least amount of surprises and removing it will only save us ~8 lines of cmake code.

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Only some minor comments. Otherwise, I'm fine with the changes here.

cmake/kokkos_arch.cmake Outdated Show resolved Hide resolved
cmake/kokkos_arch.cmake Outdated Show resolved Hide resolved
cmake/kokkos_arch.cmake Outdated Show resolved Hide resolved
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks OK to me.

@masterleinad
Copy link
Contributor

Retest this please.

Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Pasting below snippet from configuration log in CI build for reference

-- CUDA auto-detection of architecture failed with /usr/bin/g++-8. Enabling CUDA language ONLY to auto-detect architecture...
-- Looking for a CUDA compiler
-- Looking for a CUDA compiler - /usr/local/cuda/bin/nvcc
-- The CUDA compiler identification is NVIDIA 11.0.221
-- Check for working CUDA compiler: /usr/local/cuda/bin/nvcc
-- Check for working CUDA compiler: /usr/local/cuda/bin/nvcc - works
-- Detecting CUDA compiler ABI info
-- Detecting CUDA compiler ABI info - done
-- Detecting CUDA compile features
-- Detecting CUDA compile features - done
-- Detected CUDA Compute Capability 70
-- Setting Kokkos_ARCH_VOLTA70=ON

You must fix the indentation though. The rest of the cmake/kokkos_arch.cmake uses two spaces and your patch uses a mix of two and four spaces.

@jrmadsen
Copy link
Contributor Author

jrmadsen commented Feb 2, 2021

You must fix the indentation though. The rest of the cmake/kokkos_arch.cmake uses two spaces and your patch uses a mix of two and four spaces.

Nah, my IDE aligns to the original indentation of two spaces and then uses 4 spaces from then on out because 4 spaces is better than 2 😉

@jrmadsen jrmadsen requested a review from dalg24 February 2, 2021 17:35
@masterleinad
Copy link
Contributor

Retest this please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants