-
Notifications
You must be signed in to change notification settings - Fork 447
Simplify usage of namespace macros, add thrust compatibility. #326
Conversation
d3231cc
to
11ce43d
Compare
5fc9f4c
to
214c5a4
Compare
214c5a4
to
bc87fbf
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.
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; |
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.
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?
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.
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.
bc87fbf
to
a26c1a2
Compare
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
a26c1a2
to
6631c72
Compare
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:
This is change requires NVIDIA/thrust#1464.