Conversation
Removes xfail markers for cross-core Tpop tests on a5sim now that the underlying 950 backend issue is fixed in pto-isa d779cd0.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request removes several pytest.xfail markers for the "a5sim" platform across multiple cross-core runtime tests, including C2V no-split, bidirectional, and multi-pipe scenarios. This indicates that these tests are now expected to pass on the simulator. I have no feedback to provide.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughUpdate GitHub Actions CI workflows to pin ChangesPTO-ISA Commit Pin and Cross-Core Test Enablement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
YunjiQin
marked this pull request as draft
May 19, 2026 06:48
georgebisbas
added a commit
to georgebisbas/pypto
that referenced
this pull request
Jul 20, 2026
- Replace O(P^2) RoundBarrier with O(1) NeighborBarrier in ring allreduce kernel (simpler PR hw-native-sys#1383 pattern): notify right, wait left per round - Update codegen UT assertion: 'RoundBarrier' → 'NeighborBarrier' - Fix doc wording: shape[0] 'must equal' qualified with compile-time constant nuance (EN+ZH pass docs) - Fix Unicode minus signs (U+2212 → ASCII hyphen) in EN+ZH distributed_ops.md
YunjiQin
pushed a commit
that referenced
this pull request
Aug 28, 2026
…nCore composite (#2280) ## Summary Replaces the **pull-model** engine of the ring allreduce on **both rails** with a **TPUT push model** (remote write), enabling O(1) `NeighborBarrier` on the HOST builtin and eliminating the pull-model NPU memory-ordering gap. - **HOST builtin** (`builtin.tensor.allreduce_ring`): reduce-scatter + allgather converted from `TLOAD`/`TSTORE` pull to `pto::comm::TPUT` push — `TPUT<AtomicAdd>` remote-accumulate for RS, non-atomic `TPUT` for AG. Ordering is `pipe_barrier(PIPE_ALL)` around every transfer + `dsb(DSB_DDR)` before `TNOTIFY` (mirrors the in-tree allgather/all_to_all host builtins; not a GM fence). The O(P²) `RoundBarrier` is 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. - **InCore composite** (`LowerTensorRingAllReduceRule`): replaces `pld.tile.remote_load` pulls with `pld.tile.put` pushes (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 via `tile.set_validshape`. ## Requires PTOAS >= v0.55 (pypto pins v0.57) **This PR depends on [PTOAS v0.55](https://github.com/hw-native-sys/PTOAS/releases/tag/v0.55)** (release: [hw-native-sys/PTOAS#1069](hw-native-sys/PTOAS#1069), fixed in [PR #1079](hw-native-sys/PTOAS#1079)). The InCore composite's `pld.tile.put` transfers carry the **exact ragged `valid_cols`** as the partition-view extent. PTOAS ≤ v0.54 rejects dynamic partition-view shapes for `pto.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 the `ChipTensor`→`TaskTensor` kernel rename, then onto #2542 adopting the `42-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.create` staging tile instead of `pld.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 with `TNOTIFY(-1)` per round). That credit pattern does **not** match the push model's `NeighborBarrier`: - `NeighborBarrier` credits only the **two ring neighbours** per round — a single cell when `nranks == 2`, where both neighbours are the same peer and the cell carries two +1s. - The #2279 loop would 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 #2279 fixed. The epilogue now branches on `kUseNeighborBarrier`: it restores only the two neighbour cells per used row with `TNOTIFY(-1, AtomicAdd)` (twice on the shared cell when `nranks == 2`), keeping the all-peer reset for the `RoundBarrier` fallback. 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.py` reuse leg) is the NPU gate for the adapted epilogue. ## Issues this PR addresses - **#2242 (ring unaligned-data handling)**: the pull-model dcci-flush tail gap (item 1) is **moot** — the push model needs no cacheline flush (the receiver reads data the sender wrote remotely via TPUT, never a locally-TSTORE'd line). The 32-byte transfer-alignment concern (item 2) is handled by narrowing the staging tile's column mask (`ColMaskInternal` / `tile.set_validshape`) to the exact (possibly ragged) transfer extent, so partial tails transfer exactly and never over-read/overwrite adjacent slots. - **#2213 (PTOAS dynamic partition-view)**: closed as superseded by #2524; the `>= v0.55` dependency 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 ✅ (with `NeighborBarrier` enabled) - `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 ✅ - UTs: `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::kSum` only by construction, so its `TPUT<AtomicAdd>` RS is fine. The composite keeps non-atomic push + local reduce to preserve Sum/Max/Min/Prod; only a remote-atomic `TPUT<AtomicAdd>` variant would be Sum-only (`AtomicType` has no `AtomicMax/Min`). ## Follow-ups (not in this PR) - **#2310** — lift the HOST-rail in-loop `pld.tensor.allreduce` restriction via shared-signal synthesis (the other half of the #2279 review). - **TPUT_ASYNC** (pto-isa) for an overlapped / IBing forward phase — optional perf follow-on (simpler #1383 / plan 50). ## Review notes - Addresses CodeRabbit feedback: the allgather ready-barrier rationale is corrected (counters are per-round; the real guarantee is the previous round's push-done barrier every rank passes before round k), and the `nranks == 2` `NeighborBarrier` behaviour is documented.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
pto-isacommit from2c607938tod779cd0acrossci.ymlanddaily_ci.yml. The new commit fixes a Tpush/Tpop bug on a5sim.xfailmarkers intests/st/runtime/test_cross_core.pyfor the cross-core Tpop / bidirect / multi-pipe tests on a5sim, now that the underlying 950 backend issue is resolved.Testing