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

Thrust Usage Update, main branch (2024.12.02.) #790

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

krasznaa
Copy link
Member

@krasznaa krasznaa commented Dec 2, 2024

Changed how Thrust would be used in the build.

Instead of setting up a single traccc::Thrust target that would take care of everything related to Thrust, the code rather sets up traccc::Thrust in a way that allows very little access in traccc::core to Thrust.

Instead, traccc::cuda and traccc::sycl (and also traccc_test_cuda) privately link against Thrust in specific ways. So that the appropriate "device headers" would get included everywhere.

Note that this shall not only help the SYCL developments (the commit comes directly from #773...), but the HIP ones as well. (Pinging @flg and @StewMH.)

Instead of setting up a single traccc::Thrust target that would
take care of everything related to Thrust, the code rather
sets up traccc::Thrust in a way that allows very little access
in traccc::core to Thrust.

Instead, traccc::cuda and traccc::sycl (and also traccc_test_cuda)
privately link against Thrust in specific ways. So that the
appropriate "device headers" would get included everywhere.
@krasznaa krasznaa added build This relates to the build system cuda Changes related to CUDA sycl Changes related to SYCL labels Dec 2, 2024
Copy link

sonarqubecloud bot commented Dec 2, 2024

Copy link
Member

@stephenswat stephenswat left a comment

Choose a reason for hiding this comment

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

Looks good. Happy to approve if the CI accepts this.

Comment on lines +112 to +116
thrust_create_target( traccc::cuda_thrust
HOST CPP
DEVICE CUDA )
target_link_libraries( traccc_cuda
PRIVATE traccc::cuda_thrust )
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, if you link a target against multiple of these Thrust libraries, what happens? Does it give an error at configuration time or does it cause include ambiguities? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm desperately trying to find it in the code, but don't remember all the details anymore. 😦

I think the issue comes from the specific Thrust targets setting some custom target properties on themselves. See:

https://github.com/NVIDIA/cccl/blob/v2.6.1/thrust/thrust/cmake/thrust-config.cmake#L447-L448

If I remember correctly what I saw was that in case a target ends up being linked against let's say both Thrust::CPP::Device and Thrust::CUDA::Device, then CMake freaks out during the build system generation stage. Stating that the value of THRUST_INTERFACE_DEVICE couldn't be determined for the user's target. Or something like that.

But definitely, already during the CMake configuration one gets an error if trying to link against multiple "host" or "device" libraries at the same time. This is why traccc::Thrust is set up in this hacky way now. That I really can't use thrust_create_target(...) to make a target that would be appropriate for traccc::core. 😦

@krasznaa krasznaa merged commit 885d2d9 into acts-project:main Dec 2, 2024
27 checks passed
@krasznaa krasznaa deleted the ThrustUseUpdate-main-20241202 branch December 2, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build This relates to the build system cuda Changes related to CUDA sycl Changes related to SYCL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants