-
Notifications
You must be signed in to change notification settings - Fork 179
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
Fix Thrust/CUB Linkage Issues #443
Merged
gevtushenko
merged 17 commits into
NVIDIA:main
from
gevtushenko:fix-main/github/visibility
Sep 21, 2023
Merged
Fix Thrust/CUB Linkage Issues #443
gevtushenko
merged 17 commits into
NVIDIA:main
from
gevtushenko:fix-main/github/visibility
Sep 21, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gevtushenko
requested review from
elstehle,
wmaxey,
jrhemstad and
robertmaynard
and removed request for
a team
September 12, 2023 19:49
miscco
reviewed
Sep 12, 2023
miscco
approved these changes
Sep 19, 2023
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.
Minor nits
jrhemstad
reviewed
Sep 19, 2023
jrhemstad
reviewed
Sep 19, 2023
jrhemstad
approved these changes
Sep 19, 2023
elstehle
approved these changes
Sep 19, 2023
ahendriksen
reviewed
Sep 21, 2023
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 is a really clean PR with great documentation! I added some questions and one documentation suggestion.
robertmaynard
approved these changes
Sep 21, 2023
1 task
Closed
3 tasks
1 task
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
closes #166
Using CUB/Thrust in shared libraries is a known source of issues. For a while, the solution to these issues consisted of wrapping CUB/Thrust namespaces in
THRUST_CUB_WRAPPED_NAMESPACE
macro so that different shared libraries have different symbol names. This solution has poor discoverability, since issues present themselves in forms of segmentation faults, hangs, wrong results, etc. To eliminate the symbol visibility issues on our end, the following design is proposed:Hide kernel launchers: it’s important that kernel launchers like the Thrust
triple_chevron
always reside in the same library as the CUB/Thrust host API using these kernel launchers.Hide all kernels: it’s important that CUB/Thrust kernels always reside in the same library as the CUB/Thrust host API using these kernels.
Incorporate GPU architectures into symbol names: it’s important that kernels compiled for a given GPU architecture are always used by the CUB/Thrust host API compiled for that architecture.
Applying mentioned recommendations to the CCCL codebase involves the following changes:
thrust::cuda_cub::launcher::triple_chevron
is annotated with_LIBCUDACXX_HIDDEN
instead of annotating kernels with
__global__
we introduce{CUB,THRUST}_DETAIL_KERNEL_ATTRIBUTES
macro that’s equivalent to__global__ _LIBCUDACXX_HIDDEN
and annotate every CUB/Thrust kernel with itThrust symbols are placed inside an inline namespace containing the set of GPU architectures for which the TU is being compiled when CUDA is used as the device system.
Potentially breaking changes:
In Linux builds, CUB/Thrust kernels used to have default visibility. This PR changes that, so direct references to CUB/Thrust kernels in shared libraries will be broken. Since CUB/Thrust kernels are considered an implementation detail, this isn't a blocker for the PR.
Thrust ABI is broken when the set of GPU architectures doesn’t match. If a library exposes a function taking, say, device vector and is compiled for SM80, any code compiled for different architectures won’t be able to link against that library. The behavior can be opted out by defining
THRUST_DISABLE_ABI_NAMESPACE
or providing wrapped namespace.Checklist