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.

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

Add device non-shmem pair_retrieve functions #122

Merged
merged 24 commits into from
Dec 16, 2021

Conversation

PointKernel
Copy link
Member

@PointKernel PointKernel commented Dec 7, 2021

This PR added device pair_retrieve functions that don't use a shared memory output buffer. It included code cleanups like using count_least_significant_bits() as well.

@PointKernel PointKernel added type: feature request New feature request helps: rapids Helps or needed by RAPIDS labels Dec 7, 2021
@PointKernel
Copy link
Member Author

@vyasr @jrhemstad Here is preliminary work mainly to get your feedback. Will add corresponding tests shortly.

Comment on lines 1126 to 1132
// TODO: `=` operator cannot work here
// *(probe_output_begin + output_idx) = pair;
// *(contained_output_begin + output_idx) = arr[0];
thrust::get<0>(*(probe_output_begin + output_idx)) = pair.first;
thrust::get<1>(*(probe_output_begin + output_idx)) = pair.second;
thrust::get<0>(*(contained_output_begin + output_idx)) = arr[0].first;
thrust::get<1>(*(contained_output_begin + output_idx)) = arr[0].second;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why doesn't this work?

Copy link
Member Author

Choose a reason for hiding this comment

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

no idea by what miracle the = operator works just fine right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

By writing unit tests for non-shmem pair_retrieve, I see the error again:

/home/yunsongw/Work/cuCollections/include/cuco/detail/static_multimap/device_view_impl.inl(1120): error: no operator "=" matches these operands
            operand types are: thrust::detail::tuple_of_iterator_references<thrust::detail::any_assign &, thrust::detail::any_assign &> = const cuco::pair<int32_t, int32_t>

and in this case, both probe_output_begin and contained_output_begin are defined as

auto begin = thrust::make_zip_iterator(
    thrust::make_tuple(thrust::make_discard_iterator(), thrust::make_discard_iterator()));

Copy link
Collaborator

Choose a reason for hiding this comment

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

In light of our discussion offline, let's add a comment as to why we must assign the iterator elements individually rather than as a pair.

@vyasr
Copy link
Collaborator

vyasr commented Dec 7, 2021

FYI I plan to review this, I'm just trying to get my code far enough along to be able to actually use this feature so that I can actually test it in action in addition to reviewing the code.

@jrhemstad
Copy link
Collaborator

@vyasr are you still planning to review?

@vyasr
Copy link
Collaborator

vyasr commented Dec 15, 2021

@jrhemstad yes, sorry for the long delay. I'm very close to having a reviewable prototype of the code in cudf that's using this, so I've been focusing on ironing out issues there in case any of them have an impact on this API/implementation. I'll probably be able to review this tomorrow.

Copy link
Collaborator

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

This LGTM. I have some minor suggestions that I chose to ignore for now since they'll possibly (probably) be moot points after the refactor.

@vyasr vyasr merged commit 193de1a into NVIDIA:dev Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helps: rapids Helps or needed by RAPIDS type: feature request New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants