feat(collectives): ring allreduce TPUT push model on HOST builtin + InCore composite - #2280
Conversation
📝 WalkthroughWalkthroughChangesRing allreduce TPUT conversion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Rank
participant NeighborBarrier
participant RightNeighbor
Rank->>Rank: Read own receive slot
Rank->>NeighborBarrier: Wait for ready generation
Rank->>RightNeighbor: TPUT partial chunk
Rank->>NeighborBarrier: Wait for push-done generation
Rank->>Rank: Reduce and store partial
Rank->>RightNeighbor: TPUT finalized chunk
Rank->>NeighborBarrier: Wait for allgather completion
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/ir/transforms/lower_composite_ops_pass.cpp (1)
1533-1537: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the allgather ready-barrier rationale.
The comment states the ready barrier is safe because "the cumulative generation count includes the previous step's push-done notifies". Each round uses its own signal row (
ag_round), so rowk's counters start at zero and carry no credit from rowk-1. The actual guarantee comes from the previous round's push-done barrier, which every rank must pass before it enters roundk. Restate the rationale so a future change does not remove the previous round's push-done barrier on a false premise. The same wording appears at Lines 1571-1573.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ir/transforms/lower_composite_ops_pass.cpp` around lines 1533 - 1537, Update the ready-barrier rationale comment around lines 1533-1537 to remove the incorrect explanation about cumulative generation counts including previous push-done notifies. Replace it with the correct rationale: the guarantee comes from the previous round's push-done barrier that every rank must pass before entering round k. Apply the same correction to the identical wording at lines 1571-1573 to ensure consistency and prevent future maintainers from removing the previous round's push-done barrier based on the false premise.python/pypto/runtime/builtins/collectives/allreduce_ring/templates/kernel.cpp.in (1)
121-144: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd a comment documenting the two-rank
NeighborBarrierbehavior.For
nranks == 2, both neighbours map to rank 1, so each row cell receives twoAtomicAddwrites and is observed twice via the sameGE 1waits. A short comment keeps the two-rank safety invariant explicit; the existing row allocation path already uses a fresh zeroed signal buffer per allreduce call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/pypto/runtime/builtins/collectives/allreduce_ring/templates/kernel.cpp.in` around lines 121 - 144, Add a concise comment above NeighborBarrier documenting the nranks == 2 case: both neighbours resolve to rank 1, each signal cell receives two AtomicAdd notifications, and the corresponding GE 1 waits observe the shared cell twice. Keep the existing implementation unchanged and note that the signal buffer is freshly zeroed per allreduce call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@python/pypto/runtime/builtins/collectives/allreduce_ring/templates/kernel.cpp.in`:
- Around line 146-149: Update the stale StepBarrier comment near the barrier
configuration to reflect that kUseNeighborBarrier is already enabled and
NeighborBarrier has been verified on 910B2 for P=2 and P=4. Remove the outdated
instructions to land with RoundBarrier first and defer the swap; keep the
comment focused on the current implementation and its verification status.
In `@src/ir/transforms/lower_composite_ops_pass.cpp`:
- Around line 1439-1454: The tile.set_validshape operation returns a new
narrowed TileType and does not mutate put_stage in-place. Update the
pld.tile.put call to pass the narrowed alias rs_stage_valid instead of
put_stage, so the push uses the correct narrowed staging tile extent. Apply the
same fix to the allgather push path by using ag_stage_valid instead of put_stage
in its corresponding pld.tile.put call.
In `@tests/ut/ir/transforms/test_lower_composite_ops.py`:
- Around line 2102-2109: Add a non-empty assertion for the stage_creates
collection in the FP16 test, matching the sibling test’s assertion and message.
Keep the existing chunk_shapes loop and validation unchanged so the test
requires FP16 staging-tile creation before checking alignment and size
constraints.
---
Nitpick comments:
In
`@python/pypto/runtime/builtins/collectives/allreduce_ring/templates/kernel.cpp.in`:
- Around line 121-144: Add a concise comment above NeighborBarrier documenting
the nranks == 2 case: both neighbours resolve to rank 1, each signal cell
receives two AtomicAdd notifications, and the corresponding GE 1 waits observe
the shared cell twice. Keep the existing implementation unchanged and note that
the signal buffer is freshly zeroed per allreduce call.
In `@src/ir/transforms/lower_composite_ops_pass.cpp`:
- Around line 1533-1537: Update the ready-barrier rationale comment around lines
1533-1537 to remove the incorrect explanation about cumulative generation counts
including previous push-done notifies. Replace it with the correct rationale:
the guarantee comes from the previous round's push-done barrier that every rank
must pass before entering round k. Apply the same correction to the identical
wording at lines 1571-1573 to ensure consistency and prevent future maintainers
from removing the previous round's push-done barrier based on the false premise.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 73efeda0-1ee1-418f-ae6b-d8ff15d8b379
📒 Files selected for processing (7)
docs/en/dev/passes/12-lower_composite_ops.mddocs/en/dev/passes/40-lower_host_tensor_collectives.mddocs/zh/dev/passes/12-lower_composite_ops.mddocs/zh/dev/passes/40-lower_host_tensor_collectives.mdpython/pypto/runtime/builtins/collectives/allreduce_ring/templates/kernel.cpp.insrc/ir/transforms/lower_composite_ops_pass.cpptests/ut/ir/transforms/test_lower_composite_ops.py
5f1e8c2 to
83c4b26
Compare
758bdde to
0fa8e3d
Compare
0fa8e3d to
11b2885
Compare
… nranks==2 NeighborBarrier Address the two outstanding CodeRabbit nitpicks on the ring allreduce TPUT push PR (hw-native-sys#2280): * lower_composite_ops_pass.cpp: correct the allgather ready-barrier comment (both the Phase 2 header and the per-chunk in-loop wording). The previous text claimed the ready barrier's guarantee came from the cumulative generation count including the previous step's push-done notifies, but each round uses its own signal row (ag_round) and row k starts at zero. The real guarantee is that every rank must pass the previous round's push-done barrier before entering round k, so the round-k-1 push into the forwarded slot has landed and is visible. * allreduce_ring/kernel.cpp.in: document the nranks == 2 NeighborBarrier degenerate case — both neighbours resolve to the same peer, each signal cell receives two AtomicAdd notifies and is observed twice through the same GE 1 waits — safe because the signal buffer is freshly zeroed per call. Comment-only; no codegen change.
… nranks==2 NeighborBarrier Address the two outstanding CodeRabbit nitpicks on the ring allreduce TPUT push PR (hw-native-sys#2280): * lower_composite_ops_pass.cpp: correct the allgather ready-barrier comment (both the Phase 2 header and the per-chunk in-loop wording). The previous text claimed the ready barrier's guarantee came from the cumulative generation count including the previous step's push-done notifies, but each round uses its own signal row (ag_round) and row k starts at zero. The real guarantee is that every rank must pass the previous round's push-done barrier before entering round k, so the round-k-1 push into the forwarded slot has landed and is visible. * allreduce_ring/kernel.cpp.in: document the nranks == 2 NeighborBarrier degenerate case — both neighbours resolve to the same peer, each signal cell receives two AtomicAdd notifies and is observed twice through the same GE 1 waits — safe because the signal buffer is freshly zeroed per call. Comment-only; no codegen change.
e06c676 to
a92cd5b
Compare
… nranks==2 NeighborBarrier Address the two outstanding CodeRabbit nitpicks on the ring allreduce TPUT push PR (hw-native-sys#2280): * lower_composite_ops_pass.cpp: correct the allgather ready-barrier comment (both the Phase 2 header and the per-chunk in-loop wording). The previous text claimed the ready barrier's guarantee came from the cumulative generation count including the previous step's push-done notifies, but each round uses its own signal row (ag_round) and row k starts at zero. The real guarantee is that every rank must pass the previous round's push-done barrier before entering round k, so the round-k-1 push into the forwarded slot has landed and is visible. * allreduce_ring/kernel.cpp.in: document the nranks == 2 NeighborBarrier degenerate case — both neighbours resolve to the same peer, each signal cell receives two AtomicAdd notifies and is observed twice through the same GE 1 waits — safe because the signal buffer is freshly zeroed per call. Comment-only; no codegen change.
HOST builtin (builtin.tensor.allreduce_ring): - Convert reduce-scatter + allgather from TLOAD/TSTORE pull to pto::comm::TPUT push: TPUT<AtomicAdd> remote-accumulate for RS, non-atomic TPUT for AG. - Ordering: pipe_barrier(PIPE_ALL) around every TPUT + dsb(DSB_DDR) before TNOTIFY (mirrors the allgather/all_to_all host builtins; NOT a GM fence). - Enable O(1) NeighborBarrier (notify/wait left+right only) — NPU-safe because the TPUT write pipeline orders data ahead of the signal, which the pull model lacked. NPU-verified at P=2/4 (both RoundBarrier baseline and NeighborBarrier). InCore composite (LowerTensorRingAllReduceRule): - Replace pld.tile.remote_load pulls with pld.tile.put pushes (non-atomic TPUT + local reduce, preserving Sum/Max/Min/Prod). - Race-free protocol: own-value read before the ready barrier, push after it, push-done barrier, then local read+reduce+store; credits stay 2 per subchunk. - Requires PTOAS >= v0.55 (dynamic partition-view shapes for tput, issue hw-native-sys#1069). - Ragged/arbitrary lengths and FP16 preserved via balanced segments + valid shapes; stage tile narrowed with tile.set_validshape per transfer. Docs: ring builtin lowering notes (en/zh) updated to describe the push model.
… nranks==2 NeighborBarrier Address the two outstanding CodeRabbit nitpicks on the ring allreduce TPUT push PR (hw-native-sys#2280): * lower_composite_ops_pass.cpp: correct the allgather ready-barrier comment (both the Phase 2 header and the per-chunk in-loop wording). The previous text claimed the ready barrier's guarantee came from the cumulative generation count including the previous step's push-done notifies, but each round uses its own signal row (ag_round) and row k starts at zero. The real guarantee is that every rank must pass the previous round's push-done barrier before entering round k, so the round-k-1 push into the forwarded slot has landed and is visible. * allreduce_ring/kernel.cpp.in: document the nranks == 2 NeighborBarrier degenerate case — both neighbours resolve to the same peer, each signal cell receives two AtomicAdd notifies and is observed twice through the same GE 1 waits — safe because the signal buffer is freshly zeroed per call. Comment-only; no codegen change.
…r credits hw-native-sys#2279 (merged) added a self-clearing signal epilogue to the pull-model ring kernel: it reset every peer cell with TNOTIFY(-1) per round, matching the RoundBarrier's notify-all credit pattern. The TPUT push model replaces the barrier with the O(1) NeighborBarrier, whose credit pattern is different: * NeighborBarrier notifies only the two ring neighbours per round, so only the left/right cells of each used row carry a +1 (and for nranks == 2 both neighbours are the same peer, so that single cell carries +2 from two AtomicAdd notifies). * Resetting all P-1 cells (the hw-native-sys#2279 loop) would therefore corrupt the unused cells to -1 and, for nranks == 2, leave +1 stale credit in the one used cell — reintroducing the exact stale-credit barrier failure hw-native-sys#2279 fixed. The epilogue now branches on kUseNeighborBarrier: reset the two neighbour cells with TNOTIFY(-1) each (twice on the shared cell when nranks == 2, since left == right), and keep the all-peer reset for the RoundBarrier fallback. Verified against the hw-native-sys#2279 ring signal-reuse ST contract (one signal buffer reused across back-to-back calls).
…nal reuse The ring host builtin is now self-clearing (adapted epilogue): after the final barrier it restores every used barrier row to zero — the two neighbour cells per round for NeighborBarrier (twice on the shared cell when nranks == 2), all P-1 cells for the RoundBarrier fallback — so a single signal buffer can be reused across back-to-back calls like the other host builtins (hw-native-sys#2279). Adds the note to docs 42 in en + zh (parity).
a92cd5b to
19f3b7d
Compare
hw-native-sys#2542 inserted pass 14-block_nz_tensor_views, shifting the pass docs by one: derive_call_directions moved 37 -> 38. 08-param-directions.md (en+zh) still linked to the old 37- number, which broke the strict mkdocs build ('Doc file contains a link ... but the target is not found') on main and on any PR rebased onto it. Update both links to 38-. Docs-only; en/zh parity preserved.
Summary
Replaces the pull-model engine of the ring allreduce on both rails with a TPUT push model (remote write), enabling O(1)
NeighborBarrieron the HOST builtin and eliminating the pull-model NPU memory-ordering gap.builtin.tensor.allreduce_ring): reduce-scatter + allgather converted fromTLOAD/TSTOREpull topto::comm::TPUTpush —TPUT<AtomicAdd>remote-accumulate for RS, non-atomicTPUTfor AG. Ordering ispipe_barrier(PIPE_ALL)around every transfer +dsb(DSB_DDR)beforeTNOTIFY(mirrors the in-tree allgather/all_to_all host builtins; not a GM fence). The O(P²)RoundBarrieris replaced by the O(1)NeighborBarrier(notify/wait the two ring neighbours only), which is NPU-safe because the TPUT write pipeline orders the data ahead of the signal — the pull model could not provide that.LowerTensorRingAllReduceRule): replacespld.tile.remote_loadpulls withpld.tile.putpushes (non-atomic TPUT + local reduce, preserving Sum/Max/Min/Prod). Race-free per-subchunk protocol: own-value read → ready barrier → push to right neighbour → push-done barrier → local read+reduce+store; barrier credits stay 2 per subchunk (signal shape unchanged). Ragged/arbitrary lengths and FP16 are preserved via balanced segments + valid shapes, with the shared VEC staging tile narrowed per transfer viatile.set_validshape.Requires PTOAS >= v0.55 (pypto pins v0.57)
This PR depends on PTOAS v0.55 (release: hw-native-sys/PTOAS#1069, fixed in PR #1079).
The InCore composite's
pld.tile.puttransfers carry the exact raggedvalid_colsas the partition-view extent. PTOAS ≤ v0.54 rejects dynamic partition-view shapes forpto.comm.tput('pto.comm.tput' op expects dst to have a positive static shape), so the pure push model cannot compile below v0.55. The HOST builtin does not depend on this (its kernel is hand-written), but the composite rail does.The requirement is satisfied by the current pin: pypto now pins PTOAS v0.57 (via #2291). The PR is rebased onto current
main(2026-08-26, was 76 commits behind; re-rebased twice 2026-08-27 — first onto the #2530 runtime bump adopting theChipTensor→TaskTensorkernel rename, then onto #2542 adopting the42-lower_host_tensor_collectives→43-…docs rename, with the PR's ring-doc edits re-homed) and merges cleanly. The UT tests pin the push structure (pld.tile.put+tile.createstaging tile instead ofpld.tile.remote_load).Rebased — merge-order with #2279 (self-clearing signals) resolved
The rebase picked up #2279's self-clearing signal epilogue, which was written for the pull-model
RoundBarrier(reset every peer's cell withTNOTIFY(-1)per round). That credit pattern does not match the push model'sNeighborBarrier:NeighborBarriercredits only the two ring neighbours per round — a single cell whennranks == 2, where both neighbours are the same peer and the cell carries two +1s.nranks == 2, leave +1 stale credit in the one used cell — reintroducing the exact stale-credit barrier failure feat(distributed): self-clearing barrier signals in host collective kernels #2279 fixed.The epilogue now branches on
kUseNeighborBarrier: it restores only the two neighbour cells per used row withTNOTIFY(-1, AtomicAdd)(twice on the shared cell whennranks == 2), keeping the all-peer reset for theRoundBarrierfallback. The ring builtin is therefore self-clearing and signal-reuse-safe across back-to-back calls, matching the other host builtins (#2279). The ring signal-reuse ST (test_l3_host_tensor_allreduce_ring.pyreuse leg) is the NPU gate for the adapted epilogue.Issues this PR addresses
ColMaskInternal/tile.set_validshape) to the exact (possibly ragged) transfer extent, so partial tails transfer exactly and never over-read/overwrite adjacent slots.>= v0.55dependency it describes is satisfied by the v0.57 pin.Verification (NPU silicon, 910B2, PTOAS v0.55)
All on real NPUs (8x 910B2), P=2 and P=4:
tests/st/distributed/test_l3_host_tensor_allreduce_ring.py— HOST ring, P=2/4 ✅ (withNeighborBarrierenabled)tests/st/distributed/collectives/test_l3_tensor_allreduce_ring_intrinsic.py— InCore ring, P=2/4, sizes {1, 17, 4097, 65537} (ragged + >UB), Sum/Max/Min/Prod, FP16 ✅tests/st/distributed/collectives/test_l3_allreduce_ring.py+test_l3_ring_sizing_prewarm.py— no regression ✅test_lower_host_tensor_collectives.py,test_host_orch_distributed.py,test_lower_composite_ops.py(+ numerical) all green ✅Total: 25/25 ST + 190 UT passed (pre-rebase). The 2026-08-26 rebase + epilogue adaptation re-ran the ring UTs (173/174, the one failure is a pre-existing parser
TileView(pad=…)roundtrip gap on main, unrelated to this PR); the 2026-08-27 re-rebases (onto the #2530 runtime bump and the #2542 docs rename) each re-ran the same 173/174. NPU ST should be re-confirmed for the signal-reuse leg.Trade-off note (ReduceOp)
The HOST builtin is
ReduceOp::kSumonly by construction, so itsTPUT<AtomicAdd>RS is fine. The composite keeps non-atomic push + local reduce to preserve Sum/Max/Min/Prod; only a remote-atomicTPUT<AtomicAdd>variant would be Sum-only (AtomicTypehas noAtomicMax/Min).Follow-ups (not in this PR)
pld.tensor.allreducerestriction via shared-signal synthesis (the other half of the feat(distributed): self-clearing barrier signals in host collective kernels #2279 review).Review notes
nranks == 2NeighborBarrierbehaviour is documented.