Repository navigation
[PCP] Fix sampled-id sync, DSpark aux gather and FP4 indexer padding - #2125
Merged
Merged
Conversation
yitingw1
marked this pull request as draft
September 3, 2026 06:30
Contributor
🏷️ CI GuideRuns automatically on every eligible PR before approval:
Heavy model tests:
|
`pcp_merged_tp_grid` cut unconditionally on the full pcp*tp grid, which faults in the weight loader whenever `moe_inter_dim / (pcp*tp)` is not a 128 multiple -- any tp*pcp=16 deployment, where 3072/16 = 192 splits a block-quant scale block in half. It now takes the largest divisor that stays aligned and reports a `prescale` of g/(pcp*tp). The two cases that were hand-written fall out of that formula -- no scale on the full grid, the familiar 1/pcp_size when the module stays replicated -- so the pre-scale branch that had become unreachable carries weight again. 8-card configs are unchanged: still cut 8 ways, still prescale 1.0. Also from review: - Refuse ATOM_PCP_MOE_MERGE together with MoE's flatten-TP-across-DP. That fold shards routed across pcp*dp*tp, while a module summed only by the tp all_reduce and the pcp collective never sees a DP reduce. - Refuse online re-quantisation when the effective TP grid is wider than the real TP group: the gather runs on the real group and the shard-back on the override, so it would narrow 1/pcp of a weight as if it were whole. DCP's override narrows instead and is unaffected. - One `_needs_id_sync` predicate behind both the PCP and the TP id broadcast, which have to stay the same condition. - Tests: the two folds asserted against each other on every rank (a reversed flatten goes red on tp4pcp2/tp2pcp4 and, as expected, not on pcp8 where tp=1 cannot tell them apart), the aligned step-down, and that shards tile the ranks evenly -- which is what makes prescale a scalar rather than a per-rank factor. - The import guard uses skip_if_dependency_missing rather than `except Exception`, which would read a SyntaxError as a skip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
yitingw1
force-pushed
the
DSV4_PCP_opt
branch
from
October 23, 2026 09:18
bbdeecd to
04274b6
Compare
Since ROCm#2688 the FP4 prefill scorer is the row-group kernel, which reads group b's keys off block_tables[b] and asserts the table covers every group. pcp_owned_rows_by_request still gave the PCP padding rows a group of their own, one more than the requests the table has rows for, so any PCP prefill whose token count is not a multiple of pcp asserted (block_tables.shape[0] >= batch). The old prefill kernel looked rows up by batch_id_per_q_token and skipped bid < 0, so the extra group was harmless there. Padding rows sit at position 0, so their bound is 0 and they read no key in whichever group they land in. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Under PCP the MoE weights are folded into the TP grid for every model, the draft included, so each rank's FFN output is a partial that needs the PCP all_reduce. Block.forward (target and MTP) gets it from moe_pcp_merge_forward; DSparkLayer.forward_block rewrote the FFN step as a plain self.ffn() call and never summed, which halved the DSpark acceptance rate under PCP. The draft graph is recorded with is_prefill=False and is_dummy_run=False, so the decode branch's all_reduce is what goes into it. V4-Pro-DSpark, gsm8k 200 x 5-shot, k=5: pcp8 32.96% -> 63.14% acceptance against a same-source tp8 at 63.48%. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…it_rows in every forward - DSparkProposer.propose called aux_for() only to check it was not None, which under a PCP prefill all_gathered the aux buffers a second time (compute_draft_kv already gathered them) and threw the result away. Check _captures_aux instead. - pcp_split_rows was set only in ModelRunner.forward; RapidServe's decode forward overrides it and never did, so aux was not gathered there. Both forwards now call _settle_pcp_split_rows() after run_model. - Tests: the FP4 owned-rows case now has a real padding row, plus a last-request-owns-only-padding case and a brute-force comparison; a structural test pins DSpark's forward_block to the same moe_pcp_merge_forward branch as Block.forward. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Online re-quantization gathers the full weight on the real TP group before quantizing (per_Token on the row-parallel w2 always needs it). Folded onto the wider pcp*tp grid, that gather rebuilds only 1/pcp of the weight, which linear.py now refuses at load -- turning a config that ran on main (the shared expert unsharded under PCP) into a load failure. When any of the shared expert's linears will be re-quantized online, do not fold it: it stays on TP exactly as on main. Offline-quantized checkpoints are unaffected. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
yitingw1
marked this pull request as ready for review
October 26, 2026 09:15
f324bd9 inserted _settle_pcp_split_rows between ModelRunner.forward and its decorators, so the helper picked up @_profile_runner_stage, @torch.inference_mode() and @with_eplb_forward_monitor -- the last one binds a batch argument, so every forward raised TypeError -- and forward lost all three. A test now calls the helper bare and checks that forward is still wrapped. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- _settle_pcp_split_rows warns once when PCP is on, a drafter is present and the model has no pcp_split_active(): it then assumes no row-split, and a future PCP model that forgot the method would silently leave the drafter's aux ungathered. - Shorten the comments and docstrings this branch added to what/why; the derivations live in the PR description. No code change beyond the warning (AST-compared with docstrings stripped). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
# Conflicts: # atom/spec_decode/eagle_proposer.py
shared_experts on the folded TP grid, and sync sampled ids outside the speculative branch…OCm#2725 main's ROCm#2725 moves the PCP gather/reduction into FusedMoE._run_experts, around the routed experts only, and computes the shared expert on each rank's own 1/pcp rows. That already takes the prefill gain the shared-expert sharding was after, and a shared expert sharded across PCP can no longer be summed in prefill (the ranks hold different rows). It also deletes moe_pcp_merge_forward, which the DSpark draft FFN fix called; FusedMoE now all-reduces the draft's MoE itself. Removed: pcp_merged_tp_grid / PcpSharedGrid, Expert(tp_grid), the shared prescale, the online re-quant check, linear.py's override forwarding and gather guard, DSparkLayer.forward_block's merge branch, and their tests. Kept: sampled-id sync and the PCP+TBO+spec guard, pcp_split_active and the DSpark aux gather, the FP4 indexer padding fix. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ROCm#2725 makes _pcp_split_draft_inputs flag the pass as split on the forward context for FusedMoE's PCP gather, so the CPU tests have to supply one. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
aux_for must gather exactly when the target forward row-split, and the draft prefill split must follow that answer (pcp_split_rows), not the current pass's pcp_split, which drafting clears. Reverting either fix now fails a test instead of silently halving DSpark's acceptance. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Next to ROCm#2725's per-pass pcp_split the old name read as a synonym. It is the target forward's answer, fixed right after it and kept through drafting, which clears pcp_split per pass; it is now also a Context __init__ argument like its sibling. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Since ROCm#2725 both deepseek_v2 and deepseek_v4 write the target forward's split decision to context.pcp_split, and FusedMoE gathers on it. target_pcp_split now copies that value right after run_model, before drafting clears it, instead of re-deriving it through a separate pcp_split_active(). The aux gather and FusedMoE thus read one decision. Drops pcp_split_active() from pcp_utils and both models, the _pcp_active delegation, and the missing-method warning; this PR no longer touches the model files. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 task
rbrugaro-amd
added a commit
to rbrugaro-amd/ATOM
that referenced
this pull request
Nov 2, 2026
Conflict in atom/spec_decode/drafter.py imports only: keep K3_DFLASH_ARCH from this branch and main's pcp_utils imports (ROCm#2125). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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
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.
Motivation
Three correctness fixes on the PCP path (
-pcp, measured at-tp 1 -pcp 8). Each is a place where ranks disagreed about which rows they hold, or where the code assumed they agree when they do not.main(TP: 64.83 %)This PR originally also sharded the shared expert across PCP and summed the DSpark draft FFN across PCP. #2725 reworked the PCP MoE reduction (now inside
FusedMoE, routed only, shared expert on each rank's own rows), which already takes the prefill gain and sums the draft's MoE; both were dropped here. The remaining decode/memory gain of a sharded shared expert does not fit #2725's structure and is left for a separate discussion.Technical Details
1. Syncing the sampled ids
At
tp=1PCP ranks decode redundantly but not bit-identically: ~1 bf16 ULP flips near-tie argmaxes. On DeepSeek-V3.2, 32.8 % of decode steps sample divergent ids and 1.5 % reach the next forward'sinput_ids, where the decode-time MoEpcp_all_reduceassumes identical rows. The broadcast was inside the spec branch only, so PCP without a drafter and prefill steps with MTP were never synced. It now runs outside it, behind one_needs_id_sync(is_deferred_out, has_drafter)predicate shared with the TP broadcast so the two cannot drift; the verdict stays synced inside the spec branch.PCP + TBO + spec is refused at startup: TBO stripes positions per request group while the draft split stripes the batch as one sequence, so their shards disagree.
2. DSpark aux hidden states under PCP
DSpark's aux hidden states are captured by forward hooks on middle target layers. After a PCP row-split prefill those layers hold only this rank's 1/pcp round-robin stripe, but
Drafter.aux_forread them as if they were the full rows - most of what the draft consumed was the previous forward's leftovers.aux_fornow all-gathers the buffers when the target forward row-split.target_pcp_split: a snapshot of feat(moe): route before the rows move; per-pass real row counts; V4.1 FP8 KV #2725'scontext.pcp_split- the flag the model writes where it row-splits andFusedMoEgathers on - taken byModelRunner._settle_target_pcp_split()right afterrun_model, in bothModelRunner.forwardand RapidServe's decode forward. The snapshot is needed because drafting clearspcp_splitper pass before the draft runs. It is never derived from hook-side bookkeeping, which a cudagraph replay skips (an earlier attempt that did so desynchronised ranks and hung NCCL)._pcp_active_for_draft_modelreads the same snapshot instead of re-deriving the split from the draft model's class name, which had sent every V4 drafter other thanDeepseekV4MTPto the V2 gate.propose()checks that aux capture is configured instead of callingaux_for(), which would have gathered a second time.3. FP4 indexer padding rows
..._fp4_prefillbatch_id_per_q_token[r]bid = -1, skippedb→block_tables[b]bs+1, but the table hasbsrows → assertpcp_owned_rows_by_requestnow folds padding rows into the last request's group. They sit at position 0, so they read no key - the same outcome as the old skip. Both callers (V4 and the V3.2 MLA path) share the function. FP4 is native V4's default index cache everywhere but gfx942 (config.py), so on MI355X the default V4 PCP setup hit this on any prefill whose token count is not a multiple of pcp.Test Plan
Test Result
Unit tests (with the related MoE / draft-graph suites): 293 passed, 4 skipped; ruff and black clean. Each new test fails with its fix reverted.
DSpark + PCP on
main(V4-Pro-DSpark, MI355X,-tp 1 -pcp 8, gsm8k 5-shot, full 1319, same node, serial).mainneeds the FP4 padding fix to run at all, so its arm carries that one line:main+ padding fixff59d4b93)The gap is this PR as a whole (aux gather and sampled-id sync) against
main; it brings PCP level withtp8(64.83 % on an earlier base). A later refactor (target_pcp_splitnow snapshotspcp_split) re-smoked at 63.35 % on 200 questions, in line with earlier 200-question runs (63.14-63.53 %). The runs also cover the FP4 indexer fix through real middle prefill chunks. Token sync on DeepSeek-V3.2 (measured before #2725):pcp8+MTP94.64 % vstp8+MTP94.69 % acceptance, gsm8k 0.9560 / 0.9507.Known Limitations
confidence_schedule(off by default) is not PCP-safe: each rank picks its own verify length and nothing broadcasts it.Submission Checklist
🤖 Generated with Claude Code