-
Notifications
You must be signed in to change notification settings - Fork 912
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
Patch cccl and cuco so that all CUDA kernels they provide have internal linkage #14735
Patch cccl and cuco so that all CUDA kernels they provide have internal linkage #14735
Conversation
439ffdc
to
dd165b0
Compare
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 seems fine to me. @robertmaynard Can you expand a bit more on these questions before I approve:
-
How this was tested/validated with Spark-RAPIDS?
Since the CCCL changes mean C++ and CUDA sources have non compatible ABI's, we need to specify
THRUST_DISABLE_ABI_NAMESPACE
.
To clarify, "the CCCL changes" are from the patch in this PR, and they are the same namespace changes as in 2.3.0? (In other words, this patch is a partial backport of 2.3.0?)
If these patches work for cudf CI, I believe we can move them to rapids-cmake and have the rapids_cpm_cccl package automatically provide them.
This PR's CI is passing. Will we be applying these same CCCL/cuCollections patches to the rapids-cmake CCCL/cuCollections? Do you want to open that PR and test it on other RAPIDS repos (e.g. RAFT, cuML, cuGraph) before merging this one?
|
Closing as rapidsai/rapids-cmake#523 has been merged |
Description
Patch both CCCL and CUCO to have only internal linkage.
For cuco I am working on upstreaming these changes ( NVIDIA/cuCollections#422 ). Once that is accepted and we have validated that moving cuco is stable ( e.g. changes around
cuco::experimental::static_set
) we can drop this patch set.For cccl the long term fix is to move to CCCL 2.3+, but due to issues ( NVIDIA/cccl#1249, maybe others ) that isn't viable currently. If these patches work for cudf CI, I believe we can move them to rapids-cmake and have the
rapids_cpm_cccl
package automatically provide them.Since the CCCL changes mean C++ and CUDA sources have non compatible ABI's, we need to specify
THRUST_DISABLE_ABI_NAMESPACE
. Likewise we need to specifyTHRUST_IGNORE_ABI_NAMESPACE_ERROR
so that we don't change our ABI ( since it depends on thrust types ).Checklist