TL/UCP: add exact mixed-radix allgather schedules by jeffnvidia · Pull Request #1327 · openucx/ucc

jeffnvidia · 2026-07-12T14:13:09Z

What

Add exact per-phase radix schedules to TL/UCP K-nomial AllGather. A schedule
such as 8x6 uses radix 8 in the first phase and radix 6 in the second; its
product must equal the team size.

Set an explicit schedule with, for example:

UCC_TL_UCP_ALLGATHER_KN_RADIX=8x6

The same option accepts a fixed radix or auto. The existing fixed-radix path
and AllGatherV behavior remain unchanged. Invalid schedules are rejected, and
existing algorithm tuning still decides whether K-nomial AllGather is selected.

Why

For non-power team sizes, a fixed radix can produce a truncated final phase or
proxy/extra ranks. An exact mixed schedule covers the complete team with an
explicit radix for every phase.

Across three tested environments at 96 ranks, a representative exact schedule
improved the large-message endpoint by 1.81x--1.99x over the best measured
fixed radix by avoiding the fixed-radix remainder path.

Performance evidence

Results are intentionally anonymized and normalized to the best measured
fixed-radix execution for each environment. Each row uses one exact schedule
at the endpoint; it is not a pointwise best-schedule envelope.

Environment	Team	Exact schedule	Fixed baseline	Normalized speedup
System A	96 ranks	`4x4x6`	R4	1.98x
System B	96 ranks	`2x2x2x2x2x3`	R4	1.99x
System C	96 ranks	`8x3x4`	R3	1.81x

These relative results demonstrate the remainder-path mechanism; they are not
intended as absolute system-performance comparisons.

Validation

Capability debug build and focused schedule tests passed.
16-rank CUDA AllGather covered fixed R2/R4/R8 and explicit 8x2, 2x8,
and 4x4; all cases passed with zero failures.
Tests cover parsing and bounded formatting, legacy fixed-radix behavior, and
exact mixed peer sets, segment sizes, and offsets for every rank.

Automatic schedule selection is proposed separately in #1328.

jeffnvidia · 2026-07-15T12:55:25Z

Maintainer note on the tl_ucp.c diff and codestyle:

I intentionally reduced this file to the functional seven-line
ALLGATHER_KN_MIXED_RADICES entry and removed the unrelated formatting churn.
With clang-format 21.1.8, touching that entry expands formatting to the entire
legacy configuration-array initializer, changing the file from 7 additions / 0
deletions to 105 additions / 49 deletions. I did not add formatter-suppression
directives.

The minimal diff may therefore make the codestyle check fail. Could maintainers
please accept/waive that result, or advise if a separate formatting-only change
is preferred?

Add explicit per-phase radix schedules to TL/UCP K-nomial AllGather while preserving the fixed-radix and AllGatherV paths. Parse and validate exact schedules through the existing team configuration.

Cover schedule parsing, legacy fixed-radix behavior, and exact mixed-radix peer and segment layouts.

greptile-apps · 2026-08-17T06:02:54Z

Greptile Summary

The PR adds exact mixed-radix schedules to TL/UCP K-nomial AllGather while retaining the fixed-radix and automatic-selection paths.

Adds shared mixed-radix sequence and pattern support.
Adds a ranged configuration parser with copy, formatting, lookup, and release operations.
Integrates configured schedules into TL/UCP AllGather initialization and progress.
Adds parser, layout, validation, cloning, and formatting tests.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/coll_patterns/knomial.h	Introduces the radix-sequence representation, indexed access, and exact-product validation.
src/coll_patterns/recursive_knomial.h	Persists the initialization sequence and advances the active radix between mixed-radix phases.
src/coll_patterns/sra_knomial.h	Extends AllGather pattern initialization to exact mixed-radix schedules while preserving fixed-radix initialization.
src/components/tl/ucp/allgather/allgather_knomial.c	Selects configured schedules and uses each phase’s active radix during AllGather communication.
src/components/tl/ucp/tl_ucp.c	Changes ALLGATHER_KN_RADIX to the new sequence-capable configuration type.
src/utils/ucc_parser.c	Implements parsing, cloning, lookup storage, formatting, and destruction for ranged radix schedules.
src/utils/ucc_parser.h	Defines the sequence-aware ranged configuration structures and parser type.
test/gtest/coll/test_knomial_seq.cc	Covers fixed-radix compatibility and exact mixed-radix peer and segment layouts.
test/gtest/utils/test_parser.cc	Covers valid and invalid schedules, range selection, formatting bounds, and deep cloning.

_{Reviews (6): Last reviewed commit: "TL/UCP: unify radix sequence state" | Re-trigger Greptile}

Sergei-Lebedev · 2026-08-17T09:14:22Z

/build

Sergei-Lebedev · 2026-08-20T13:05:53Z

     UCC_CONFIG_TYPE_UINT_RANGED},

+    {"ALLGATHER_KN_MIXED_RADICES", "",
+     "Optional exact mixed-radix schedule for knomial allgather, for example "


i would maybe change semantic here, instead of exact decompostion consider it as allowed factors in decomposition so it generalises to any team size in a run.

don't we want the option for the user to provide the exact order ? (between 8x6 and 6x8 for example) I'm thinking that maybe the user would for example want to start with the first radix being the number of ranks in one node.

In the second PR, I provide an auto tuner that selects the radices and the order anyways but if the user chooses to select, I was thinking we ggive him "full control"

Agree, but at the same time if we consider this config as exact decomposition then it is less useful. If context creates multiple teams of different size then most of them will skip this value. Maybe extend with extra "=", for instance
RADIX="2x3x4" tells UCC to decompose using provided values
RADIX="=2x3x4" use exact decomposition and works for team size 24 only

I kept the x-separated value as an exact ordered sequence. We already have auto for team-size-independent selection and the explicit sequence for users who need exact ordering. To handle contexts containing teams of different sizes, a sequence-product mismatch now falls back to auto instead of failing. This addresses the multi-team concern without introducing a third allowed-factors mode or additional = syntax.

svcnbu-swx-hpcx · 2026-08-20T16:03:26Z

🤖 CI Triage Agent — Linter · commit d17161bf

TL;DR: The Linter (clang-tidy) job failed because allgather_knomial.c:250 contains a dead store — radix = p->radix; is assigned but never read in the ALLGATHER branch — which clang-tidy's clang-analyzer-deadcode.DeadStores check flags as an error (exit 125). Remove the dead assignment.

Full analysis

Summary: clang-tidy failed with 1 warning-treated-as-error: "Value stored to 'radix' is never read" in allgather_knomial.c.

Root cause: In ucc_tl_ucp_allgather_knomial_start, the ALLGATHER branch reassigns radix = p->radix; (line 250) after ucc_kn_ag_pattern_init, but nothing in that branch subsequently reads radix. radix is only consumed in the ALLGATHERV/else branches, which use the value set at initialization (line 234) and don't depend on line 250. This makes line 250 a dead store, which clang-analyzer-deadcode.DeadStores treats as an error under -warnings-as-errors, so the run-clang-tidy step counts an error: and exits 125.

Implicated commit: d17161b — "TL/UCP: address mixed-radix review" by jeffnvidia (the exact commit under test); the mixed-radix pattern work in ec51644 is the origin of the pattern-init reshuffle.

File: src/components/tl/ucp/allgather/allgather_knomial.c:250

Suggested fix: Delete the dead assignment radix = p->radix; on line 250. If the intent was for later code in the ALLGATHER branch to use the pattern-selected radix, then actually use it (e.g. pass it where needed); otherwise removing the line clears the clang-tidy error. If retaining it for symmetry/readability is desired, annotate with // NOLINT(clang-analyzer-deadcode.DeadStores), but simple removal is cleanest.

Related: PR #1327 (this change); prior mixed-radix PRs touching this file (#1176, #1072). No existing issue found for this specific warning.

svcnbu-swx-hpcx · 2026-08-20T16:07:41Z

🤖 CI Triage Agent — Lint (ROCm) · commit d17161bf

TL;DR: The Lint (ROCm) job failed because clang-tidy (run with -warnings-as-errors) flagged a dead store at allgather_knomial.c:250 — radix = p->radix; is assigned but never read afterward. Remove that unused assignment (or use the variable) to fix the lint error.

Full analysis

Summary: clang-tidy clang-analyzer-deadcode.DeadStores error caused the Lint job to exit with code 125 (the build itself succeeded).

Root cause: In ucc_tl_ucp_allgather_knomial_start, the UCC_COLL_TYPE_ALLGATHER branch assigns radix = p->radix; at line 250, but radix is not read again in that branch (only offset/rbuf are used, then the branch ends). clang-analyzer reports "Value stored to 'radix' is never read," and because the CI step counts any error: line and exits 125, the lint gate fails. This dead store was introduced by the new mixed-radix allgather logic, where ucc_kn_ag_pattern_init computes the effective radix into p->radix.

Implicated commit: ec516448 (jeffnvidia, "TL/UCP: add exact mixed-radix allgather"), on the current head d17161bf (jeffnvidia, "TL/UCP: address mixed-radix review").

File: src/components/tl/ucp/allgather/allgather_knomial.c:250

Suggested fix: Remove the now-unused radix = p->radix; line in the ALLGATHER branch. If the intent was to make the effective (possibly clamped) radix visible for later use, then it must actually be used — but since the subsequent code in that branch doesn't reference radix, the clean fix is to delete line 250. If radix is unused after ucc_kn_ag_pattern_init in all branches, also confirm the local isn't left otherwise unused.

Related: PR #1327 (this PR, jmahou/ucc-mixed-radix-clean). No prior matching issue found.

svcnbu-swx-hpcx · 2026-08-20T16:08:08Z

🤖 CI Triage Agent — Linter-NVIDIA · commit d17161bf

TL;DR: The Linter-NVIDIA clang-tidy job failed because allgather_knomial.c:250 stores p->radix into the local radix variable that is never read again, tripping clang-analyzer-deadcode.DeadStores (treated as error). Remove the dead radix = p->radix; assignment.

Full analysis

Summary: clang-tidy-17 static analysis failed with 1 warning treated as error: dead store to radix in allgather_knomial.c, causing the job to exit 125.

Root cause: In ucc_tl_ucp_allgather_knomial_start(), the UCC_COLL_TYPE_ALLGATHER branch calls ucc_kn_ag_pattern_init(...) and then executes radix = p->radix; (line 250). The local radix is not used anywhere after this assignment in that branch (the code uses p members, offset, and rbuf), so the store is dead. clang-analyzer flags it as clang-analyzer-deadcode.DeadStores, and the linter workflow treats warnings as errors (1 warning treated as error), producing exit code 125. This was introduced by the mixed-radix refactor on this branch, where the pattern-init API changed and left the reassignment vestigial.

Implicated commit: d17161b — jeffnvidia, "TL/UCP: address mixed-radix review" (HEAD of PR #1327; the dead-store pattern originates in the mixed-radix series starting ec51644).

File: src/components/tl/ucp/allgather/allgather_knomial.c:250

Suggested fix: Delete the dead assignment on line 250 (radix = p->radix;). Since radix is already initialized at line 234 from task->allgather_kn.p.radix and is only read in the ALLGATHERV/AGX branches (lines 271, 296, 300), removing line 250 clears the warning without changing behavior. If the intent was to refresh radix after ucc_kn_ag_pattern_init mutates p->radix, verify whether any later ALLGATHER-branch code actually needs it — currently none does — otherwise the removal is correct.

Related: PR #1327 (jmahou/ucc-mixed-radix-clean); prior mixed-radix commit ec51644.

Sergei-Lebedev · 2026-08-31T11:47:22Z

+    if (nradices == 1) {
+        ucc_knomial_pattern_init(size, rank, radices[0], p);
+        p->type         = KN_PATTERN_ALLGATHER;
+        p->count        = count;
+        p->block_size   = p->radix_pow * p->radix;
+        p->block_offset = ucc_knomial_pattern_loop_rank(p, rank) /
+                          p->block_size * p->block_size;
+        return;
+    }
+
+    p->radix        = radices[0];
    p->type         = KN_PATTERN_ALLGATHER;
+    p->iteration    = 0;
+    p->n_iters      = nradices;
+    p->node_type    = KN_NODE_BASE;
+    p->backward     = 0;
+    p->radix_pow    = 1;
+    p->full_pow_size = size;
+    p->size         = size;
+    p->rank         = rank;
+    p->n_extra      = 0;
    p->count        = count;
-    p->block_size   = p->radix_pow * radix;
-    p->block_offset = ucc_knomial_pattern_loop_rank(p, rank) / p->block_size *
-                      p->block_size;
+    p->block_size   = p->radix;
+    p->block_offset = ucc_knomial_pattern_loop_rank(p, rank) /
+                      p->block_size * p->block_size;
+    p->radices      = radices;


this can be simplified, most of the field here relevant for nradices=1 and nradices>1

Done. Fixed and exact sequences now both use ucc_knomial_pattern_init(). The exact case only overrides its specific invariants, and the common AllGather fields are initialized once.

Sergei-Lebedev · 2026-08-31T11:47:31Z

    ucc_rank_t     block_size;
    ptrdiff_t      block_offset;
    int            is64;
+    const ucc_kn_radix_t *radices;


Sergei-Lebedev · 2026-08-31T11:52:28Z

+typedef uint16_t ucc_kn_radix_t;
+#define UCC_KN_MAX_RADIX_PHASES (sizeof(ucc_rank_t) * CHAR_BIT - 1)
+
+typedef struct ucc_kn_radix_schedule {


this strucutre belongs to knomial pattern. Also schedule is not a good name, it mixes with ucc_schedule_t . Maybe ucc_kn_radix_seq?

Done. The radix type and sequence now live in coll_patterns/knomial.h, and I renamed the type to ucc_kn_radix_seq_t to avoid confusion with ucc_schedule_t.

Sergei-Lebedev · 2026-08-31T11:53:49Z

    ucc_rank_t     block_size;
    ptrdiff_t      block_offset;
    int            is64;
+    const ucc_kn_radix_t *radices;


why not to use ucc_kn_radix_schedule?

Done. The pattern now keeps a pointer to the complete ucc_kn_radix_seq_t, rather than only the raw radix array.

Sergei-Lebedev · 2026-08-31T11:55:11Z

+        task->allgather_kn.p.n_iters = schedule->n_radices;
+    } else {
+        task->allgather_kn.p.radices = NULL;
+        task->allgather_kn.p.n_iters = 0;


why need to set n_iters here? It should be part of init or reset

Done. Collective initialization now only stores the sequence pointer. n_iters is set by ucc_kn_ag_pattern_init() whenever the pattern is initialized or restarted.

Keep the exact radix sequence with the K-nomial pattern, initialize fixed and exact patterns through the common path, and fall back to automatic radix selection when the sequence product does not match the team size. Extend focused tests to cover exact-pattern invariants and restart.

Sergei-Lebedev · 2026-09-01T05:48:08Z

+                       ucc_kn_radix_t radix,
+                       const ucc_kn_radix_seq_t *radix_seq, size_t count,


let's pass only radix_seq here as argument. No reason to keep both while single value radix can be represented as radix_seq of len 1

Done. ucc_kn_ag_pattern_init() now takes only radix_seq and derives the initial radix from it. A fixed radix is represented as a sequence of length one.

Sergei-Lebedev · 2026-09-01T05:57:17Z

+ucc_status_t ucc_tl_ucp_allgather_knomial_init_r(
+    ucc_base_coll_args_t *coll_args, ucc_base_team_t *team,
+    ucc_coll_task_t **task_h, ucc_kn_radix_t radix)
+{
+    return ucc_tl_ucp_allgather_knomial_init_common(
+        coll_args, team, task_h, radix, NULL);
+}


why do we need init_common if it just same as init_r?

Removed init_common. The sequence initializer is now the private implementation. The existing scalar init_r entry point is retained as a thin compatibility wrapper and delegates through a one-element sequence.

Sergei-Lebedev · 2026-09-01T05:58:50Z

+    const ucc_kn_radix_seq_t   *radix_seq = p->radix_seq;
+    ucc_kn_radix_t              radix = radix_seq ? radix_seq->radices[0] :
+                                                   p->radix;


Sergei-Lebedev · 2026-09-01T06:04:05Z

        task->subset.map    = sbgp->map;
    }
    task->allgather_kn.p.radix = radix;
+    task->allgather_kn.p.radix_seq =


keep only radix_seq, single radix is radix_seq of size 1

Done. The task now stores only the persistent radix_seq. Fixed radix is stored as a one-element sequence. p->radix is initialized by the pattern and is used only as the active radix for the current phase.

Sergei-Lebedev · 2026-09-01T06:06:32Z

+            coll_args, team, task_h, radix);
+    }
+    if (!ucc_tl_ucp_allgather_knomial_seq_matches(radix_seq, tsize)) {
+        tl_debug(UCC_TL_TEAM_LIB(tl_team),


rework code duplication

Done. Initialization now constructs the auto sequence once and only overrides it for a valid configured fixed or exact sequence. An invalid exact sequence simply leaves the auto value in place. Sequence validation is centralized.

Sergei-Lebedev · 2026-09-01T06:08:03Z

+        return ucc_tl_ucp_allgather_knomial_init_r(
+            coll_args, team, task_h, radix);
+    }
+    if (!ucc_tl_ucp_allgather_knomial_seq_matches(radix_seq, tsize)) {


is it enough? will it work if we have a sequence like this [0]?

Handled. The parser rejects radix zero, and validation independently rejects a constructed one-element zero sequence, zero inside a mixed sequence, missing storage, overflow, and product mismatch. These cases are covered by the new validation test.

Sergei-Lebedev · 2026-09-01T06:09:09Z

    ucc_rank_t     block_size;
    ptrdiff_t      block_offset;
    int            is64;
+    const ucc_kn_radix_seq_t *radix_seq;


Sergei-Lebedev · 2026-09-01T06:10:05Z

+
+    ASSERT_TRUE(ucc_config_sscanf_kn_radix(
+        "0-4K:host:8x6,4K-inf:cuda:4,auto", &p, NULL));
+    expect_schedule(


need to rename after changing kn_schedule > kn_seq. Here and other places

Done. The helper, test names, test file, and configuration descriptions now consistently use “sequence” rather than “schedule”.

Sergei-Lebedev · 2026-09-01T06:11:32Z

 gtest_LDADD    += $(UCX_LIBS) $(UCX_LIBADD)
 endif

+if TL_UCP_ENABLED


why test knomial_schedule depends on TL UCP?

It no longer depends on TL/UCP. The test was renamed to test_knomial_seq.cc, uses only the generic K-nomial pattern code, and is now included unconditionally in the gtest sources.

jeffnvidia mentioned this pull request Jul 12, 2026

TL/UCP: select exact allgather radix schedules #1328

Open

jeffnvidia force-pushed the jmahou/ucc-mixed-radix-clean branch 3 times, most recently from 2dab095 to 828683a Compare July 15, 2026 09:07

jeffnvidia marked this pull request as ready for review July 15, 2026 09:31

jeffnvidia force-pushed the jmahou/ucc-mixed-radix-clean branch from 367c6f9 to 828683a Compare July 15, 2026 12:48

jeffnvidia force-pushed the jmahou/ucc-mixed-radix-clean branch 4 times, most recently from da58ec5 to d737a5a Compare July 19, 2026 14:26

jeffnvidia added 2 commits July 19, 2026 20:20

TL/UCP: add exact mixed-radix allgather

ec51644

Add explicit per-phase radix schedules to TL/UCP K-nomial AllGather while preserving the fixed-radix and AllGatherV paths. Parse and validate exact schedules through the existing team configuration.

TEST: cover mixed-radix allgather schedules

94d3bd9

Cover schedule parsing, legacy fixed-radix behavior, and exact mixed-radix peer and segment layouts.

jeffnvidia force-pushed the jmahou/ucc-mixed-radix-clean branch from d737a5a to 94d3bd9 Compare July 19, 2026 17:24

Sergei-Lebedev added the Ready-for-Review label Aug 17, 2026

Sergei-Lebedev reviewed Aug 20, 2026

View reviewed changes

TL/UCP: address mixed-radix review

d17161b

jeffnvidia added 2 commits August 20, 2026 19:11

TL/UCP: remove dead radix assignment

155b53b

TL/UCP: address follow-up mixed-radix review

8e47d8d

Sergei-Lebedev reviewed Aug 31, 2026

View reviewed changes

Sergei-Lebedev reviewed Sep 1, 2026

View reviewed changes

TL/UCP: unify radix sequence state

1f41ba6

		ucc_kn_radix_t radix,
		const ucc_kn_radix_seq_t *radix_seq, size_t count,

Uh oh!

Conversation

jeffnvidia commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Performance evidence

Validation

Uh oh!

jeffnvidia commented Jul 15, 2026

Uh oh!

greptile-apps Bot commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

Sergei-Lebedev commented Aug 17, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffnvidia Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svcnbu-swx-hpcx commented Aug 20, 2026

Uh oh!

svcnbu-swx-hpcx commented Aug 20, 2026

Uh oh!

svcnbu-swx-hpcx commented Aug 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffnvidia Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffnvidia Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffnvidia Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffnvidia Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffnvidia commented Jul 12, 2026 •

edited

Loading

greptile-apps Bot commented Aug 17, 2026 •

edited

Loading

jeffnvidia Aug 31, 2026 •

edited

Loading

jeffnvidia Aug 31, 2026 •

edited

Loading

jeffnvidia Aug 31, 2026 •

edited

Loading

jeffnvidia Aug 31, 2026 •

edited

Loading

jeffnvidia Aug 31, 2026 •

edited

Loading