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

[SYCLomatic] Simplify segmented sort algorithms #1347

Conversation

danhoeflinger
Copy link
Contributor

Remove the two pair sorts algorithm options from segmented sort APIs. This shouldn't hurt much performance wise at runtime, as most cases will not reach this code, but it will significantly reduce the number of kernels required to JIT compile.

Must be preceded by merging of oneapi-src/SYCLomatic-test#483, to remove the direct testing of this internal API.

Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger danhoeflinger requested a review from a team as a code owner October 9, 2023 20:16
Copy link
Contributor

@tomflinda tomflinda left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -2212,17 +2148,11 @@ segmented_sort_keys(
dpct::internal::segmented_sort_keys_by_parallel_for_of_sorts(
::std::forward<Policy>(policy), keys_in, keys_out, n, nsegments,
begin_offsets, end_offsets, descending, begin_bit, end_bit);
} else if (nsegments < 512) // for loop of parallel sorts when we have a small
// number of total sorts to limit total overhead
} else
Copy link
Contributor

Choose a reason for hiding this comment

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

This line need format

@@ -2272,19 +2202,12 @@ segmented_sort_pairs(
::std::forward<Policy>(policy), keys_in, keys_out, values_in,
values_out, n, nsegments, begin_offsets, end_offsets, descending,
begin_bit, end_bit);
} else if (nsegments < 512) // for loop of parallel sorts when we have a small
// number of total sorts to limit total overhead
} else
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor

@yihanwg yihanwg left a comment

Choose a reason for hiding this comment

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

LGTM with comments

Copy link
Contributor

@zhimingwang36 zhimingwang36 left a comment

Choose a reason for hiding this comment

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

LGTM

@zhimingwang36 zhimingwang36 merged commit f2e7926 into oneapi-src:SYCLomatic Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants