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

TL/UCP: add reduce srg knomial #1058

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sergei-Lebedev
Copy link
Contributor

What

Add reduce srg knomial algorithm to TL UCP

Why ?

Improves performance of large msg reduce collective

How ?

Implemented as schedule of reduce scatter knomial followed by gather knomial

reduce/reduce.c \
reduce/reduce_knomial.c \
reduce/reduce_dbt.c \
reduce/reduce_srg_knomial.c
Copy link
Contributor Author

Choose a reason for hiding this comment

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

alignment

@@ -499,6 +513,9 @@ ucc_tl_ucp_reduce_scatter_knomial_init_r(ucc_base_coll_args_t *coll_args,
ucc_kn_rsx_pattern_init(size, rank, radix,
count, &task->reduce_scatter_kn.p);

} else if (ct == UCC_COLL_TYPE_REDUCE) {
ucc_kn_rsx_pattern_init(size, VRANK(rank, coll_args->args.root, size),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add reordering for reduce

UCC_TL_TEAM_SIZE(tl_team),
count);

/* 1st step of reduce: knomial reduce_scatter */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

move comment up

cfg_radix = ucc_tl_ucp_get_radix_from_range(tl_team,
count * ucc_dt_size(dt),
mt, p, 4);
radix = ucc_knomial_pattern_get_min_radix(cfg_radix,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add tl_trace

args.args.dst.info.buffer = rs_rbuf;
args.args.mask |= UCC_COLL_ARGS_FIELD_FLAGS;
args.args.flags |= UCC_COLL_ARGS_FLAG_IN_PLACE;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rem empty line

targs->dst.info.count = frag_count;

targs = &frag->tasks[1]->bargs.args; /* GATHER */
targs->src.info.buffer = PTR_OFFSET(targs->src.info.buffer, offset * dt_size);;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove ;;

}

if (!UCC_IS_ROOT(*args, trank) || UCC_IS_INPLACE(*args)) {
st = ucc_mc_alloc(&schedule->scratch_mc_header, count * dt_size, mt);
Copy link
Collaborator

@MamziB MamziB Jan 30, 2025

Choose a reason for hiding this comment

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

Can you avoid the scratch buffer allocation? For example, in the first call of this collective algorithm, allocate a large enough buffer for this team, and reuse this scratch buffer for the future calls into this collective.

root = args->root;
rank = VRANK(rank, root, size);
}

UCC_KN_REDUCE_GOTO_PHASE(task->reduce_scatter_kn.phase);
block_count = ucc_sra_kn_compute_block_count(count, rank, p);
get_rs_work_buf(task, block_count, &wb);
if (KN_NODE_EXTRA == node_type) {
peer = ucc_ep_map_eval(task->subset.map,
Copy link
Collaborator

@MamziB MamziB Jan 30, 2025

Choose a reason for hiding this comment

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

I see that in many places we are trying to find out the actual peer. Can you add a new function like knomial_get_actual_peer() that unifies all these VRANK/INV_VRANK/ucc_ep_map_eval and use this function instead?

@MamziB
Copy link
Collaborator

MamziB commented Jan 30, 2025

@Sergei-Lebedev very solid PR, Thanks for creating this new SRG reduce algorithm. Please see my comments.

Copy link
Collaborator

@samnordmann samnordmann left a comment

Choose a reason for hiding this comment

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

I reviewed #1035 directly

Copy link
Collaborator

@janjust janjust left a comment

Choose a reason for hiding this comment

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

I let minor comments here and there - overall it's great, just stylist things

@@ -1,5 +1,5 @@
/**
* Copyright (c) 2021-2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
* Copyright (c) 2021-2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

2025

@@ -1,5 +1,5 @@
/**
* Copyright (c) 2021-2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
* Copyright (c) 2021-2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

2025

size_t frag_count;
size_t offset;
ucc_coll_args_t *targs;
ucc_rank_t rank;
Copy link
Collaborator

Choose a reason for hiding this comment

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

variable alignment

team = TASK_TEAM(&schedule_p->super);
rank = UCC_TL_TEAM_RANK(team);
if (UCC_IS_ROOT(*args, rank)) {
count = args->dst.info.count;
Copy link
Collaborator

Choose a reason for hiding this comment

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

assignment alignmnet

dt_size = ucc_dt_size(args->dst.info.datatype);
} else {
count = args->src.info.count;
dt_size = ucc_dt_size(args->src.info.datatype);
Copy link
Collaborator

Choose a reason for hiding this comment

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

assignment alignment

/* 1st step of reduce: knomial reduce_scatter */
UCC_CHECK_GOTO(ucc_tl_ucp_reduce_scatter_knomial_init_r(&args, team, &task,
radix),
out, status);
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if it's just me, but looks like out, status); have a weird alignment, or should be no newline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants