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

Mark optional CUDA runtime functions as weak symbols #895

Conversation

robertmaynard
Copy link
Contributor

No description provided.

@robertmaynard robertmaynard added bug Something isn't working non-breaking Non-breaking change 5 - DO NOT MERGE Hold off on merging; see PR for details labels Oct 19, 2021
@robertmaynard robertmaynard requested a review from a team as a code owner October 19, 2021 18:31
@github-actions github-actions bot added the cpp Pertains to C++ code label Oct 19, 2021
@robertmaynard robertmaynard changed the base branch from branch-21.10 to branch-21.12 October 19, 2021 18:32
Will make it easier to support other compilers
cudaDevAttrMemoryPoolsSupported only checks that the driver supports cudaMemPools, not
that the runtime does as well.
Copy link

@JohnZed JohnZed left a comment

Choose a reason for hiding this comment

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

Looks great! Needs a review from a real RMM expert but these are just my $0.02.

#if CUDART_VERSION >= 11020 // 11.2 introduced cudaMallocAsync
#define RMM_CUDA_MALLOC_ASYNC_SUPPORT

#ifndef RMM_WEAK_ATTRIBUTE
Copy link

Choose a reason for hiding this comment

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

Maybe a comment is worth it here to explain what's going on?


// We first need to check if the runtime version supports cudaMemPool
// before checking that the driver does.
int version = 0;
Copy link

Choose a reason for hiding this comment

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

My personal style would be to break out these checks (driver + runtime) into a helper function like cuda_malloc_async_available() to simplify the constructor and encapsulate these annoying details. However, it is only used one place, so I can certainly live with this as well.

Copy link
Member

Choose a reason for hiding this comment

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

It's a good idea considering #889 may cause duplication of the checks.

@robertmaynard
Copy link
Contributor Author

Closing this PR as the weak symbol approach is non-viable for all supported platforms

@robertmaynard robertmaynard deleted the mark_optional_cuda_runtime_symbols_as_weak branch November 22, 2021 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - DO NOT MERGE Hold off on merging; see PR for details bug Something isn't working cpp Pertains to C++ code non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants