-
Notifications
You must be signed in to change notification settings - Fork 114
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
Update Thrust to CCCL #206
base: develop
Are you sure you want to change the base?
Conversation
thrust_create_target(Thrust${BACKEND} | ||
HOST CPP | ||
DEVICE ${BACKEND}) |
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.
These lines switch between the different Thrust backends, and this feature is lost in the new version. By default, CCCL uses the CUDA backend for Thrust, but it looks like BabelStream may want to use TBB or OMP.
To keep this functionality, force-set the cache variable CCCL_THRUST_DEVICE_SYSTEM
to the desired backend before any find_package
/CPM
calls.
Force setting that variable tells the CCCL CMake package to use the desired device backend for the CCCL::Thrust
target that gets linked into CCCL::CCCL
. See this for details. This is a convenience for the common case where only one backend is used per configure. If multiple Thrust targets with different backends are needed in the same build, lmk, there's a different approach that can be used.
Also, using paths based off of CMAKE_SOURCE_DIR
is fragile. If BabelStream is included in another project (via add_subdirectory, CPM, etc), this variable will point to the top-level project's source dir, not BabelStream's. Use the project specific variable BabelStream_SOURCE_DIR
instead.
The include
call should wrap it's argument in quotes. This is best practice for all paths in cmake to support spaces, etc.
Finally, rather than manually checking find_package
and falling back to CPMAddPackage
, it looks like CPMFindPackage
will do this for us:
The function CPMFindPackage will try to find a local dependency via CMake's find_package and fallback to CPMAddPackage, if the dependency is not found.
All together, you're looking at something like:
set(MIN_CCCL_VERSION 2.4.0)
# Tell CCCL's package to configure Thrust using the desired backend:
set(CCCL_THRUST_DEVICE_SYSTEM ${BACKEND} CACHE STRING "" FORCE)
# CPMFindPackage will:
# 1. Attempt to locate a local installation of CCCL.
# Set CCCL_DIR to the directory containing `cccl-config.cmake` to force a
# specific installation to be used.
# 2. If no local package is found, the requested version will be downloaded
# from GitHub and configured using CPMAddPackage.
include("${BabelStream_SOURCE_DIR}/src/thrust/CPM.cmake")
CPMFindPackage(
NAME CCCL
GITHUB_REPOSITORY nvidia/cccl
GIT_TAG v${MIN_CCCL_VERSION}
)
register_link_library(CCCL::CCCL)
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.
Thank you for the review! I set CCCL_THRUST_DEVICE_SYSTEM
now before starting to find and fetch CCCL.
As for handling CPM, I decided now to revert to FetchContent to just make the CMake setup simpler.
Let me know if anything is still missing, thx!
6590a17
to
5e86faa
Compare
Recent changes in Thrust, for the CUDA backend, no longer allow including `<thrust/device_vector.h>` into a translation unit that is not compiled by a CUDA compiler. As a workaround, the vectors containing the data are moved from the header into the benchmark implementation file.
05afc2e
to
eafbf41
Compare
eafbf41
to
be3132b
Compare
@gonzalobg I would appreciate your input on this PR. Do these changes break any of your workflows? |
Ping. @tomdeakin, this PR is really important to enable users to use CCCL instead of the legacy Thrust library. |
We are planning a re-think of CUDA and Thrust models generally with @gonzalobg. To manage expectations on timescales, we hope to release v6 in time for Supercomputing, but it depends on how much progress we make on other changes for v6. |
Thank you for the answer! I knew @gonzalobg is somehow involved, but I didn't know the plan. In any case, I hope v6 will support Thrust from CCCL, however it is done. |
The Thrust library no longer exists as a standalone project since it has been absorbed into the CCCL. I therefore propose to update the Thrust-based version of BabelStream to account for this. Specifically, this PR:
CCCL
via cmake'sfind_package
. Newer CUDA toolkits ship CCCL, and you can point cmake to pick it up with the already availableSDK_DIR
. If no version of CCCL is found, cmake will try to find the legacy Thrust and CUB targets.device_vector
into code that is not compiled by nvcc. I therefore pulled the corresponding data members into the source file using a the PIMPL pattern.make_zip_iterator
, which simplifies the benchmark implementation. With this change, CUDA < 11.3 will no longer be able to compile the benchmark.I also want to emphasize that using a new version of CCCL is crucial, since CCCL constantly improves performance of Thrust algorithms. However, for comparability of benchmarks, it makes sense to pick the CCCL version of the CUDA SDK by default, and not introduce another variable.
I tried configuring and building
thrust-stream
on my Ubuntu 24.04 (default nvcc 12.5, default g++ 13.2) from a<git_clone>/build
directory:So fetching CCCL seems to work, using CCCL from a CUDA toolkit works, using CCCL from a HPC SDK works, and using legacy Thrust and CUB from an old CUDA SDK works.
I did not test whether rocThrust works.