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

Jump to bottom

Clean up cudf device atomic with cuda::atomic_ref #13583

Merged
merged 11 commits into from
Jul 10, 2023
Merged

Clean up cudf device atomic with cuda::atomic_ref #13583

merged 11 commits into from
Jul 10, 2023

Conversation

PointKernel
Copy link
Member

@PointKernel PointKernel commented Jun 14, 2023 •
edited
Loading

Description

Contributes to #13575

Depends on #13574, #13578

This PR cleans up custom atomic implementations in libcudf by using cuda::atomic_ref when possible. It removes atomic bitwise operations like and, or and xor since libcudac++ already provides proper replacements.

Checklist

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

@PointKernel PointKernel added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue tech debt improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 14, 2023
@PointKernel PointKernel self-assigned this Jun 14, 2023
@PointKernel PointKernel closed this Jul 9, 2023
@PointKernel PointKernel reopened this Jul 9, 2023
@github-actions github-actions bot removed the CMake CMake build issue label Jul 9, 2023
PointKernel
PointKernel commented Jul 9, 2023
View reviewed changes
cpp/tests/hash_map/map_test.cu
Comment on lines -71 to -74
key_value_types<int8_t, int8_t>,
key_value_types<int16_t, int16_t>,
key_value_types<int8_t, float>,
key_value_types<int16_t, double>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed types not supported by atomic_ref. Not really an issue since those data structures will be replaced by cuco ones soon.

PointKernel
PointKernel commented Jul 9, 2023
View reviewed changes
cpp/src/text/minhash.cu
@@ -92,7 +93,9 @@ struct minhash_fn {
for (std::size_t seed_idx = 0; seed_idx < seeds.size(); ++seed_idx) {
auto const hasher = cudf::detail::MurmurHash3_32<cudf::string_view>{seeds[seed_idx]};
auto const hvalue = hasher(hash_str);
atomicMin(d_output + seed_idx, hvalue);
cuda::atomic_ref<cudf::hash_value_type, cuda::thread_scope_block> ref{
Copy link
Member Author

Choose a reason for hiding this comment

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

IIUC, this is a per warp function thus using thread_scope_block is safe.

@PointKernel PointKernel added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jul 10, 2023
@PointKernel PointKernel marked this pull request as ready for review July 10, 2023 02:01
@PointKernel PointKernel requested a review from a team as a code owner July 10, 2023 02:01
bdice
bdice approved these changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Minor question about #includes. I think your assessment of thread_scope_block is correct, so I'll approve. Thanks for doing this!

cpp/src/join/mixed_join_size_kernels_semi.cu
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we missing #include <cuda/atomic> in this file? Also applies to a few other files.

Copy link
Member Author

Choose a reason for hiding this comment

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

The current join implementations include all shared headers in join_common_utils.hpp thus each file relies on transitive inclusions. Considering join will be refactored with new cuco hash maps, my plan was to refactor/cleanup this during the cuco migration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. No worries!

cpp/src/strings/case.cu Show resolved Hide resolved
@davidwendt
Copy link
Contributor

Is something in cpp/include/cudf/detail/utilities/device_atomics.cuh still being used?

@PointKernel
Copy link
Member Author

Is something in cpp/include/cudf/detail/utilities/device_atomics.cuh still being used?

Yes, we cannot close the issue for now since some types are not supported by atomic_ref yet like durations, timestamps and bools. Opened NVIDIA/cccl#1024 to track this.

@PointKernel
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 67deda0 into rapidsai:branch-23.08 Jul 10, 2023
53 checks passed
@PointKernel PointKernel deleted the remove-device-atomic branch July 10, 2023 18:35
@davidwendt davidwendt mentioned this pull request Jan 29, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

@davidwendt davidwendt davidwendt approved these changes

@vyasr vyasr Awaiting requested review from vyasr vyasr is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees

@PointKernel PointKernel

Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
cuDF/Dask/Numba/UCX
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PointKernel @davidwendt @bdice