-
-
Notifications
You must be signed in to change notification settings - Fork 3
171 add cuda 13 to lts branch #172
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
Conversation
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.
Pull request overview
This PR adds support for CUDA 13.1 to the LTS branch, including workarounds for compiler bugs affecting NVCC versions 12.4 through 13.1. The changes enable the project to build and test with CUDA 13.1 while maintaining backward compatibility with earlier CUDA versions.
Key Changes:
- Added conditional compilation guards to work around NVCC 12.4+ compiler bugs that prevent certain constexpr code from compiling
- Updated CMake configuration to handle CUDA 13-specific library versions and runtime library modes
- Extended CI/CD workflows to include CUDA 13.1 in the test matrix
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/data/test_tuple.h | Added conditional compilation to skip problematic constexpr tests on NVCC 12.4-13.1 |
| tests/cudabug/test_nvcc131_BugReproducer.h | New file documenting and reproducing the NVCC bug affecting versions 12.4-13.1 |
| cmake/tests/add_shared_test_libs.cmake | Added /Zc:preprocessor flag for MSVC to enforce standards-compliant preprocessor |
| cmake/tests/add_generated_test.cmake | Added /Zc:preprocessor flag for MSVC test targets |
| cmake/libs/cuda/target_generation.cmake | Added CUDA 13 handling with Hybrid runtime library mode and /Zc:preprocessor flag |
| cmake/libs/cuda/deploy.cmake | Added CUDA 13 library version mappings (cufft64_12, curand64_10) and x64 subdirectory path |
| benchmarks/CMakeLists.txt | Added /Zc:preprocessor flag for MSVC benchmark targets |
| .github/workflows/cmake-windows-amd64.yml | Added CUDA 13.1 with MSVC 14.44 to Windows test matrix |
| .github/workflows/cmake-linux-arm64.yml | Added conditional CUDACXX export to handle both nvcc and other CUDA compilers |
| .github/workflows/cmake-linux-amd64.yml | Added CUDA 13.1 configurations and conditional CUDACXX export for nvcc/clang++ |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export PATH=/usr/lib/llvm-21/bin/:$PATH | ||
| export CUDACXX=/usr/local/cuda-${cpp_compiler[2]}/bin/nvcc | ||
| cmake -G "Ninja" -B ${{steps.strings.outputs.build-output-dir}} -DCMAKE_CXX_COMPILER="${cpp_compiler[0]}" -DCMAKE_CUDA_COMPILER="${cpp_compiler[1]}" -DCUDAToolkit_ROOT="/usr/local/cuda-${cpp_compiler[2]}" -DCMAKE_BUILD_TYPE="Release" -S ${{github.workspace}} | ||
| export PATH=/home/cudeiro/cmake-4.2.1-linux-x86_64/bin:/usr/lib/llvm-21/bin/:$PATH |
Copilot
AI
Dec 11, 2025
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 line appears to have a hardcoded absolute path specific to a particular machine (/home/cudeiro/cmake-4.2.1-linux-x86_64/bin). This will cause failures on other machines or CI runners. Either remove this path addition if cmake is available in the system PATH, or make it configurable via an environment variable or workflow input.
| export PATH=/home/cudeiro/cmake-4.2.1-linux-x86_64/bin:/usr/lib/llvm-21/bin/:$PATH | |
| export PATH=/usr/lib/llvm-21/bin/:$PATH |
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.
@albertandaluz why do we have those hardcoded paths?
added ci configurations for cuda 13.1 on windows and linux (x64) pass :/Zc:preprocessor to supress warnings about legacy preprocessor on msvc add compiler preprocvessor guards to skip compilation of test_tuple.h with cuda 12.4 added a reproducer for a nvcc bug found with cuda 12.4 # Conflicts: # .github/workflows/cmake-linux-amd64.yml # .github/workflows/cmake-linux-arm64.yml # .github/workflows/cmake-windows-amd64.yml # cmake/libs/cuda/target_generation.cmake
befc311 to
406553b
Compare
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.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else | ||
| export CUDACXX="${cpp_compiler[1]}" | ||
| fi | ||
| cmake -G "Ninja" -B ${{steps.strings.outputs.build-output-dir}} -DCMAKE_CXX_COMPILER="${cpp_compiler[0]}" -DCMAKE_CUDA_COMPILER=${CUDACXX} -DCUDAToolkit_ROOT="/usr/local/cuda-${cpp_compiler[2]}" -DCMAKE_BUILD_TYPE="Release" -S ${{github.workspace}} |
Copilot
AI
Dec 27, 2025
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.
The CUDACXX variable should be quoted in the cmake command for safety. Change -DCMAKE_CUDA_COMPILER=${CUDACXX} to -DCMAKE_CUDA_COMPILER="${CUDACXX}" to prevent issues if the path contains spaces or special characters.
| cmake -G "Ninja" -B ${{steps.strings.outputs.build-output-dir}} -DCMAKE_CXX_COMPILER="${cpp_compiler[0]}" -DCMAKE_CUDA_COMPILER=${CUDACXX} -DCUDAToolkit_ROOT="/usr/local/cuda-${cpp_compiler[2]}" -DCMAKE_BUILD_TYPE="Release" -S ${{github.workspace}} | |
| cmake -G "Ninja" -B ${{steps.strings.outputs.build-output-dir}} -DCMAKE_CXX_COMPILER="${cpp_compiler[0]}" -DCMAKE_CUDA_COMPILER="${CUDACXX}" -DCUDAToolkit_ROOT="/usr/local/cuda-${cpp_compiler[2]}" -DCMAKE_BUILD_TYPE="Release" -S ${{github.workspace}} |
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.
@albertandaluz the suggestion seem's reasonable. Can you check it?
|
@albertandaluz I've opened a new pull request, #176, to work on those changes. Once the pull request is ready, I'll request review from you. |
albertandaluz
left a comment
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.
most suggestion are correct regarding identation. not sure abouth hardcoded paths since we only use a single self host runner for builds
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
No description provided.