[cudax->libcu++] Move the hierarchy type from cudax to libcu++#6611
[cudax->libcu++] Move the hierarchy type from cudax to libcu++#6611pciolkosz merged 5 commits intoNVIDIA:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
pciolkosz
force-pushed
the
move_hierarchy_to_libcudacxx
branch
from
cae6dfb to
9b1a990
Compare
This comment has been minimized.
This comment has been minimized.
| */ | ||
| _CCCL_BEGIN_NAMESPACE_CUDA | ||
|
|
||
| namespace __detail |
There was a problem hiding this comment.
I'm surprised that we have a __detail namespace in libcu++
There was a problem hiding this comment.
We commonly do not use that, I would prefer to drop it if possible, but nothing critical
|
|
||
| namespace cuda::experimental | ||
| { | ||
| _CCCL_BEGIN_NAMESPACE_CUDA |
There was a problem hiding this comment.
the review process is hard. The files have been moved and GitHub doesn't mark many lines as under review. I will add the comments below.
There was a problem hiding this comment.
dim3 should be ::dim3 and fully qualify uint32_t (and similar)
There was a problem hiding this comment.
_CCCL_HOST_DEVICE should be _CCCL_API
There was a problem hiding this comment.
noexcept is missing in multiple points
There was a problem hiding this comment.
We will be doing a refactor of the hierarchy files later on, so I would not review it too deeply now
There was a problem hiding this comment.
that's entirely fine.
Reviewing in this way is hard. Github should allow reviewing moved files...
libcudacxx/include/cuda/__memory_resource/legacy_pinned_memory_resource.h
Show resolved
Hide resolved
pciolkosz
force-pushed
the
move_hierarchy_to_libcudacxx
branch
2 times, most recently
from
636245b to
85e87fc
Compare
pciolkosz
force-pushed
the
move_hierarchy_to_libcudacxx
branch
from
85e87fc to
73a666d
Compare
🥳 CI Workflow Results🟩 Finished in 1h 53m: Pass: 100%/124 | Total: 1d 01h | Max: 1h 47m | Hits: 97%/230572See results here. |
|
|
||
| if ("MSVC" STREQUAL "${CMAKE_CXX_COMPILER_ID}") | ||
| # libcudacxx requires dim3 to be usable from a constexpr context, and the CUDART headers require | ||
| # __cplusplus to be defined for this to work: |
There was a problem hiding this comment.
We should file a bug for them to fix that and use the appropriate MSVC dialect detection.
There was a problem hiding this comment.
I believe that is already fixed
wmaxey
left a comment
wmaxey
left a comment
There was a problem hiding this comment.
Approved for the CMake changes.
5 participants
The usual moving of APIs. The hierarchy and launch testing is a bit overlapped, so I had to temporarily comment out some tests.