Skip to content

[cudax->libcu++] Move buffer type from cudax to libcu++ - #6627

Merged
pciolkosz merged 13 commits into
NVIDIA:mainfrom
pciolkosz:move_buffer_to_libcudacxx
Dec 1, 2025
Merged

pciolkosz merged 13 commits into
NVIDIA:mainfrom
pciolkosz:move_buffer_to_libcudacxx

Conversation

@pciolkosz

Copy link
Copy Markdown
Contributor

PR moving last bits of the buffer type and de-duplicating the __memory_accessibility enum

@pciolkosz
pciolkosz requested review from a team as code owners November 14, 2025 01:57
@pciolkosz
pciolkosz requested a review from griwes November 14, 2025 01:57
@github-project-automation github-project-automation Bot moved this to Todo in CCCL Nov 14, 2025
@cccl-authenticator-app cccl-authenticator-app Bot moved this from Todo to In Review in CCCL Nov 14, 2025
@github-actions

This comment has been minimized.

@pciolkosz
pciolkosz requested a review from a team as a code owner November 14, 2025 23:09
@pciolkosz
pciolkosz requested a review from wmaxey November 14, 2025 23:09
@github-actions

This comment has been minimized.

Comment thread cudax/test/CMakeLists.txt
@@ -119,15 +119,6 @@ foreach (cudax_target IN LISTS cudax_TARGETS)

cudax_add_catch2_test(test_target containers ${cudax_target}
containers/uninitialized_buffer.cu

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

arent we moving uninitialized_buffer too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't really use it anywhere, I think it can stay experimental until we have a usage for it

@bernhardmgruber bernhardmgruber Nov 25, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the new replacement for thrust::device_vector<T>(n, thrust::no_init), which is used everywhere in benchmarks and tests?

@pciolkosz pciolkosz Nov 25, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

cuda::make_buffer<T>(stream, cuda::device_default_memory_pool(cuda::devices[i]), n, cuda::no_init). We might consider a version of it in the future that does not take a stream

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would look much better if we could just do:

cuda::make_buffer<T>(stream, cuda::devices[i], n, cuda::no_init);

We could add a constructor from cuda::memory_location which would mean the default memory pool

Comment thread libcudacxx/include/cuda/__container/buffer.h Outdated
Comment thread libcudacxx/include/cuda/__container/uninitialized_async_buffer.h Outdated
Comment thread libcudacxx/include/cuda/__memory_resource/synchronous_resource_adapter.h Outdated
Comment thread libcudacxx/test/libcudacxx/cuda/containers/buffer/conversion.cu
Comment thread libcudacxx/include/cuda/__container/buffer.h
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Comment thread libcudacxx/include/cuda/__container/heterogeneous_iterator.h Outdated
@davebayer

Copy link
Copy Markdown
Contributor

pre-commit.ci autofix

@copy-pr-bot

copy-pr-bot Bot commented Dec 1, 2025

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Comment thread libcudacxx/test/libcudacxx/cuda/containers/buffer/properties.cu Outdated
@davebayer

Copy link
Copy Markdown
Contributor

pre-commit.ci autofix

@davebayer

Copy link
Copy Markdown
Contributor

/ok to test c804cfe

@copy-pr-bot

copy-pr-bot Bot commented Dec 1, 2025

Copy link
Copy Markdown
Contributor

/ok to test c804cfe

@davebayer, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

@davebayer

Copy link
Copy Markdown
Contributor

/ok to test a804cfe

@github-actions

github-actions Bot commented Dec 1, 2025

Copy link
Copy Markdown
Contributor

🥳 CI Workflow Results

🟩 Finished in 9h 38m: Pass: 100%/122 | Total: 1d 21h | Max: 3h 01m | Hits: 99%/215255

See results here.

@pciolkosz
pciolkosz merged commit 5dd8bdb into NVIDIA:main Dec 1, 2025
135 of 136 checks passed
@github-project-automation github-project-automation Bot moved this from In Review to Done in CCCL Dec 1, 2025
@github-actions

github-actions Bot commented Dec 1, 2025

Copy link
Copy Markdown
Contributor

Backport failed for branch/3.2.x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin branch/3.2.x
git worktree add -d .worktree/backport-6627-to-branch/3.2.x origin/branch/3.2.x
cd .worktree/backport-6627-to-branch/3.2.x
git switch --create backport-6627-to-branch/3.2.x
git cherry-pick -x 5dd8bdb567b92ddec5f81cf7f49a61d31dea41e7

pciolkosz added a commit to pciolkosz/cccl that referenced this pull request Dec 2, 2025
* Move buffer type from cudax to libcu++

* Fix cudax compilation

* Fix merge and review feedback

* Waive buffer prologue include checks

* Use unwrapped_begin in get_unsynchronized

* ifdef problematic cases for now

* Apply suggestion from @davebayer

* [pre-commit.ci] auto code formatting

* Apply suggestion from @davebayer

---------

Co-authored-by: David Bayer <48736217+davebayer@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
pciolkosz added a commit to pciolkosz/cccl that referenced this pull request Dec 2, 2025
* Move buffer type from cudax to libcu++

* Fix cudax compilation

* Fix merge and review feedback

* Waive buffer prologue include checks

* Use unwrapped_begin in get_unsynchronized

* ifdef problematic cases for now

* Apply suggestion from @davebayer

* [pre-commit.ci] auto code formatting

* Apply suggestion from @davebayer

---------

Co-authored-by: David Bayer <48736217+davebayer@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Comment on lines +14 to +15
// Temporary workaround to not trigger issues in CUB headers missing prologue
#define _CCCL_WAIVE_PROLOGUE_INCLUDE_CHECK

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What was the problem? I don't like that we are disabling this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

CUB didn't properly include prologue headers and I didn't want to try to fix that as part of this PR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand but then there are these workarounds coming out of nowhere. I think it would be better to fix this problem in another PR before merging this one.

It seems that we will include more cub headers, so we need a systematic solution.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fixed by #6844

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I still think it would be nice to have an opt-out like that available for similar future cases, especially since the prologue thing is not essential, but I don't mind removing it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, it is absolutely essential. All code in libcu++ should be guarded by the prologue/epilogue include. We do many important stuff there to prevent other macros breaking our code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But now we also include CUB in libcu++ and CUB doesn't use it, so I would be cautious to call it essential

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And not only that, we suppress compiler warnings and other stuff. That's why the checks for the prologue/epilogue wrapping are so strict (and annoying)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CUB doesn't do many of the things we do in libcu++, but that doesn't mean we should relax requirements on what we do in libcu++

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am with Dave here, this is potentially breaking libcu++ which we must not do

pciolkosz added a commit to pciolkosz/cccl that referenced this pull request Dec 4, 2025
* Move buffer type from cudax to libcu++

* Fix cudax compilation

* Fix merge and review feedback

* Waive buffer prologue include checks

* Use unwrapped_begin in get_unsynchronized

* ifdef problematic cases for now

* Apply suggestion from @davebayer

* [pre-commit.ci] auto code formatting

* Apply suggestion from @davebayer

---------

Co-authored-by: David Bayer <48736217+davebayer@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
davebayer added a commit that referenced this pull request Dec 4, 2025
* Move buffer type from cudax to libcu++

* Fix cudax compilation

* Fix merge and review feedback

* Waive buffer prologue include checks

* Use unwrapped_begin in get_unsynchronized

* ifdef problematic cases for now

* Apply suggestion from @davebayer

* [pre-commit.ci] auto code formatting

* Apply suggestion from @davebayer

---------

Co-authored-by: David Bayer <48736217+davebayer@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants