Repository navigation
perf(minimax-m3): optimize gfx1250 prefill indexing - #2971
XiaobingSuper wants to merge 6 commits into
Conversation
🏷️ CI GuideRuns automatically on every eligible PR before approval:
Heavy model tests:
|
195f67e to
0eed759
Compare
8567406 to
0740626
Compare
0740626 to
8356d4d
Compare
8356d4d to
6610c79
Compare
6610c79 to
bb11ec4
Compare
| if arch != "gfx1250" or not is_fp8 or batch != 1: | ||
| return SCORE_BLOCK_SIZE_Q, None, None, None | ||
| if heads == 1 and 2369 <= max_query_len <= 2688 and 768 <= max_block <= 1024: | ||
| return 64, 22, 2, 3 |
There was a problem hiding this comment.
these magic numbers lost their derivation, please leave a comment so we know where 22/3 comes from..
There was a problem hiding this comment.
Fixed in e82c45d. The shape ranges and fixed chunk/stage values are gone; the launch is now derived from runtime CU count, query planes, and context blocks, with comments for the occupancy thresholds.
| ): | ||
| """One page's dot, causal mask and block max.""" | ||
| i = blk * BLOCK_SIZE_K | ||
| k = tl.load( |
There was a problem hiding this comment.
the unmasked load lost its justification, please restore the comment or add mask=?
There was a problem hiding this comment.
Fixed in e82c45d. Restored the justification immediately above the load: each page id names a fully allocated 128-row cache page, and the later causal mask removes positions beyond the live sequence.
| chunk z is the same pages for every query tile that reaches it. | ||
| """ | ||
| index = device.index if device.index is not None else torch.cuda.current_device() | ||
| if ( |
There was a problem hiding this comment.
the chunk window has three owners now, can we keep one table keyed on (arch, is_fp8, max_query_len, ...) instead of re-deriving it?
There was a problem hiding this comment.
Fixed in e82c45d. _prefill_score_config is now the single gfx1250 FP8 policy owner; it returns the complete tile/chunk/warp/stage/store configuration from (arch, dtype class, q, blocks, batch, heads, CUs). The original generic chunk policy is only the unchanged fallback.
| if q_zero: | ||
| kw["idx_q"].zero_() | ||
|
|
||
| tuned = m.minimax_m3_index_topk(**kw, emit_sparse_block_table=True) |
There was a problem hiding this comment.
this would still pass if the tuned launch stopped firing, please assert the selected policy and the new chunk=24 branch..
There was a problem hiding this comment.
Fixed in e82c45d. The policy test now asserts every selected launch tuple, including fallback boundaries and the 16-page cadence adjustment; GPU tests compare scores and all emitted outputs against the original path. Final runs: CI simulation 215 passed / 1 skipped, gfx1250 191 passed / 1 skipped.
8fe4488 to
e82c45d
Compare
e82c45d to
861bbb1
Compare
|
GPU0 follow-up at
The policy is now derived from query tiles, batch/heads, context-block waves, and runtime CU count; no model name or trace-shape table. An experimental direct port of the MI355 FlyDSL scorer was rejected: 83.397 us vs 69.282 us Triton on the anchor, with non-bit-exact scores and changed ordered top-k. |
This reverts commit 88354a2b23917fdcb1c4f3be4ab43ae6c8d0bf84. That commit was an early snapshot of #2971. The current version of the PR follows as three commits. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Wave32 selector follow-up pushed in This is the selector hotspot found by kernel profiling, not another scorer tile change. The gfx1250 policy uses
The direct SGLang-style fused score+streaming-topk prototype was also measured and rejected (85-153 us on the anchor versus ~52 us current complete path). |
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
b6207dc to
9483ab5
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
9483ab5 to
58177a3
Compare
valarLip
left a comment
There was a problem hiding this comment.
⚠️ Action required before merge (5)
- Drop
@functools.cachefrom_aiter_selector_wins.index_topk.py:894— the key now includesrows = heads * total_q, so every distinct prefill size adds a permanent entry on every arch (gfx950 too, whererowsdoesn't even affect the result). CPU probe: 40k distinct steps -> 40k entries,maxsize=None. The body is two integer compares; just remove the cache (bounding it at 128 would only churn) and makerows/compute_unitsrequired.lru_cacheonparallel_prefill_selector_wins(prefill_policy.py:62) buys nothing either. - Make
prefill_score_configthe single owner of the scorer launch (valar-cop's "three owners" ask is half done). Warps/stages/chunk still come from three places merged withor(index_topk.py:1717,1728-1729:_prefill_score_launch_config+_score_chunk_blocks+ the policy). Have the policy return a complete config (noNonesentinels), fold the gfx1250 FP8(4, 1)default into it, and delete theorfallbacks. - Remove the duplicated fallback page body.
index_topk.py:533-563re-inlines exactly what_index_block_score_page(..., POST_REDUCE_SCALE=False)computes; Triton inlines@triton.jithelpers, so call the helper (or show the ISA/timing difference that justifies keeping two copies). - Delete the
try: _small_k_selector() except ImportError: skipintest_gfx1250_wave32_selector_matches_prefill_fallback. aiter#7878 is merged and CI installs aiter main; the import is mandatory (index_topk.py:62), so the skip only hides a broken selector (same failure mode as #3162). - Add a multi-request parity case that actually reaches the parallel selector. None of the GPU cases with
batch > 1is wide enough ([1703,717]~958 blocks,[640]*4~257) to passparallel_prefill_selector_wins, so the partial/merge kernels'pid_b/row_startsindexing is only covered for batch 1. E.g.[2858, 2331]with ~3900-block prefixes.
Smaller: the prefill-warps rationale is now orphaned above DECODE_TOPK_* (index_topk.py:758-764); the "short/underfilled" comment at :534 is wrong (that branch is every non-gfx1250 / non-FP8 launch); "GPU0 complete-path A/B" numbers at :884 lost their log pointer; rows means heads*total_q in one gate and total_q in the other — rename. Perf numbers in the description are fine; please run ci:atom/ci:vllm MiniMax-M3 accuracy before merge (description says GSM8K wasn't rerun).
Reviewed at 58177a3. valar-cop items already fixed: unmasked-load justification restored (:441, :541), policy test now asserts the selected launch. Not stale vs main (no minimax_m3 changes since base 74e389b, clean merge); aiter#7878 merged.
|
ci triage for 2b446fc:
|
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed all five review items in
Also moved the prefill warp rationale to its policy constant, restored an accessible raw-log pointer, and distinguished flattened selector rows from query rows. Validation on e15-09 GPU0 (gfx1250): 288 passed; Black/Ruff clean. A 42,120-shape host probe preserves every resolved scorer launch tuple. Review-cleanup A/B/A/B is flat-to-better: 22-shape aggregate @valarLip ready for re-review. |
valarLip
left a comment
There was a problem hiding this comment.
⚠️ Action required before merge (3)
All five items from the last round are fixed (details below). What's left is small and doesn't block:
- Fix the decode-selector comment that now contradicts itself.
index_topk.py:708-709(new) says "the best single pair over the deployed shapes is (512, 8)", but:700-701says the best single pair is (256, 4) and (512, 8) is only 0.6% behind it. Change it to "(512, 8) is the chosen pair; the warps are needed because decode's grid is 50-200 CTAs". - Assert that the batch-2 case actually takes the parallel selector.
tests/test_minimax_m3_index_topk.py:957-968checksbool(parallel_launches) is expect_parallel, andexpect_parallelcomes from the gate under test. If that gate regresses, the[2858, 2331]case silently stops covering the partial/mergepid_b/row_startsindexing. A CPU probe shows it reaches the path at 128/256/304 CUs with heads=1 (it sits exactly on the workspace bound, 4x16 = 64 keys). Addassert parallel_launchesforqlens == [2858, 2331] and heads == 1. - Use block units in the policy comments.
prefill_policy.py:25-27,209say "page chunk" and "28-page chunks", butchunk_blockscounts 128-token blocks, and in this module a "page" is a 16-token physical page (8 per block). Say "block chunk" and "28-block chunks".
Optional: prefill_score_config's lru_cache(maxsize=128) (prefill_policy.py:120) is bounded now, but it is keyed on per-step max_query_len/max_block, so eager prefill mostly misses. A CPU probe with 5000 distinct q gave 0 hits. The body is integer arithmetic, so dropping the cache costs nothing.
CI at head: ATOM Test passed, including MiniMax-M3-MXFP4 accuracy. ATOM SGLang Test passed. The red vLLM/ATOM gate entries are runs that were cancelled and superseded. In ATOM vLLM Test (run 43680334394), MiniMax-M3-MXFP4 EAGLE3 TP4 passed and 3 non-MiniMax jobs are still pending. Merge once those are green.
Re-reviewed at 2b446fc. Already fixed: _aiter_selector_wins cache removed and its kwargs required (probe re-run: no cache left on either selector gate); prefill_score_config returns the complete launch with no None/or fallbacks, and the gfx1250 FP8 (4,1) default is folded into _default_prefill_score_config; the duplicated fallback body is gone (every STORE_GROUP=1 launch calls _index_block_score_page, same op order as main); the ImportError skip is removed; the [2858, 2331] parallel-selector case is added; the warp rationale is moved, the "short/underfilled" comment dropped, the gist log pointer restored, and selector_rows/query_rows renamed. Cleanup A/B is in the description. Merges cleanly with main incl. #3104 (its _launch_select call is decode-only and compatible).
Part of https://github.com/wuhuikx/LLM-Booster/issues/341.
Summary
Optimize the MiniMax-M3 prefill index pipeline on gfx1250:
rows * width / CU;pages_per_blockcontract: page-128/MXFP4 output cannot be reused as parallel-selector workspace and falls back safely.The remaining
28-page multi-plane value is a measured kernel loop tile, not an activation shape window. Activation is derived from query tiles, planes, context work, and CU count; the fixed tile controls work inside an already-selected specialization.Dependency
The wave32 selector support landed in ROCm/aiter#7878, merge commit
5c1489c242fc50e508ae7e9f5f5b4027a7da0366.Current-main A/B/A/B
Revisions and environment:
74e389b7661fb53ffb0a7daa93b0d73afe4d747658177a3aa716de417820aa4bc6243650dd4b17d82b446fcf0804cb912409acc7b8e08398d5ec5ef2ce3681929fe19dd2c0e34bc392388fcf331a2496rocm/fw-bringup:gfx1250-minimax-m3-20261110Command inside the container:
The harness and every raw log are in this reproducible benchmark artifact.
Results:
1919.612,1902.898 us1542.718,1551.615 us1911.255 -> 1547.167 us(1.235x,-19.0%)q=2517, context105301):82.693 -> 55.080 us(1.501x,-33.4%)22.5% / 24.1% / 27.9% / 23.5% / 29.5%lower complete-path time2927.612 -> 2128.982 us(1.375x,-27.3%)190.868 -> 166.689 us206.085 -> 114.047 us228.375 -> 139.802 us166.776 -> 102.313 usThe maintainer-requested cleanup was measured separately, A/B/A/B against the
measured head. The complete 22-shape aggregate is
1559.166 -> 1535.917 us(1.015x); issue-anchor + DPEP aggregate is2131.666 -> 2129.550 us(flat). The raw cleanup logs are in the same artifact.The short/underfilled shapes retain main's original score-kernel specialization. Their complete-path times remain around the launch floor; the aggregate numbers above include them rather than filtering them out.
Production routing
emit_sparse_block_table=True; they can use the parallel selector when its workload and workspace gates pass.pages_per_block=1; the capacity-derived policy keeps it off the aliased parallel workspace while both aiter and Triton emit paths preserve page geometry.Validation
288 passedtopk_idx,sparse_bt, andsparse_ctxparity, including ties and strided queriesget_gfx_runtime()git diff --check: cleanNot validated in this revision: full-model TTFT/throughput and a post-rebase GSM8K run. This PR's performance claim is limited to the complete production index operator; the output regression tests are bit-exact.