perf(qwen3-14b decode): early-dispatch infra + critical-path restructuring#691
perf(qwen3-14b decode): early-dispatch infra + critical-path restructuring#691ChaoWao wants to merge 18 commits into
Conversation
…uring Five mechanisms for minimum critical-path latency: 1. SPMD q/k/v_proj: originally per-tile discrete tasks so downstream rope could consume each tile independently. Now that rope + QK-norm are fused into fa_fused, tiles can't provide early-start benefit — consolidate to SPMD to reduce dependency-resolution overhead. 2. Merged seeds (q_seed / kv_seed / mlp_out_seed): these zero-fill tasks have long dep chains and excessive successor counts. Their successors all depend on fa_fused anyway, so one merged seed + fa dep suffices — reduces orchestrator generation, scheduler dep resolution, and AICore launch overhead. 3. seed_dummy barrier: unflagged task_dummy gates rms_recip / kv_seed / mlp_out_seed so they are NOT early-dispatched. At prev_out completion, only q_proj should get the dispatch window (it is fa's longest predecessor). Without the barrier, kv/mlp seeds compete for the window, delaying q_proj. 4. allow_early_resolve on critical path (q_proj -> fa_fused -> out_proj -> next-layer dcr_xgamma) + fa producers (rms_recip, k/v_proj, fa_work_build, mlp_out_seed, q_seed, down_proj): each flagged task propagates dispatch_fanin to its consumers, enabling pre-staging before producers truly complete. 5. out_proj 24/26 critical-path split: ~24 of the 50 out_proj tasks feed the critical downstream chain (residual_rms_cast -> gate/up_proj). These 24 connect directly to fa for first-wave dispatch. The remaining 26 go through out_proj_dummy (unflagged task_dummy, no early dispatch) to defer their scheduling priority. Co-Authored-By: Claude <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR restructures the ChangesDecode layer scheduling rework
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SeedDummy as seed_dummy
participant QSeed as q_seed
participant QProj as q_proj (spmd)
participant KVSeed as kv_seed
participant KProj as k_proj (spmd)
participant VProj as v_proj (spmd)
participant FaFused as fa_fused
participant OutProjDummy as out_proj_dummy
participant OutProj as out_proj tasks
participant DownProj as down_proj
SeedDummy->>KVSeed: gates dispatch
QSeed->>QProj: allow_early_resolve
KVSeed->>KProj: deps + early dispatch prevention
KVSeed->>VProj: deps + early dispatch prevention
QProj-->>FaFused: rope dependency task ids
KProj-->>FaFused: rope dependency task ids
VProj-->>FaFused: rope dependency task ids
FaFused->>OutProjDummy: attn_done_tid
FaFused->>OutProj: direct deps (priority subset)
OutProjDummy->>OutProj: deferred subset
OutProj->>DownProj: allow_early_resolve
Possibly related PRs
Suggested labels: 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.
Code Review
This pull request restructures the decode layer of the Qwen3-14B model to minimize critical-path latency. Key changes include consolidating q/k/v_proj to SPMD, merging zero-fill seeds, introducing a seed_dummy barrier to prevent early-dispatch competition, flagging critical path stages with allow_early_resolve, and splitting out_proj tasks into critical and deferred waves. Feedback focuses on improving code robustness and maintainability by replacing hardcoded task dependency indices with dynamic list comprehensions and refactoring duplicated loop logic in the out_proj split into an inline helper function.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| name_hint="rms_recip", | ||
| deps=[prev_out_tids[i] for i in range(DOWN_ON)], | ||
| allow_early_resolve=True, | ||
| deps=[prev_out_tids[0], prev_out_tids[1], prev_out_tids[2], prev_out_tids[3], prev_out_tids[4], seed_dummy], |
There was a problem hiding this comment.
Hardcoding the indices 0 to 4 of prev_out_tids is fragile and will break if DOWN_ON changes. Consider using a single list comprehension to dynamically construct the dependency list up to DOWN_ON and append seed_dummy without using list concatenation.
| deps=[prev_out_tids[0], prev_out_tids[1], prev_out_tids[2], prev_out_tids[3], prev_out_tids[4], seed_dummy], | |
| deps=[prev_out_tids[i] if i < DOWN_ON else seed_dummy for i in range(DOWN_ON + 1)], |
| with pl.spmd( | ||
| Q_ON * QKV_OK, | ||
| name_hint="q_proj", | ||
| deps=[prev_normed_tids[0], prev_normed_tids[1], prev_normed_tids[2], prev_normed_tids[3], prev_normed_tids[4], q_seed_tid], |
There was a problem hiding this comment.
Hardcoding the indices 0 to 4 of prev_normed_tids is fragile and will break if DOWN_ON changes. Consider using a single list comprehension to dynamically construct the dependency list up to DOWN_ON and append q_seed_tid without using list concatenation.
| deps=[prev_normed_tids[0], prev_normed_tids[1], prev_normed_tids[2], prev_normed_tids[3], prev_normed_tids[4], q_seed_tid], | |
| deps=[prev_normed_tids[i] if i < DOWN_ON else q_seed_tid for i in range(DOWN_ON + 1)], |
| rope_dep_tids[WORK_TID_IDX] = work_tid # fa_fused (now folds in rope) also waits on fa_work_build | ||
| # ── KV seed: zero k_proj + v_proj buffers. ── | ||
| # ── KV seed: zeroed via seed_dummy barrier (see comment above). ── | ||
| with pl.at(level=pl.Level.CORE_GROUP, name_hint="kv_seed", deps=[prev_out_tids[0], prev_out_tids[1], prev_out_tids[2], prev_out_tids[3], prev_out_tids[4], seed_dummy]) as kv_seed_tid: |
There was a problem hiding this comment.
Hardcoding the indices 0 to 4 of prev_out_tids is fragile and will break if DOWN_ON changes. Consider using a single list comprehension to dynamically construct the dependency list up to DOWN_ON and append seed_dummy without using list concatenation.
| with pl.at(level=pl.Level.CORE_GROUP, name_hint="kv_seed", deps=[prev_out_tids[0], prev_out_tids[1], prev_out_tids[2], prev_out_tids[3], prev_out_tids[4], seed_dummy]) as kv_seed_tid: | |
| with pl.at(level=pl.Level.CORE_GROUP, name_hint="kv_seed", deps=[prev_out_tids[i] if i < DOWN_ON else seed_dummy for i in range(DOWN_ON + 1)]) as kv_seed_tid: |
| attn_proj_fp32 = pl.create_tensor([BATCH, HIDDEN], dtype=pl.FP32) | ||
|
|
||
| with pl.at(level=pl.Level.CORE_GROUP, name_hint="down_seed", deps=[prev_out_tids[_si] for _si in range(DOWN_ON)]) as seed_tid: | ||
| with pl.at(level=pl.Level.CORE_GROUP, name_hint="mlp_out_seed", allow_early_resolve=True, deps=[prev_out_tids[0], prev_out_tids[1], prev_out_tids[2], prev_out_tids[3], prev_out_tids[4], seed_dummy]) as mlp_out_seed_tid: |
There was a problem hiding this comment.
Hardcoding the indices 0 to 4 of prev_out_tids is fragile and will break if DOWN_ON changes. Consider using a single list comprehension to dynamically construct the dependency list up to DOWN_ON and append seed_dummy without using list concatenation.
| with pl.at(level=pl.Level.CORE_GROUP, name_hint="mlp_out_seed", allow_early_resolve=True, deps=[prev_out_tids[0], prev_out_tids[1], prev_out_tids[2], prev_out_tids[3], prev_out_tids[4], seed_dummy]) as mlp_out_seed_tid: | |
| with pl.at(level=pl.Level.CORE_GROUP, name_hint="mlp_out_seed", allow_early_resolve=True, deps=[prev_out_tids[i] if i < DOWN_ON else seed_dummy for i in range(DOWN_ON + 1)]) as mlp_out_seed_tid: |
| KV_ON * QKV_OK, | ||
| name_hint="k_proj", | ||
| allow_early_resolve=True, | ||
| deps=[prev_normed_tids[0], prev_normed_tids[1], prev_normed_tids[2], prev_normed_tids[3], prev_normed_tids[4], kv_seed_tid], |
There was a problem hiding this comment.
Hardcoding the indices 0 to 4 of prev_normed_tids is fragile and will break if DOWN_ON changes. Consider using a single list comprehension to dynamically construct the dependency list up to DOWN_ON and append kv_seed_tid without using list concatenation.
| deps=[prev_normed_tids[0], prev_normed_tids[1], prev_normed_tids[2], prev_normed_tids[3], prev_normed_tids[4], kv_seed_tid], | |
| deps=[prev_normed_tids[i] if i < DOWN_ON else kv_seed_tid for i in range(DOWN_ON + 1)], |
| KV_ON * QKV_OK, | ||
| name_hint="v_proj", | ||
| allow_early_resolve=True, | ||
| deps=[prev_normed_tids[0], prev_normed_tids[1], prev_normed_tids[2], prev_normed_tids[3], prev_normed_tids[4], kv_seed_tid], |
There was a problem hiding this comment.
Hardcoding the indices 0 to 4 of prev_normed_tids is fragile and will break if DOWN_ON changes. Consider using a single list comprehension to dynamically construct the dependency list up to DOWN_ON and append kv_seed_tid without using list concatenation.
| deps=[prev_normed_tids[0], prev_normed_tids[1], prev_normed_tids[2], prev_normed_tids[3], prev_normed_tids[4], kv_seed_tid], | |
| deps=[prev_normed_tids[i] if i < DOWN_ON else kv_seed_tid for i in range(DOWN_ON + 1)], |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
models/qwen3/14b/decode_fwd.py (1)
932-933: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the split from the tile count rather than hardcoding
24.
N_OUT_DIRECT = N_SPLITS_OUT * K_SPLITS_OUT - 24bakes in the assumption that there are exactly 50 out_proj tasks (the "First 26" comment). IfN_SPLITS_OUTorK_SPLITS_OUTare retuned, the 24/26 balance drifts silently, and a total below 24 makesN_OUT_DIRECTnegative. Consider a named constant or a fraction of the total so the split scales with the tiling.🤖 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 `@models/qwen3/14b/decode_fwd.py` around lines 932 - 933, The out-proj task split in decode_fwd.py is hardcoded with a fixed 24-task remainder, which will drift or go negative if the tiling changes. Update the logic around N_OUT_DIRECT in the decode path to derive the direct-vs-first split from the total tile count (N_SPLITS_OUT * K_SPLITS_OUT) using a named constant or computed fraction instead of embedding 24, and ensure the calculation stays valid for smaller or retuned split configurations.
🤖 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 `@models/qwen3/14b/decode_fwd.py`:
- Around line 526-530: The q_proj stage is missing the allow_early_resolve flag,
which breaks propagation through the primary attention path. Update the pl.spmd
call in q_proj within decode_fwd.py to include allow_early_resolve=True,
matching the neighboring attention stages so dispatch_fanin can continue through
q_proj → fa_fused → out_proj → dcr_xgamma.
---
Nitpick comments:
In `@models/qwen3/14b/decode_fwd.py`:
- Around line 932-933: The out-proj task split in decode_fwd.py is hardcoded
with a fixed 24-task remainder, which will drift or go negative if the tiling
changes. Update the logic around N_OUT_DIRECT in the decode path to derive the
direct-vs-first split from the total tile count (N_SPLITS_OUT * K_SPLITS_OUT)
using a named constant or computed fraction instead of embedding 24, and ensure
the calculation stays valid for smaller or retuned split configurations.
🪄 Autofix (Beta)
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
Run ID: c2020571-38a8-454f-b4af-86a637fb7b6e
📒 Files selected for processing (1)
models/qwen3/14b/decode_fwd.py
- avoid IfExp in manual dependency expressions - keep seed_dummy independent of previous xgamma - name and clamp the out_proj direct-task split
- avoid IfExp in manual dependency expressions - keep seed_dummy independent of previous xgamma
Add sync_start=True alongside allow_early_resolve on the fused attention pl.spmd scope.
暂时seed_dummy依赖xgamma
Note: the
|
…ect wave - q_proj: allow_early_resolve=True so its dispatch bumps fa_fused's dispatch_fanin, enabling the sync_start early-dispatch path (simpler #1304). - out_proj: fold the last 24 direct-fa tiles into ONE SPMD task (was 24 separate CORE_GROUP tasks) so the direct wave is a single early-dispatch candidate; block_idx -> (n, k) tile, shared out_proj_direct_tid. - seed_dummy: deps=[] (empty barrier) to match its comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uring Five mechanisms for minimum critical-path latency: 1. SPMD q/k/v_proj: originally per-tile discrete tasks so downstream rope could consume each tile independently. Now that rope + QK-norm are fused into fa_fused, tiles can't provide early-start benefit — consolidate to SPMD to reduce dependency-resolution overhead. 2. Merged seeds (q_seed / kv_seed / mlp_out_seed): these zero-fill tasks have long dep chains and excessive successor counts. Their successors all depend on fa_fused anyway, so one merged seed + fa dep suffices — reduces orchestrator generation, scheduler dep resolution, and AICore launch overhead. 3. seed_dummy barrier: unflagged task_dummy gates rms_recip / kv_seed / mlp_out_seed so they are NOT early-dispatched. At prev_out completion, only q_proj should get the dispatch window (it is fa's longest predecessor). Without the barrier, kv/mlp seeds compete for the window, delaying q_proj. 4. allow_early_resolve on critical path (q_proj -> fa_fused -> out_proj -> next-layer dcr_xgamma) + fa producers (rms_recip, k/v_proj, fa_work_build, mlp_out_seed, q_seed, down_proj): each flagged task propagates dispatch_fanin to its consumers, enabling pre-staging before producers truly complete. 5. out_proj 24/26 critical-path split: ~24 of the 50 out_proj tasks feed the critical downstream chain (residual_rms_cast -> gate/up_proj). These 24 connect directly to fa for first-wave dispatch. The remaining 26 go through out_proj_dummy (unflagged task_dummy, no early dispatch) to defer their scheduling priority. Co-Authored-By: Claude <noreply@anthropic.com>
- avoid IfExp in manual dependency expressions - keep seed_dummy independent of previous xgamma
Add sync_start=True alongside allow_early_resolve on the fused attention pl.spmd scope.
…ect wave - q_proj: allow_early_resolve=True so its dispatch bumps fa_fused's dispatch_fanin, enabling the sync_start early-dispatch path (simpler #1304). - out_proj: fold the last 24 direct-fa tiles into ONE SPMD task (was 24 separate CORE_GROUP tasks) so the direct wave is a single early-dispatch candidate; block_idx -> (n, k) tile, shared out_proj_direct_tid. - seed_dummy: deps=[] (empty barrier) to match its comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
47ebebb to
70b898c
Compare
- Port PR hw-native-sys#691 critical-path changes onto current main - Replace decode fa_fused with direct A2A3 CCEC via pl.jit.extern - Preserve public signatures and the vLLM TND/BSND tensor ABI - Add runtime tiling and uniform, ragged, and cache-offset checks - Restore CANN attribution and include the OSLA v2 license and notice - Leave prefill and the Ascend950 attention fallback out of scope
- Port PR hw-native-sys#691 critical-path changes onto current main - Replace decode fa_fused with direct A2A3 CCEC via pl.jit.extern - Preserve public signatures and the vLLM TND/BSND tensor ABI - Use round-to-nearest-even for Q/K/V BF16 boundary conversion - Add runtime tiling and uniform, ragged, and cache-offset checks - Restore CANN attribution and include the OSLA v2 license and notice - Leave prefill and the Ascend950 attention fallback out of scope
## Summary - Restore the Qwen3-14B prefill runtime-scope topology used before the fused QKPV rewrite: parent token scope, one RoPE/KV scope, and one attention scope per finalization window. - Keep `all_q_padded_tile` and `attn_tile` in the parent token scope so they bridge child scopes without retaining child scratch tensors. - Keep `attn_tile` as an inout buffer instead of returning and rebinding it across the attention scope, avoiding a scope-local SSA phi escape in generated orchestration C++. - Add source regressions for the four-ring topology and parent buffer ownership. - Validation: 161 golden tests and all pre-commit hooks pass; the exact batch-16/max-seq-4096 host orchestration compiles and assembles; Qwen3-14B with #691 decode, a 3.5k prompt, 2 GiB ring heap, and 20 generated tokens passes 3/3 hardware runs with no allocator/deadlock signatures. ## Related Issues - Follow-up to #746, which removed the inner manual scope boundaries during the fused prefill rewrite.
Summary
Five mechanisms for minimum critical-path latency in the qwen3-14B decode layer.
1. SPMD q/k/v_proj
Originally per-tile discrete tasks so downstream rope could consume each tile's output independently and start early. Now that rope + QK-norm are fused into
fa_fused(one big kernel), the tiles can no longer provide early-start benefit — all outputs are consumed atomically by the fused kernel. Consolidating to SPMD eliminates the per-tile dependency-resolution overhead (fewer edges in the task graph).2. Merged seeds (q_seed / kv_seed / mlp_out_seed)
These zero-fill tasks have long dependency chains and excessive successor counts. Their successors all depend on
fa_fusedanyway, so funneling through a single merged seed + fa dep suffices. This reduces orchestrator generation, scheduler dependency resolution, and AICore launch overhead.3.
seed_dummybarrierseed_dummy(unflaggedtask_dummy) gates rms_recip / kv_seed / mlp_out_seed so they are NOT early-dispatched. At the moment prev_out completes, onlyq_projshould get the dispatch window (it is fa's longest predecessor). Without the barrier, kv/mlp seeds would compete for the window, delaying q_proj.4.
allow_early_resolveon critical pathFlags the critical path (q_proj → fa_fused → out_proj → next-layer dcr_xgamma) so each stage propagates
dispatch_faninto the next, enabling pre-staging before producers truly complete. Fa's other producers (rms_recip, k/v_proj, fa_work_build, mlp_out_seed, q_seed, down_proj) are also flagged so fa'sdispatch_fanincan reachfanin_actual_count.5. out_proj 24/26 critical-path split
Of the 50 out_proj tasks, ~24 feed the critical downstream chain (residual_rms_cast → gate/up_proj). These 24 connect directly to fa (
deps=[attn\_done\_tid]) for first-wave dispatch. The remaining 26 go throughout\_proj\_dummy(task_dummy, unflagged → no early dispatch) to defer their scheduling priority, ensuring the critical 24 execute first.Test
python decode_fwd.py -p a2a3 -d <dev> --validate-fwd --fwd-layers 2 --enable-l2-swimlane— golden argmax passes.🤖 Generated with Claude Code