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

Add custom thrust/cub namespace when it is supported #1730

Merged
merged 7 commits into from
Nov 26, 2024

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Nov 21, 2024

This PR allows ginkgo to add custom namespace wrapper around thrust and cub.
We had the issue that ginkgo-alone perform correctly, but some thrust routine is hanging when linking with the other libraries also using thrust.
Ths similar experience had described in NVIDIA/cub#545
Some similar issues may be solved when the thrust is 1.17.2 or 2.1+ (NVIDIA/cub#572), which embeds all architecture in the namespace.
We always use thrust shipped from the cuda Toolkit, and cuda 11.6 (11.6.2 uses thrust 1.15) starts to accept the THRUST_CUB_WRAPPED_NAMESPACE

I put it as gko such that we do not need to use THRUST_NS_QUALIFIER as namespace, which is not defined before supporting custom namespace.
if we need to forward declare something without thrust header, we will need to add ginkgo macro to help that. (currently not neccessary)

Also, when thrust is shipped by CCCL, there is an issue missing inline with host compiler from NVIDIA/cccl#1922.
when using host compiler to compile thrust with custom namespace, it may lead cub::Debug multiple symbols or definition. It does not face the issue without custom namespace though. I avoid them now by ensuring executor.cpp does not include thrust header.

We enable it when the custom namespace supported

@yhmtsai yhmtsai added 1:ST:ready-for-review This PR is ready for review 1:ST:run-full-test labels Nov 21, 2024
@yhmtsai yhmtsai added this to the Ginkgo 1.9.0 milestone Nov 21, 2024
@yhmtsai yhmtsai requested a review from a team November 21, 2024 12:41
@yhmtsai yhmtsai self-assigned this Nov 21, 2024
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. reg:ci-cd This is related to the continuous integration system. reg:documentation This is related to documentation. mod:cuda This is related to the CUDA module. type:solver This is related to the solvers mod:hip This is related to the HIP module. type:factorization This is related to the Factorizations type:reordering This is related to the matrix(LinOp) reordering labels Nov 21, 2024
@yhmtsai yhmtsai force-pushed the custom_thrust_namespace branch 3 times, most recently from 07ed9a8 to 3bfdc5e Compare November 21, 2024 17:42
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.57%. Comparing base (681caa0) to head (96c9796).
Report is 4 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1730      +/-   ##
===========================================
- Coverage    90.94%   89.57%   -1.38%     
===========================================
  Files          782      782              
  Lines        63297    63527     +230     
===========================================
- Hits         57564    56902     -662     
- Misses        5733     6625     +892     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

CMakeLists.txt Outdated
Comment on lines 35 to 36
option(GINKGO_CUDA_CUSTOM_THRUST_NAMESPACE "Add custom namespace to thrust and cub in cuda to avoid potential break when the other libraries also uses thrust" OFF)
option(GINKGO_HIP_CUSTOM_THRUST_NAMESPACE "Add custom namespace to thrust in hip to avoid potential break when the other libraries also uses thrust" OFF)
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should always enable this if it is supported by the Thrust/rocThrust version we found. If we disable it by default, it may break people's code, I don't see a downside to enabling it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, minor fix: "when the other libraries also uses thrust" --> "when the other libraries also use thrust". And maybe instead of "break", we should say "conflicts"?

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 am not too confident about all thrust version always properly respect the custom namespace
like cccl inline issue from host compiler and rocThrust only implement it in half way for several versions.
I think enabling them by default but facing some compiling issue then user turn it off, which is somehow better than hanging somewhere else then taking long time to acknoledge the issue actually from thrust in my mind now.

@yhmtsai yhmtsai force-pushed the custom_thrust_namespace branch from 3bfdc5e to 96c9796 Compare November 22, 2024 13:57
@yhmtsai yhmtsai added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Nov 25, 2024
@yhmtsai yhmtsai force-pushed the custom_thrust_namespace branch from 96c9796 to c809aea Compare November 25, 2024 14:05
@yhmtsai yhmtsai changed the title Add custom thrust/cub namespace with option Add custom thrust/cub namespace when it is supported Nov 26, 2024
@yhmtsai yhmtsai force-pushed the custom_thrust_namespace branch from cecf2a0 to 96233a2 Compare November 26, 2024 10:34
Co-authored-by: Tobias Ribizel <ribizel@kit.edu>
@yhmtsai yhmtsai force-pushed the custom_thrust_namespace branch from 96233a2 to b85680c Compare November 26, 2024 10:36
@yhmtsai yhmtsai merged commit 6f3cb5b into develop Nov 26, 2024
10 of 11 checks passed
@yhmtsai yhmtsai deleted the custom_thrust_namespace branch November 26, 2024 17:45
@yhmtsai yhmtsai mentioned this pull request Nov 27, 2024
MarcelKoch pushed a commit to MarcelKoch/ginkgo that referenced this pull request Dec 2, 2024
…supported

This PR adds custom thrust namespace for hip and cuda when it is supported to reduce the potential conflict by thrust itself.
Hip enables it after 5.7 and cuda enable it after 11.6

Related PR: ginkgo-project#1730
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. 1:ST:run-full-test mod:cuda This is related to the CUDA module. mod:hip This is related to the HIP module. reg:build This is related to the build system. reg:ci-cd This is related to the continuous integration system. reg:documentation This is related to documentation. reg:testing This is related to testing. type:factorization This is related to the Factorizations type:reordering This is related to the matrix(LinOp) reordering type:solver This is related to the solvers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants