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

Separate MurmurHash32 from hash_functions.cuh #13681

Merged
merged 15 commits into from
Jul 16, 2023
Merged

Separate MurmurHash32 from hash_functions.cuh #13681

merged 15 commits into from
Jul 16, 2023

Conversation

davidwendt
Copy link
Contributor

@davidwendt davidwendt commented Jul 10, 2023 •
edited
Loading

Description

Moves the MurmurHash32 class definition from hash_functions.cuh to a new murmur32.cuh file.
Also moves the new file and the hash_functions.cuh from cpp/include/cudf/detail/utilities/ to cpp/include/cudf/hashing/detail
The hash functions were redeclared from the cudf::detail namespace to the cudf::hashing::detail namespace.

The remaining changes are side-effects of making the above changes.
This PR is a follow on to PR #13626
No new logic or functions have changed. Only internal detail headers have been refactored.

Checklist

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

@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 10, 2023
@davidwendt davidwendt self-assigned this Jul 10, 2023
@github-actions github-actions bot added the conda label Jul 10, 2023
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jul 11, 2023
@davidwendt davidwendt marked this pull request as ready for review July 11, 2023 19:52
@davidwendt davidwendt requested review from a team as code owners July 11, 2023 19:52
@davidwendt davidwendt requested review from vyasr, mythrocks and bdice July 11, 2023 19:52
ajschmidt8
ajschmidt8 approved these changes Jul 11, 2023
View reviewed changes
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes

bdice
bdice reviewed Jul 13, 2023
View reviewed changes
cpp/include/cudf/hashing/detail/hash_functions.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/hashing/detail/murmur_hash32.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/hashing/detail/murmur_hash32.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/join.hpp Outdated Show resolved Hide resolved
cpp/src/hash/murmur_hash.cu Outdated Show resolved Hide resolved
cpp/include/cudf/hashing/detail/hash_functions.cuh Outdated Show resolved Hide resolved
@davidwendt davidwendt requested a review from a team as a code owner July 13, 2023 19:30
@github-actions github-actions bot added the CMake CMake build issue label Jul 13, 2023
bdice
bdice approved these changes Jul 13, 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.

Approving with minor questions/suggestions.

cpp/include/cudf/hashing/detail/default_hash.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/hashing/detail/hashing.hpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I could see an argument to move the hash_combine utility functions like from this file into hashing/detail/hash_functions.cuh. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of these are used by .cpp files so I did not want them to include hash_functions.cuh which has some CUDA intrinsics in it.

Copy link
Contributor

@bdice bdice Jul 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW Those CUDA intrinsics for funnel shifts don't actually compile to fewer instructions. Originally I thought it did make a difference, but I double checked this a while back and didn't see any change in the PTX/SASS. I just haven't had a chance to go back and replace those. If it helps, we can replace the intrinsics with the equivalent shifting code (which is CPU-compatible) and move them to an appropriate location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can address this in a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

#13706

cpp/src/hash/spark_murmurhash3_x86_32.cu Outdated Show resolved Hide resolved
cpp/src/partitioning/partitioning.cu Outdated Show resolved Hide resolved
@davidwendt davidwendt changed the title Separate MurmurHash32 from hash_functions.cuh Separate MurmurHash32 from hash_functions.cuh Jul 14, 2023
@bdice bdice mentioned this pull request Jul 14, 2023
Merged
3 tasks
karthikeyann
karthikeyann approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@karthikeyann karthikeyann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me.
Only couple of minor questions to be clarified. could be addressed in future PR too.

cpp/src/hash/md5_hash.cu Show resolved Hide resolved
cpp/src/partitioning/partitioning.cu Outdated Show resolved Hide resolved
cpp/include/cudf/hashing/detail/default_hash.cuh Show resolved Hide resolved
cpp/src/partitioning/partitioning.cu Outdated Show resolved Hide resolved
@bdice bdice mentioned this pull request Jul 15, 2023
Merged
3 tasks
@davidwendt
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 0784543 into rapidsai:branch-23.08 Jul 16, 2023
@davidwendt davidwendt deleted the sep-murmur32-hash-functions branch July 16, 2023 21:00
@ttnghia ttnghia added breaking Breaking change and removed non-breaking Non-breaking change labels Jul 17, 2023
@ttnghia
Copy link
Contributor

ttnghia commented Jul 17, 2023

Re-label as breaking as it causes breaking in spark-rapids-jni.

@bdice bdice mentioned this pull request Jul 17, 2023
Open
@nvdbaranec nvdbaranec mentioned this pull request Jul 17, 2023
Merged
@bdice bdice mentioned this pull request Jul 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karthikeyann karthikeyann karthikeyann approved these changes

@bdice bdice bdice approved these changes

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

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

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

Assignees

@davidwendt davidwendt

Labels
3 - Ready for Review Ready for review by team breaking Breaking change CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@davidwendt @ttnghia @bdice @karthikeyann @ajschmidt8