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

Some additional kernel thread index refactoring. #14107

Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Sep 13, 2023

Description

This PR refactors a few kernels to use thread_index_type and associated utilities. I started this before realizing how much scope was still left in issue #10368 ("Part 2 - Take another pass over more challenging kernels"), and then I stopped working on this due to time constraints. For the moment, I hope this PR makes a small dent in the number of remaining kernels to convert to using thread_index_type.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@bdice bdice requested a review from a team as a code owner September 13, 2023 20:21
@bdice bdice self-assigned this Sep 13, 2023
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Sep 13, 2023
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Thanks for the fixups!

cpp/include/cudf/detail/copy_if_else.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/copy_if_else.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/copy_if_else.cuh Show resolved Hide resolved
cpp/include/cudf/detail/copy_if_else.cuh Show resolved Hide resolved
assert(start_idx < num_states);

curandState localState = state[start_idx];

for (size_type idx = start_idx; idx < build_tbl_size; idx += stride) {
for (thread_index_type tidx = start_idx; tidx < build_tbl_size; tidx += stride) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a range-based for loop to replace this pattern?

for(auto const tidx: grid_1d::thread_indices()) {...}

So we won't need to have stride like this.

Copy link
Contributor Author

@bdice bdice May 2, 2024

Choose a reason for hiding this comment

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

For now I'm going to leave that out of scope for this PR. I want to tighten the scope here and merge this since it's fairly old. Please feel free to comment on #10368 -- the topic of a range-based loop has already come up there.

@bdice bdice changed the base branch from branch-23.10 to branch-24.06 May 2, 2024 21:07
@bdice bdice marked this pull request as draft May 2, 2024 21:08
@bdice bdice marked this pull request as ready for review May 2, 2024 22:20
@bdice bdice requested review from ttnghia and harrism May 2, 2024 22:20
@bdice bdice added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 2, 2024
Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Looks good!

@bdice
Copy link
Contributor Author

bdice commented May 6, 2024

I pushed a couple small fixes for benchmark builds (I hadn't run that locally yet). This should be ready to merge after CI passes.

@bdice
Copy link
Contributor Author

bdice commented May 7, 2024

/merge

@rapids-bot rapids-bot bot merged commit a958274 into rapidsai:branch-24.06 May 7, 2024
70 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants