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

Fix merge-sort when sub/work group size is small #2002

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

carbotaniuman
Copy link
Contributor

On some implementations, the max subgroup size is 1, which is too small for the sorting algorithm to produce correct results. Make the value at least 4.

On some implementations, the max subgroup size is 1, which is too small
for the sorting algorithm to produce correct results. Make the value at
least 4.
@dmitriy-sobolev
Copy link
Contributor

dmitriy-sobolev commented Jan 17, 2025

@carbotaniuman, thanks for noting that issue. I would propose a slightly different approach correcting the chunk size, which appears to be more robust.

// The chunk size must not exceed two sorted sub-sequences to be merged,
// ensuring that at least one work-item processes them.
const _IndexT __chunk = std::min<_IndexT>(__is_cpu ? 32 : 4, __n_sorted * 2);

@carbotaniuman
Copy link
Contributor Author

I tested the change on my local installation and it produces the correct results with the new change.

Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev left a comment

Choose a reason for hiding this comment

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

LGTM, but I would ask @MikeDvorskiy to check the changes as well. I may be biased because I also contributed.

Copy link
Contributor

@MikeDvorskiy MikeDvorskiy left a comment

Choose a reason for hiding this comment

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

LGTM

@dmitriy-sobolev dmitriy-sobolev merged commit e945898 into uxlfoundation:main Jan 20, 2025
22 checks passed
@dmitriy-sobolev
Copy link
Contributor

dmitriy-sobolev commented Jan 20, 2025

@carbotaniuman, thank you once again. If you want your contributions to be mentioned in CREDITS.txt, do not hesitate to create a PR with their brief descriptions targeting that file.

@dmitriy-sobolev dmitriy-sobolev changed the title Ensure __max_sg_size is at least 4 in sort Fix merge-sort when sub/work group size is small Jan 20, 2025
@carbotaniuman carbotaniuman deleted the fix_sort_small_sg branch February 8, 2025 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants