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 unit test for scalar vs. CGSize1 probing #509

Merged
merged 4 commits into from
Jun 20, 2024

Conversation

sleeepyjack
Copy link
Collaborator

This PR adds a unit test to verify that the probing sequences for the scalar probing iterator vs. the iterator that uses a cooperative group of size 1 are actually equivalent.

@sleeepyjack sleeepyjack self-assigned this Jun 19, 2024
@sleeepyjack sleeepyjack requested a review from PointKernel as a code owner June 19, 2024 00:13
@sleeepyjack
Copy link
Collaborator Author

Found the culprit with double hashing:
Apparently this expression in the scalar code path is different from this one in the CG code path. I'd have to wrap my head around that logic again to fix this. Alternatively we could internally dispatch to the CG code path in the CG probing iterator when CGSize==1 and call it a day.

@PointKernel
Copy link
Member

With #501 fixing the sanitize hash logic, removing max from the scalar probing should fix the issue.

@sleeepyjack sleeepyjack added the Needs Review Awaiting reviews before merging label Jun 19, 2024
@sleeepyjack
Copy link
Collaborator Author

@PointKernel
Copy link
Member

that fix broke something else: https://github.com/NVIDIA/cuCollections/actions/runs/9584556889/job/26440581801?pr=509#step:7:758

Which fix are you referring to? removing max from double hashing?

@sleeepyjack
Copy link
Collaborator Author

Which fix are you referring to? removing max from double hashing?

Operator presedence: f0ae693

Not sure where my logic is flawed. 😢

@PointKernel
Copy link
Member

Which fix are you referring to? removing max from double hashing?

Operator presedence: f0ae693

Not sure where my logic is flawed. 😢

No good way but printing out all probing steps. I will take a look tomorrow.

Copy link
Member

@PointKernel PointKernel left a comment

Choose a reason for hiding this comment

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

Should be ready to go.

@sleeepyjack
Copy link
Collaborator Author

You sir are the MVP

@sleeepyjack sleeepyjack merged commit 6c0d7ee into NVIDIA:dev Jun 20, 2024
15 checks passed
@PointKernel PointKernel added the type: bug Something isn't working label Jun 20, 2024
@sleeepyjack sleeepyjack deleted the test-scalar-probing branch June 20, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review Awaiting reviews before merging type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants