-
-
Notifications
You must be signed in to change notification settings - Fork 3
168 add cuda 13 to main branch #169
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
base: main
Are you sure you want to change the base?
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
Key changes attempted:
- Addition of CUDA 13.x to CI matrices for Windows and Linux AMD64 workflows
- Enforcement of standards-compliant preprocessor flag (
/Zc:preprocessor) for MSVC across test and benchmark targets - Conditional logic for CUDA 13+ to use "Hybrid" instead of "Shared" runtime library
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/cmake-windows-amd64.yml |
Adds CUDA 13.x matrix entries for cl and llvm compilers (version does not exist) |
.github/workflows/cmake-linux-amd64.yml |
Adds CUDA 13.x matrix entries for g++ and clang++ compilers (version does not exist) |
cmake/libs/cuda/target_generation.cmake |
Adds conditional logic for CUDA 13+ to use Hybrid runtime library and passes /Zc:preprocessor to CUDA compiler |
cmake/libs/cuda/deploy.cmake |
Adds CUDA 13 specific library version mappings and x64 subdirectory path handling |
cmake/tests/add_shared_test_libs.cmake |
Adds /Zc:preprocessor flag for MSVC to test libraries |
cmake/tests/add_generated_test.cmake |
Adds /Zc:preprocessor flag for MSVC to generated tests |
benchmarks/CMakeLists.txt |
Adds /Zc:preprocessor flag for MSVC to benchmarks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elseif(CUDA_VERSION_MAJOR EQUAL 13) | ||
| # cufft is at version 11 for CUDA 12 | ||
| list(TRANSFORM TARGET_DEPENDENCIES_COMMON REPLACE "cufft64_${CUDA_VERSION_MAJOR}" "cufft64_12") | ||
| list(TRANSFORM TARGET_DEPENDENCIES_COMMON REPLACE "curand64_${CUDA_VERSION_MAJOR}" "curand64_10") | ||
| endif() | ||
| list(TRANSFORM TARGET_DEPENDENCIES_COMMON APPEND ".dll") | ||
| list(TRANSFORM TARGET_DEPENDENCIES_COMMON PREPEND "${CUDAToolkit_BIN_DIR}/") | ||
| if(CUDA_VERSION_MAJOR EQUAL 13) | ||
| #todo: winarm64 | ||
| list(TRANSFORM TARGET_DEPENDENCIES_COMMON PREPEND "${CUDAToolkit_BIN_DIR}/x64/") | ||
| else() | ||
| list(TRANSFORM TARGET_DEPENDENCIES_COMMON PREPEND "${CUDAToolkit_BIN_DIR}/") | ||
| endif() |
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.
Inconsistent indentation: These lines use tab characters while the rest of the file uses spaces. Please use consistent spacing (4 spaces based on surrounding code) to maintain code style consistency.
|
@albertandaluz I've opened a new pull request, #170, to work on those changes. Once the pull request is ready, I'll request review from you. |
7051a99 to
5866303
Compare
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
06c7ca5 to
67dc450
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 13 out of 13 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #define NVCC_VERSION_CALCULATED (__CUDACC_VER_MAJOR__ * 10000 + __CUDACC_VER_MINOR__ * 100 + __CUDACC_VER_BUILD__) | ||
| #define NVCC_VERSION_12_4_99 120499 | ||
|
|
||
| // Condition 1: we are compiling with MSVC + nvcc OR other compilers + nvcc versions lower than 12.4.93 |
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 comment mentions 'nvcc versions lower than 12.4.93', but the actual comparison on line 32 checks against NVCC_VERSION_12_4_99 (which represents version 12.4.99). This creates a mismatch between the comment and the code logic. Consider updating the comment to accurately reflect the version being checked (12.4.99) or adjusting the version constant if 12.4.93 was the intended threshold.
| // Condition 1: we are compiling with MSVC + nvcc OR other compilers + nvcc versions lower than 12.4.93 | |
| // Condition 1: we are compiling with MSVC + nvcc OR other compilers + nvcc versions lower than 12.4.99 |
| std::cout << "test_tuple Failed!!" << std::endl; | ||
| return -1; | ||
| } | ||
| #else |
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.
Inconsistent indentation: the '#else' directive has a leading space while the surrounding '#ifdef' and '#endif' directives don't. All preprocessor directives should have consistent indentation.
| #else | |
| #else |
|
|
||
| // Using and_v2 here and keeping and_v in line 67 | ||
| // or the other way arround, compiles | ||
| // Changin result1 to the literal true, compiles |
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.
Typo in comment: 'Changin' should be 'Changing'.
| // Changin result1 to the literal true, compiles | |
| // Changing result1 to the literal true, compiles |
| #include <fused_kernel/core/execution_model/operation_model/operation_tuple.h> | ||
| #include <fused_kernel/algorithms/basic_ops/vector_ops.h> | ||
| #include <fused_kernel/core/execution_model/memory_operations.h> | ||
| #ifdef __NVCC__ |
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 macros NVCC_VERSION_CALCULATED and NVCC_VERSION_12_4_99 are used on line 24 but are never defined in this file. These macros need to be defined before use, similar to how they are defined in tests/cudabug/test_nvcc131_BugReproducer.h (lines 28-29).
| #ifdef __NVCC__ | |
| #ifdef __NVCC__ | |
| // Helper macros to compute and compare the NVCC version. | |
| // NVCC_VERSION_CALCULATED follows the pattern MAJOR * 10000 + MINOR * 100 + BUILD. | |
| #ifndef NVCC_VERSION_CALCULATED | |
| #if defined(__CUDACC_VER_MAJOR__) && defined(__CUDACC_VER_MINOR__) && defined(__CUDACC_VER_BUILD__) | |
| #define NVCC_VERSION_CALCULATED (__CUDACC_VER_MAJOR__ * 10000 + __CUDACC_VER_MINOR__ * 100 + __CUDACC_VER_BUILD__) | |
| #else | |
| // Fallback: treat unknown NVCC version as 0 so that the comparison remains well-formed. | |
| #define NVCC_VERSION_CALCULATED 0 | |
| #endif | |
| #endif | |
| #ifndef NVCC_VERSION_12_4_99 | |
| #define NVCC_VERSION_12_4_99 120499 | |
| #endif |
| constexpr bool result1 = myint.instance == 1; | ||
|
|
||
| // Using and_v2 here and keeping and_v in line 67 | ||
| // or the other way arround, compiles |
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.
Typo in comment: 'arround' should be 'around'.
| // or the other way arround, compiles | |
| // or the other way around, compiles |
| #ifndef CLANG_HOST_DEVICE | ||
| static_assert(false); | ||
| #endif | ||
|
|
||
| #if defined(__NVCC__) || (CLANG_HOST_DEVICE == 1) |
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.
This static_assert will always fail, causing compilation to fail unconditionally. The check should verify whether CLANG_HOST_DEVICE is defined, not trigger a failure when it is defined. Consider using '#ifndef' instead to ensure the macro is properly defined, or remove this assertion if it's intended for debugging.
| #ifndef CLANG_HOST_DEVICE | |
| static_assert(false); | |
| #endif | |
| #if defined(__NVCC__) || (CLANG_HOST_DEVICE == 1) | |
| #if defined(__NVCC__) || (defined(CLANG_HOST_DEVICE) && (CLANG_HOST_DEVICE == 1)) |
| elseif(CUDA_VERSION_MAJOR EQUAL 13) | ||
| # cufft is at version 12 for CUDA 13 | ||
| list(TRANSFORM TARGET_DEPENDENCIES_COMMON REPLACE "cufft64_${CUDA_VERSION_MAJOR}" "cufft64_12") | ||
| list(TRANSFORM TARGET_DEPENDENCIES_COMMON REPLACE "curand64_${CUDA_VERSION_MAJOR}" "curand64_10") | ||
| endif() | ||
| list(TRANSFORM TARGET_DEPENDENCIES_COMMON APPEND ".dll") | ||
| list(TRANSFORM TARGET_DEPENDENCIES_COMMON PREPEND "${CUDAToolkit_BIN_DIR}/") | ||
| if(CUDA_VERSION_MAJOR EQUAL 13) |
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.
Inconsistent spacing: this line uses tabs after 'if' statements. The file appears to mix tabs and spaces. Consider using consistent spacing (spaces) throughout to match CMake conventions.
| #include <fused_kernel/algorithms/basic_ops/vector_ops.h> | ||
| #include <fused_kernel/core/execution_model/memory_operations.h> | ||
| #ifdef __NVCC__ | ||
| // Condition 1: we are compiling with MSVC + nvcc OR other compilers + nvcc versions lower than 12.4.93 |
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 comment on line 23 mentions 'nvcc versions lower than 12.4.93', but the actual comparison on line 24 checks against NVCC_VERSION_12_4_99 (which represents version 12.4.99). This creates a mismatch between the comment and the code logic. Consider updating the comment to accurately reflect the version being checked (12.4.99) or adjusting the version constant if 12.4.93 was the intended threshold.
| // Condition 1: we are compiling with MSVC + nvcc OR other compilers + nvcc versions lower than 12.4.93 | |
| // Condition 1: we are compiling with MSVC + nvcc OR other compilers + nvcc versions lower than 12.4.99 |
morousg
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.
Review the copilot comments
added cuda 13.x compiler to CI
enforce standards compliant preprocessor in msvc
skipping 13.x for jetson