[libcu++] Add explicit alignment argument to buffer construction#7623
[libcu++] Add explicit alignment argument to buffer construction#7623pciolkosz merged 6 commits intoNVIDIA:mainfrom
Conversation
|
|
||
| { // from size with no_init and allocation_alignment env (offset_by_alignment_resource verifies alignment) | ||
| const ::cuda::std::size_t alignment = ::cuda::mr::default_cuda_malloc_alignment / 2; | ||
| const auto env = ::cuda::std::execution::prop{::cuda::allocation_alignment, alignment}; |
There was a problem hiding this comment.
question: Is this the expected API most users would use? If so, this is an awfully verbose way to simply specify an allocation alignment.
There was a problem hiding this comment.
I plan to discuss some conventions around environment based APIs next week, so we can deep dive into this topic.
But for non-default alignment specifically, I consider it a ninja feature and I am not against it being verbose. But we could try to simplify it, for example having ::cuda::allocation_alignment wrap the alignment value instead of relying on ::cuda::std::execution::prop
There was a problem hiding this comment.
I wouldn't be too quick to assume that non-default alignment is a ninja feature. Many features essential for performance, like vectorized load/stores or TMA, require over-aligned allocations
There was a problem hiding this comment.
I believe we default case should be no alignment specified as is done here. If a user wants to specify the alignment they should be prepared to jump through a few hoops
That said, I believe this isn't too bad
pciolkosz
force-pushed
the
alignment_specification_for_buffer
branch
from
f45b475 to
6afce21
Compare
|
/ok to test 6afce21 |
This comment has been minimized.
This comment has been minimized.
|
/ok to test 0f456c5 |
This comment has been minimized.
This comment has been minimized.
libcudacxx/test/libcudacxx/cuda/containers/buffer/constructor.cu
Outdated
Show resolved
Hide resolved
|
|
||
| { // from size with no_init and allocation_alignment env (offset_by_alignment_resource verifies alignment) | ||
| const ::cuda::std::size_t alignment = ::cuda::mr::default_cuda_malloc_alignment / 2; | ||
| const auto env = ::cuda::std::execution::prop{::cuda::allocation_alignment, alignment}; |
There was a problem hiding this comment.
I believe we default case should be no alignment specified as is done here. If a user wants to specify the alignment they should be prepared to jump through a few hoops
That said, I believe this isn't too bad
pciolkosz
force-pushed
the
alignment_specification_for_buffer
branch
2 times, most recently
from
97e41bb to
9975146
Compare
libcudacxx/test/libcudacxx/cuda/containers/uninitialized_async_buffer.cu
Outdated
Show resolved
Hide resolved
Co-authored-by: Michael Schellenberger Costa <miscco@nvidia.com>
pciolkosz
force-pushed
the
alignment_specification_for_buffer
branch
from
2b26070 to
87ce705
Compare
🥳 CI Workflow Results🟩 Finished in 2h 40m: Pass: 100%/99 | Total: 1d 22h | Max: 2h 39m | Hits: 80%/252570See results here. |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin branch/3.3.x
git worktree add -d .worktree/backport-7623-to-branch/3.3.x origin/branch/3.3.x
cd .worktree/backport-7623-to-branch/3.3.x
git switch --create backport-7623-to-branch/3.3.x
git cherry-pick -x 633cb6bd51a2a36e10e7795a66f6aa61c3f5a3ac |
…DIA#7623) * Add explicit alignment argument to buffer * Fix format * Review feedback * Apply suggestions from code review Co-authored-by: Michael Schellenberger Costa <miscco@nvidia.com> * Fix format * Fix missing include --------- Co-authored-by: anon <users.noreply.github.com> Co-authored-by: Michael Schellenberger Costa <miscco@nvidia.com>
* [libcu++] Add explicit alignment argument to buffer construction (#7623) * Add explicit alignment argument to buffer * Fix format * Review feedback * Apply suggestions from code review Co-authored-by: Michael Schellenberger Costa <miscco@nvidia.com> * Fix format * Fix missing include --------- Co-authored-by: anon <users.noreply.github.com> Co-authored-by: Michael Schellenberger Costa <miscco@nvidia.com> * [libcu++] Add dynamic_accessibility_property added to all resources (#7727) * Add dynamic_accessibility_property to all memory resources * Fix forward property test * Workaround for gcc 7 * Review feedback * Address review comments * Fix merge conflict * Fix replacment issue --------- Co-authored-by: Michael Schellenberger Costa <miscco@nvidia.com> * Version bump buffer and resource wrappers (#7833) * Compilation fixes --------- Co-authored-by: pciolkosz <pciolkosz@nvidia.com> Co-authored-by: Wesley Maxey <71408887+wmaxey@users.noreply.github.com>
3 participants
Closes #7506
This PR adds
allocation_alignmenttag that can be passed to the buffer through env argument, which will be then passed to the resource on allocation and also stored in the buffer for later access and copy construction