TL/UCP: reduce srg #1035
Open
TL/UCP: reduce srg #1035
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sergei-Lebedev
force-pushed
the
topic/reduce_srg
branch
from
f181526 to
d7d5bb9
Compare
|
Can one of the admins verify this patch? |
samnordmann
reviewed
Feb 6, 2025
| @@ -83,7 +85,7 @@ static inline ucc_rank_t ucc_kn_pattern_radix_pow_init(ucc_knomial_pattern_t *p, | |||
| static inline void | |||
| ucc_knomial_pattern_init_impl(ucc_rank_t size, ucc_rank_t rank, | |||
| ucc_kn_radix_t radix, ucc_knomial_pattern_t *p, | |||
| int backward) | |||
| int backward, int extra) | |||
There was a problem hiding this comment.
Suggested change
| int backward, int extra) | |
| int backward, int has_extra) |
to indicate it's a bool and not the number of extra
| if (rank == root && ucc_knomial_pattern_loop_first_iteration(p) && !UCC_IS_INPLACE(*args)) { | ||
| ucc_kn_g_pattern_peer_seg(vrank, p, &peer_seg_count, | ||
| &peer_seg_offset); | ||
| status = ucc_mc_memcpy( |
There was a problem hiding this comment.
is error handled here?
| } | ||
| if (ucc_unlikely(UCC_OK != status)) { | ||
| task->super.status = status; | ||
| return; |
There was a problem hiding this comment.
just checking: do we need to free ucp task here?
| The extension adds the support for arbitrary radix. | ||
| 3. The algorithm targets Large message sizes (ie. optimized for max bandwidth). | ||
| 4. If number of ranks in the team can not form a full radix subtree | ||
| (for radix=2 this means the team size is not power of 2) then there will be |
There was a problem hiding this comment.
Suggested change
| (for radix=2 this means the team size is not power of 2) then there will be | |
| (this means the team size is not a power of the radix) then there will be |
| However, if they are used together as part of SRG reduce one has to | ||
| provide the same radix for both routines. | ||
| 6. After the completion of reduce-scatter phase the local result (at non EXTRA | ||
| ranks) will be located in dst buffer at offset the can be commputed by the |
There was a problem hiding this comment.
Suggested change
| ranks) will be located in dst buffer at offset the can be commputed by the | |
| ranks) will be located in the dst buffer at an offset that can be computed by the |
| UCC_CHECK_GOTO(ucc_tl_ucp_reduce_scatter_knomial_init_r(&args, team, &task, | ||
| radix), | ||
| out, status); | ||
| UCC_CHECK_GOTO(ucc_schedule_add_task(schedule, task), out, status); |
There was a problem hiding this comment.
I think there is memory leak here in case of error (e.g. the scratch buffer, or the schedule handle)
| args.args.dst.info.buffer = rs_rbuf; | ||
| args.args.mask |= UCC_COLL_ARGS_FIELD_FLAGS; | ||
| args.args.flags |= UCC_COLL_ARGS_FLAG_IN_PLACE; | ||
|
|
|
|
||
| UCC_CHECK_GOTO(ucc_tl_ucp_gather_knomial_init_r(&args, team, &task, radix), | ||
| out, status); | ||
| UCC_CHECK_GOTO(ucc_schedule_add_task(schedule, task), out, status); |
There was a problem hiding this comment.
is there mem leak here as well ?
| &n_frags, &pipeline_depth); | ||
| if (n_frags > 1) { | ||
| bargs.mask |= UCC_BASE_CARGS_MAX_FRAG_COUNT; | ||
| bargs.max_frag_count = ucc_buffer_block_count(max_frag_count, n_frags, 0); |
There was a problem hiding this comment.
align the "="? (I'm not sure of the convention)
| @@ -214,6 +216,7 @@ void ucc_tl_ucp_reduce_scatter_knomial_progress(ucc_coll_task_t *coll_task) | |||
| size_t data_size = count * dt_size; | |||
| ucc_rank_t rank = task->subset.myrank; | |||
| ucc_rank_t size = task->subset.map.ep_num; | |||
| ucc_rank_t root = 0; | |||
There was a problem hiding this comment.
nit: does it have a real meaning to set root at 0 here and below ?
|
Should we update the tests with the new algo(s) ? |
|
Reference PR, no reviewes needed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Adding scatter reruce gather algorithm for reduce collective in TL UCP.
Why ?
Improves performance of medium and large message size reduce
How ?
Implementation is similar to SRA allreduce, but the last operation is gather instead of allgather