Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not move arrival_token in barrier::try_wait_* #283

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+2 −2
Open

Do not move arrival_token in barrier::try_wait_* #283

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions libcudacxx/include/cuda/std/detail/libcxx/include/__cuda/barrier.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,15 +525,15 @@ friend class _CUDA_VSTD::__barrier_poll_tester_parity;

template<class _Rep, class _Period>
_LIBCUDACXX_NODISCARD_ATTRIBUTE _LIBCUDACXX_INLINE_VISIBILITY
bool try_wait_for(arrival_token && __token, const _CUDA_VSTD::chrono::duration<_Rep, _Period>& __dur) {
bool try_wait_for(arrival_token & __token, const _CUDA_VSTD::chrono::duration<_Rep, _Period>& __dur) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a test that would have caught this? e.g., a try_wait_for that fails and retries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The current tests for this are a little janky:

cccl/libcudacxx/.upstream-tests/test/std/thread/thread.barrier/try_wait_for.pass.cpp

Line 41 in 137f5c7

while(b->try_wait_for(cuda::std::move(*tok), delay) == false) {}

It's moving from a moved-from object in a loop.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This may not actually be possible to do without adding some runtime state tracking to track a moved-from token. So it may not be worth it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Main thing is to just remove the std::move which would catch not needing to pass an r-value ref.

auto __nanosec = _CUDA_VSTD::chrono::duration_cast<_CUDA_VSTD::chrono::nanoseconds>(__dur);

return __try_wait(_CUDA_VSTD::move(__token), __nanosec);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are still moving here, is that intentional?

Applies below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is intentional in the sense that I tried to make as few changes as possible. I did not change the internal APIs from && to &. One thing I had not considered is that arrival_token might actually invalidate the original location on move. Does it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs to update the internal APIs as well.

}

template<class _Clock, class _Duration>
_LIBCUDACXX_NODISCARD_ATTRIBUTE _LIBCUDACXX_INLINE_VISIBILITY
bool try_wait_until(arrival_token && __token, const _CUDA_VSTD::chrono::time_point<_Clock, _Duration>& __time) {
bool try_wait_until(arrival_token & __token, const _CUDA_VSTD::chrono::time_point<_Clock, _Duration>& __time) {
return try_wait_for(_CUDA_VSTD::move(__token), (__time - _Clock::now()));
}

Expand Down
Loading