Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Simplify usage of namespace macros, add thrust compatibility. #326

Merged
merged 1 commit into from
Jul 19, 2021

Conversation

alliepiper
Copy link
Collaborator

@alliepiper alliepiper commented Jun 18, 2021

This provides a workaround for downstream projects that encounter
a variety of issues from dynamically linking multiple libraries that
use Thrust and CUB.

See the new cub/util_namespace.h header for details.

Added several tests and checks to validate that this behavior is correct.

New tests:

  • test/test_namespace_wrapped.cu
  • test/cmake/check_namespace.cmake

This is change requires NVIDIA/thrust#1464.

Copy link
Collaborator

@gevtushenko gevtushenko left a comment

Choose a reason for hiding this comment

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

All of the header files contain the new namespace macro. Ones that don't contain it don't require it (macro + includes):

cub> find . -type f | wc -l
96
cub> find . -name "*cuh" -exec grep -l 'CUB_NAMESPACE_BEGIN' {} \; | wc -l
88
cub> find . -name "*cuh" -exec grep -l 'CUB_NAMESPACE_END' {} \; | wc -l
88

The same applies to the inclusion of util_namespace.cuh which can be found in config.cuh:

find . -name "*cuh" -exec grep -L 'config\.cuh' {} \; | wc -l
14

Headers that don't include config.h include util_namespace.cuh directly.

So it seems good to me. Thank you for this work!

There is one minor question that can be skipped, though.

@@ -402,6 +398,7 @@ CUB_RUNTIME_FUNCTION inline cudaError_t PtxVersionUncached(int& ptx_version)
__host__ inline cudaError_t PtxVersionUncached(int& ptx_version, int device)
{
SwitchDevice sd(device);
(void)sd;
Copy link
Collaborator

Choose a reason for hiding this comment

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

SwitchDevice has the non-trivial constructor and destructor. Its object won't cause unused variable warnings in this context. Have you encountered an issue here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't remember if I actually hit an error here, but either way I still prefer to explicitly mark these as used. Unused variable warnings are notorious for false positives, so adding these defensively helps avoid situations where a new compiler release will create issues for our users.

This provides a workaround for downstream projects that encounter
a variety of issues from dynamically linking multiple libraries that
use Thrust and CUB.

See the `cub/util_namespace.h` header for details.

Added tests and checks to validate that this behavior is correct.

New tests:
  - test/test_namespace_wrapped.cu
  - test/cmake/check_namespace.cmake
@alliepiper alliepiper marked this pull request as ready for review July 19, 2021 19:28
@alliepiper alliepiper merged commit 36c7b55 into NVIDIA:main Jul 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants