Skip to content

Update CMakeLists.txt to link new header files for NVTX v3 for CUDA 1… #949

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DevinCharles
Copy link

@DevinCharles DevinCharles commented Jul 18, 2025

User description

…2.9+

Description

Adding a switch in CMakeLists.txt to locate and link the appropriate NVTX headers for CUDA 12.9+, while maintaining the previous compatibility with <= CUDA 12.8.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Scope

  • This PR comprises a set of related changes with a common goal

How Has This Been Tested?

Builds with both nvhpc 25.3 and 25.5 (CUDA 12.8 and 12.9) with --debug and --gpu flags enabled.
Solve 1x randomly chosen 2D example problem per build.

Test Configuration:

  • Personal Laptop (Older Dell P5540 w/ Quadro P2000, Rocky Linux 8.10 - 4.18.0-553.el8_10.x86_64)
  • nvhpc 25.3 and nvhpc 25.5

Checklist

  • I have added comments for the new code
  • I added Doxygen docstrings to the new code
  • I have made corresponding changes to the documentation (docs/)
  • I have added regression tests to the test suite so that people can verify in the future that the feature is behaving as expected
  • I have added example cases in examples/ that demonstrate my new feature performing as expected.
    They run to completion and demonstrate "interesting physics"
  • I ran ./mfc.sh format before committing my code
  • New and existing tests pass locally with my changes, including with GPU capability enabled (both NVIDIA hardware with NVHPC compilers and AMD hardware with CRAY compilers) and disabled
  • This PR does not introduce any repeated code (it follows the DRY principle)
  • [ x] I cannot think of a way to condense this code and reduce any introduced additional line count

PR Type

Bug fix


Description

  • Update CMakeLists.txt to support NVTX v3 for CUDA 12.9+

  • Add conditional linking for different CUDA versions

  • Maintain backward compatibility with CUDA <= 12.8

  • Include nvtx3 component and cudalib linker option


Diagram Walkthrough

flowchart LR
  A["CUDA Toolkit Detection"] --> B["Check CUDA Version"]
  B --> C["CUDA <= 12.8"]
  B --> D["CUDA >= 12.9"]
  C --> E["Link CUDA::nvToolsExt"]
  D --> F["Link CUDA::nvtx3"]
  E --> G["Add cudalib=nvtx option"]
  F --> G
Loading

File Walkthrough

Relevant files
Bug fix
CMakeLists.txt
Update NVTX linking for CUDA version compatibility             

CMakeLists.txt

  • Add conditional NVTX linking based on CUDA version
  • Request nvtx3 component in CUDAToolkit package
  • Add cudalib=nvtx linker option for all cases
  • Maintain compatibility with older CUDA versions
+14/-3   

@DevinCharles DevinCharles requested a review from a team as a code owner July 18, 2025 06:14
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Logic Error

The endif() statement on line 540 appears to close the wrong conditional block. It should close the NVHPC/PGI compiler check that starts on line 527, but there's an additional endif() on line 541 that suggests incorrect nesting of conditional blocks.

endif()

Missing Fallback

The code assumes that either CUDA::nvToolsExt or CUDA::nvtx3 will be available, but there's no error handling if neither target exists. This could lead to silent failures or build issues.

if (TARGET CUDA::nvToolsExt) # CUDA <= 12.8
    target_link_libraries(${a_target} PRIVATE CUDA::nvToolsExt)
else() # CUDA >= 12.9
    target_link_libraries(${a_target} PRIVATE CUDA::nvtx3)
endif()

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle missing nvtx3 component gracefully

The nvtx3 component may not be available in older CUDA versions, causing build
failures. Consider making the nvtx3 component optional and falling back to the
standard CUDAToolkit package if it's not found.

CMakeLists.txt [530-537]

-# Ask specifically for the header-only NVTX v3 interface
-find_package(CUDAToolkit REQUIRED COMPONENTS nvtx3)
+# Try to find NVTX v3 interface first, fallback to standard toolkit
+find_package(CUDAToolkit REQUIRED COMPONENTS nvtx3 QUIET)
+if (NOT CUDAToolkit_FOUND)
+    find_package(CUDAToolkit REQUIRED)
+endif()
 
 if (TARGET CUDA::nvToolsExt) # CUDA <= 12.8
     target_link_libraries(${a_target} PRIVATE CUDA::nvToolsExt)
 else() # CUDA >= 12.9
     target_link_libraries(${a_target} PRIVATE CUDA::nvtx3)
 endif()
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that requiring the nvtx3 component in find_package could cause build failures with older CUDA versions, and the proposed fallback mechanism improves the build script's robustness and backward compatibility.

Medium
  • More

Copy link

codecov bot commented Jul 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.06%. Comparing base (565178a) to head (09b0f54).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #949   +/-   ##
=======================================
  Coverage   44.06%   44.06%           
=======================================
  Files          68       68           
  Lines       18220    18220           
  Branches     2292     2292           
=======================================
  Hits         8029     8029           
  Misses       8821     8821           
  Partials     1370     1370           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sbryngelson
Copy link
Member

works on runners but i'm seeing some weird behavior with nvhpc/25.5 on Phoenix (which is not used on the runners right now, 24.5 is). will try some things out myself before merging.

@DevinCharles
Copy link
Author

Let me know if there's anything I can check on my side. I'll add 24.5 to the list to check. Do the runners run all tests? Is there a specific test I can check our can you add the errors here if you think they're relevant to other systems?

@sbryngelson
Copy link
Member

sbryngelson commented Jul 19, 2025

Well 25.5 fails for me on Phoenix w/ your PR but that computer is currently running v24.5 when the runners go - so the current PR has v24.5 working fine. I can change this PR so it loads the latest modules (I should probably just do that) and then we will see what breaks/doesn't break. When I tried doing it myself on Phoenix it just broke in a strange way so I'll try again later.

@sbryngelson
Copy link
Member

Hmm I cannot get this working with nvhpc v25.5 on my end (using your/our changes). Can you check out the latest master branch and see if you can compile? If you don't mind, can you give me details of your compiler stack?

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

Successfully merging this pull request may close these issues.

2 participants