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

Test combined internal/user-side use of NVTX #1690

Merged
merged 1 commit into from
May 3, 2024

Conversation

bernhardmgruber
Copy link
Contributor

This PR adds a test where the NVTX API is used internally by CUB via the vendored nvtx3.hpp, and a second time in the test's main file with headers cloned from the NVTX GitHub repository. The test verifies that CUB's use of NVTX does not break users using NVTX beside CUB.

Copy link

copy-pr-bot bot commented May 2, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Comment on lines 29 to 30
CPMAddPackage(NAME NVTX GITHUB_REPOSITORY NVIDIA/NVTX GIT_TAG release-v3 DOWNLOAD_ONLY True)
add_subdirectory(${NVTX_SOURCE_DIR}/c ${NVTX_BINARY_DIR}/c)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the best way to obtain a dependency without a top-level CMakeLists.txt?

Comment on lines 282 to 284
if (${test_src} MATCHES "test_nvtx_in_usercode.cu")
target_link_libraries(${test_target} nvtx3-cpp)
endif()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I best add a library dependency to only a single test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do something like this:

if ("${test_target}" MATCHES "nvrtc")
target_compile_definitions(${test_target} PRIVATE NVRTC_CUB_PATH="-I${CMAKE_SOURCE_DIR}/cub")
target_compile_definitions(${test_target} PRIVATE NVRTC_THRUST_PATH="-I${CMAKE_SOURCE_DIR}/thrust")
target_compile_definitions(${test_target} PRIVATE NVRTC_LIBCUDACXX_PATH="-I${CMAKE_SOURCE_DIR}/libcudacxx/include")
target_compile_definitions(${test_target} PRIVATE NVRTC_CTK_PATH="-I${CUDAToolkit_INCLUDE_DIRS}")
endif()

@alliepiper might have a better idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I updated my changeset.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is indeed the best way for now. Thrust has a fancier mechanism that lets us add a test-specific ${test_name}.cmake file that gets loaded and applied to the test targets, but that doesn't currently exist in CUB.

@bernhardmgruber bernhardmgruber force-pushed the nvtx_userside_use branch 2 times, most recently from 031833d to a2a7a6f Compare May 2, 2024 17:35
cub/test/CMakeLists.txt Outdated Show resolved Hide resolved
@bernhardmgruber bernhardmgruber force-pushed the nvtx_userside_use branch 2 times, most recently from 8d44a76 to 8bacb3f Compare May 2, 2024 18:39
@bernhardmgruber bernhardmgruber marked this pull request as ready for review May 3, 2024 07:12
@bernhardmgruber bernhardmgruber requested review from a team as code owners May 3, 2024 07:12
@bernhardmgruber bernhardmgruber requested review from wmaxey and elstehle May 3, 2024 07:12
@bernhardmgruber bernhardmgruber merged commit c910b16 into NVIDIA:main May 3, 2024
404 checks passed
@bernhardmgruber bernhardmgruber deleted the nvtx_userside_use branch May 3, 2024 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants