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

Make Copy_if family of APIs use reduce_then_scan algorithm #1763

Merged
merged 14 commits into from
Aug 30, 2024

Conversation

danhoeflinger
Copy link
Contributor

@danhoeflinger danhoeflinger commented Aug 5, 2024

This PR adds copy_if family to use reduce_then_scan algorithm where it is beneficial.

Moves all algorithm selection decisions to __parallel_copy_if, and unifies range API to also use this function. Adding support for an assignment operator for single work group copy_if. This allows us to unify the algorithmic selection, and provide performance improvements to the ranges API.

Adds __parallel_reduce_then_scan_copy function which will be used by partition and unique in the future to take advantage of shared functionality.


This PR is targeted to #1762, to allow for a clean diff, and is a part of the following sequence of PRs meant to be merged in order:

#1769 [MERGED] Relocate __lazy_ctor_storage to utils header
#1770 [MERGED] Use __result_and_scratch_storage within scan kernels
#1762 Add reduce_then_scan algorithm for transform scan family
#1763 Make Copy_if family of APIs use reduce_then_scan algorithm (This PR)
#1764 Make Partition family of APIs use reduce_then_scan algorithm
#1765 Make Unique family of APIs use reduce_then_scan algorithm

This work is a collaboration between @mmichel11 @adamfidel and @danhoeflinger, and based upon an original prototype by Ted Painter.

@danhoeflinger danhoeflinger force-pushed the dev/shared/copy_if_reduce_then_scan branch 4 times, most recently from b517414 to d85ce61 Compare August 6, 2024 14:18
@danhoeflinger danhoeflinger force-pushed the dev/shared/transform_reduce_then_scan branch from 9eb3099 to fc43983 Compare August 6, 2024 17:16
@danhoeflinger danhoeflinger force-pushed the dev/shared/copy_if_reduce_then_scan branch 3 times, most recently from e33efaa to 5f4882a Compare August 6, 2024 18:10
@danhoeflinger danhoeflinger added this to the 2022.7.0 milestone Aug 6, 2024
@danhoeflinger danhoeflinger force-pushed the dev/shared/transform_reduce_then_scan branch from 6cb5fc1 to 82f3ee5 Compare August 6, 2024 19:02
@danhoeflinger danhoeflinger force-pushed the dev/shared/copy_if_reduce_then_scan branch 3 times, most recently from 6696396 to f757df5 Compare August 6, 2024 20:59
@danhoeflinger danhoeflinger changed the title [Draft] Make copy_if family of APIs to use reduce_then_scan algorithm [Draft] Make Copy_if family of APIs use reduce_then_scan algorithm Aug 6, 2024
@danhoeflinger danhoeflinger force-pushed the dev/shared/transform_reduce_then_scan branch from 4df758a to 690e121 Compare August 7, 2024 14:44
@danhoeflinger danhoeflinger force-pushed the dev/shared/copy_if_reduce_then_scan branch from f757df5 to 3301a89 Compare August 7, 2024 14:48
@danhoeflinger danhoeflinger force-pushed the dev/shared/transform_reduce_then_scan branch from 690e121 to d0ef883 Compare August 7, 2024 16:40
@danhoeflinger danhoeflinger force-pushed the dev/shared/copy_if_reduce_then_scan branch from 3301a89 to 000a823 Compare August 7, 2024 16:41
@danhoeflinger danhoeflinger force-pushed the dev/shared/copy_if_reduce_then_scan branch from d318354 to 1da7875 Compare August 27, 2024 17:47
julianmi
julianmi previously approved these changes Aug 29, 2024
Copy link
Contributor

@julianmi julianmi left a comment

Choose a reason for hiding this comment

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

LGTM. I would hold off merging this until the main patch was merged.

@danhoeflinger danhoeflinger force-pushed the dev/shared/copy_if_reduce_then_scan branch from 1da7875 to 8601892 Compare August 29, 2024 16:31
Copy link
Contributor Author

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

This one LGTM, after rebasing minor changes from transform_scan.

@danhoeflinger danhoeflinger force-pushed the dev/shared/copy_if_reduce_then_scan branch from 8601892 to df02a01 Compare August 29, 2024 20:19
Base automatically changed from dev/shared/transform_reduce_then_scan to main August 30, 2024 15:42
@danhoeflinger danhoeflinger dismissed julianmi’s stale review August 30, 2024 15:42

The base branch was changed.

@danhoeflinger danhoeflinger force-pushed the dev/shared/copy_if_reduce_then_scan branch from df02a01 to 75c55c4 Compare August 30, 2024 15:46
julianmi
julianmi previously approved these changes Aug 30, 2024
Copy link
Contributor

@julianmi julianmi left a comment

Choose a reason for hiding this comment

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

LGTM. Please wait for the CI before merging this.

adamfidel
adamfidel previously approved these changes Aug 30, 2024
Copy link
Contributor

@adamfidel adamfidel left a comment

Choose a reason for hiding this comment

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

This PR looks good to me.

Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger danhoeflinger dismissed stale reviews from adamfidel and julianmi via 1918518 August 30, 2024 18:52
Copy link
Contributor

@mmichel11 mmichel11 left a comment

Choose a reason for hiding this comment

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

LGTM

@danhoeflinger danhoeflinger merged commit 7e2fb7a into main Aug 30, 2024
19 checks passed
@danhoeflinger danhoeflinger deleted the dev/shared/copy_if_reduce_then_scan branch August 30, 2024 19:15
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