Repository navigation
[Perf] [AMD] qwen3: Fuse prefill QK-norm + RoPE on ROCm with the JIT qknorm_rope kernel - #47052
fengshaoyi-amd wants to merge 10 commits into
Conversation
…qknorm_rope kernel
|
/tag-and-rerun-ci amd-extra |
BBuf
left a comment
There was a problem hiding this comment.
Please address the [P1] items, then re-ping.
Agent-assisted pre-review, findings verified against 039301ba3897.
| D.set_value(kHeadDim); | ||
| R.set_value(kRopeDim); | ||
| device.set_options<kDLCUDA>(); | ||
| device.set_options<kDLGPU>(); |
There was a problem hiding this comment.
[P1] This also enables Cosmos3's packed-KV path on ROCm: the video path probes pack_kv=True and then consumes the fused prefix/suffix buffers. Neither AMD-registered test here invokes that variant, and its existing tests are CUDA-only. Please add AMD reference coverage for packed K/V, or keep this variant gated off on HIP.
There was a problem hiding this comment.
Kept off on HIP. Your 98cbb77c rejects KV packing there; 86343f05d3 moves that check into _unvalidated_on_hip, which can_use_fused_inplace_qknorm_rope (returns False) and the direct JIT call (raises NotImplementedError) share, and test_kernel_gate_rejects_unvalidated_variants pins it on AMD CI. So on ROCm Cosmos3 video never takes the packed-KV path. After the merge with main, the kDLGPU line this comment is on comes from #47300 and is no longer part of this diff.
There was a problem hiding this comment.
[BBuf Review Agent] Checked this head: packed KV is blocked on HIP in both the capability check and direct JIT calls. This addresses the packed-KV concern.
| num_tokens, num_q_heads, num_kv_heads, head_dim, dtype, torch.int64 | ||
| ) | ||
| outs = [] | ||
| for half_warp in (True, False): |
There was a problem hiding this comment.
[P2] Non-blocking: kUseHalfWarp is hardcoded false under USE_ROCM, so both iterations launch the full-warp kernel. This does not validate the claimed half-warp shuffle behavior; the newly AMD-registered out-of-place variant comparisons have the same issue. Please remove these redundant HIP comparisons or replace them with independent-reference coverage.
There was a problem hiding this comment.
Removed test_half_warp_matches_full_warp, and test_two_rows_per_warp_match_one_row_per_warp in the out-of-place file now skips on HIP. On HIP the shuffles stay covered by test_matches_unfused_path against an fp64 reference, and the out-of-place kernel by test_out_of_place_qknorm_rope_matches_inplace_and_keeps_inputs, bit for bit against the in-place one.
There was a problem hiding this comment.
[BBuf Review Agent] Confirmed the redundant HIP comparisons are removed or skipped, and the enabled path keeps the fp64 reference checks. This resolves the P2.
|
@fengshaoyi-amd I pushed Automated pre-review for |
BBuf
left a comment
There was a problem hiding this comment.
Please address the [P1] items, then re-ping.
Agent-assisted pre-review, findings verified against 98cbb77c0d70.
| #pragma unroll | ||
| for (uint32_t i = 0; i < kElemsPerThread; ++i) { | ||
| float swapped = __shfl_sync(kActiveMask, elems[i], partner_lane); | ||
| float swapped = shfl_idx32(kActiveMask, elems[i], partner_lane); |
There was a problem hiding this comment.
[P1] Cosmos3 video with T > 1 still reaches this unrounded NeoX path on ROCm: rejecting pack_kv makes it use the in-place fusion. The new numerical tests use round_norm_before_rope=True, while independent coverage for this variant is CUDA-only. Could we gate this specialization on HIP or add AMD numerical and Cosmos3 validation before enabling it?
There was a problem hiding this comment.
Gated on HIP in 86343f05d3. This shuffle is in the unrounded path, and _unvalidated_on_hip now also rejects NeoX with round_norm_before_rope=False, so can_use_fused_inplace_qknorm_rope returns False and a direct call raises NotImplementedError. On ROCm, Cosmos3 video at T > 1 then goes through _apply_qwen3_qk_norm_rope to the separate QK-norm and RoPE ops, as on main. The only NeoX variant enabled on HIP is the rounded one (the shuffle at line 369), which test_qk_norm_rope_hip.py checks against an fp64 reference.
There was a problem hiding this comment.
[BBuf Review Agent] Confirmed the shared gate rejects unrounded NeoX on HIP, and Cosmos3 video with T > 1 falls back to separate QK-norm and RoPE. This addresses the concern.
|
/rerun-test test/registered/kernels/ops/attention/test_qk_norm_rope_hip.py test/registered/kernels/ops/diffusion/test_qknorm_rope_out_of_place.py Targeted validation for |
|
🚀 🚀 ⛔ |
…-rope # Conflicts: # python/sglang/srt/models/qwen3.py
…half-warp checks Cosmos3 video at T > 1 requests the unrounded NeoX in-place variant, which has no AMD numerical coverage. Reject it on HIP next to KV packing, so those callers keep the separate QK-norm and RoPE ops on ROCm. kUseHalfWarp is false under USE_ROCM, so the half-warp comparisons ran the same kernel twice on HIP: remove the one in test_qk_norm_rope_hip.py and skip the out-of-place file's on HIP.
|
@BBuf Pulled AMD CI on this head: jit-kernel-unit-test-amd runs |
|
/rerun-failed-ci |
1 similar comment
|
/rerun-failed-ci |
|
Thanks for rerunning.
The tests for this change pass on both vendors: |
|
/rerun-failed-ci |
|
/rerun-failed-ci |
|
CI triage for bd3d8b0 (this head merged into main dbeecc9). None of these failures come from this PR. Its change only reaches the ROCm callers of the fused QK-norm + RoPE JIT kernel (Qwen3 attention and ERNIE-Image), and none of the failing tests goes through that kernel. Still failing:
Being rerun after runner-side failures:
The tests covering this change pass: |
BBuf
left a comment
There was a problem hiding this comment.
LGTM. Both earlier findings are resolved.
|
/rerun-failed-ci |
|
@BBuf thanks for the approval. Could you merge this when you have a moment? The required checks (
|
|
/rerun-failed-ci |
|
[BBuf Review Agent] @fengshaoyi-amd these CI failures look related to this change, could you take a look?
|
|
@BBuf This failure isn't from this change. It's the ROCm VAE nondeterminism that #46947 fixed, and the reruns here can't pick that fix up.
I'll merge main so the next run includes #46947. |
|
[BBuf Review Agent] @fengshaoyi-amd You're right, Flux3's HIP GPT-J path is unchanged by this diff, so I'm dropping the attribution of this failure to the PR. The current head also includes #46947 after the main merge. |
|
[BBuf Review Agent] Could you rebase this onto the latest main? It conflicts with main now. |
…-rope # Conflicts: # python/sglang/srt/models/qwen3.py
This comment has been minimized.
This comment has been minimized.
|
[BBuf Review Agent] @fengshaoyi-amd Confirmed: the hook flags the ForwardMode import added by #47632, and that file is unchanged by this PR. This lint failure is unrelated to your changes. |
PR Pundit review(Testing PR Pundit Code Review agent. Not all comments may be relevant. This is a one-time automated pass: please address what is useful, no need to request a re-review. Please reply to each point, whether it applies or not: we harvest these replies, and both kinds are enormously helpful for tuning the agent. Reach out to @peymanr for suggestions.)
1. 🟠 Should fix: Fused ROCm qknorm_rope kernel runs on decode despite prefill-only docstrings, untested for decode/graph-replay
Who is affected: ROCm Qwen3 users whose decode does not take the aiter fused path ( Checked against the code by an independent pass. 2. 🟠 Should fix: On ROCm
|
|
@peymanr Thanks. By number:
|
|
[BBuf Review Agent] @fengshaoyi-amd Checked this head: the HIP opt-out is honored and tested, and the docstring now covers decode. The init-time gate and |
PR Pundit review(Testing PR Pundit Code Review agent. Not all comments may be relevant. This is a one-time automated pass: please address what is useful, no need to request a re-review. Please reply to each point, whether it applies or not: we harvest these replies, and both kinds are enormously helpful for tuning the agent. Reach out to @peymanr for suggestions.)
1. 🟠 Should fix: Dumped ROCm server args show enable_fused_qk_norm_rope=None despite Qwen3 fusing: default resolved inline
On ROCm the Who is affected: ROCm users running Qwen3 who inspect logged or dumped server args; runtime behaviour is unaffected. Checked against the code by an independent pass. Related past reviews: #20667 on 2. 🟠 Should fix: Hand-built deterministic Qwen3 on ROCm uses fused kernel not bit-identical to unfused: gate checks only env var
Who is affected: Only ROCm unit tests or tools that build Qwen3 directly with deterministic inference in the exec config but never run Checked against the code by an independent pass. 3. 🟠 Should fix: Duplicated ROCm shuffle wrappers must be kept in sync with warp.cuh: this PR added per-file #ifdef helpers
The new Who is affected: Maintainers of the qknorm_rope JIT kernel; no runtime effect on CUDA or ROCm. Checked against the code by an independent pass. Related past reviews: #4302 on Shared from a pilot of PR Pundit, an agentic code reviewer that builds and tests the change and reads the project's past reviews. Reviewed at |
|
@peymanr Thanks. By number:
|
|
[BBuf Review Agent] @fengshaoyi-amd Keeping the model-specific default inline is fine. The help text explains it, and the ROCm gate honors explicit opt-out. |
…-rope # Conflicts: # test/registered/kernels/ops/diffusion/test_qknorm_rope_out_of_place.py
|
/rerun-failed-ci |
Motivation
On ROCm, Qwen3 prefill runs QK-norm and RoPE as five launches per layer:
apply_qk_normcopies the strided q and k views of the packed qkv to contiguous tensors, runs two aiter RMSNorms, thenrotary_emblaunchesrotary_embedding. The fused path inapply_qk_normis CUDA-only. In a Qwen3-8B prefill profile on MI355X (batch 1/8, input 2048/8192, main7fe5b127e4) these kernels take 4.8% to 6.8% of GPU kernel time.The diffusion JIT kernel
fused_inplace_qknorm_ropealready does this in one in-place launch (strided q/k, GQA, fp32 cos/sin cache table), but it does not compile on HIP: bare__shfl_syncwith a 32-bit mask hits HIP's 64-bit-mask static_assert. So the ROCm path enabled in #35573 silently falls back to the unfused ops; the kernel's tests are registered for CUDA CI only. #40585 fixes the same shuffles for MiniMax-H3; this PR carries an equivalent fix so it does not depend on it. #47001 (open) adds the same in-place fusion for XPU through sgl-kernel-xpu, and #47785 (open) a CUDA prefill fusion; all three changeforward_prepare_native, so whichever lands later needs a small rebase.Modifications
Scope: ROCm only. On HIP the kernel's NeoX variants with
round_norm_before_rope=Truenow build. Two ROCm callers request them: Qwen3 attention, which also serves the models built onQwen3Model(such as Qwen3-Embedding and InternVL) when the gate below accepts them, and ERNIE-Image, which keeps the fused kernel only if its first call matches its reference bit for bit. NeoX withround_norm_before_rope=Falseand KV packing stay off on HIP, so their callers, such as Cosmos3 video, keep the separate ops as on main. GPT-J variants, CUDA, XPU, NPU, and the aiter decode fusion are unchanged.qknorm_rope.cuh: on HIP, shuffle with__shfl/__shfl_xorat width 32 (the kernel's logical warp on wave64). CUDA builds are unchanged.qknorm_rope_jit.py: on HIP,can_use_fused_inplace_qknorm_ropereturns False and a direct call raisesNotImplementedErrorfor the two variants without AMD numerical coverage, KV packing and NeoX withround_norm_before_rope=False.models/utils.py: addcan_use_jit_qk_norm_rope, which gates on HIP,--enable-fused-qk-norm-ropenot set to off, plainRotaryEmbeddingwith an fp32 cache, plainRMSNormweights in the activation dtype, and no deterministic / on-policy / inductor prefill mode, and builds the kernel at init so it is never first built during graph capture; addapply_qk_norm_rope, which runs the kernel in place withround_norm_before_rope=Trueor falls back toapply_qk_norm+rotary_emb.qwen3.py: outside the CUDA decode branch from [Perf] [NVIDIA] qwen3: Reuse fused QK-norm/RoPE kernel for decode #46908,forward_prepare_nativecallsapply_qk_norm_rope. On ROCm it serves prefill / extend, and decode when the aiter decode fusion from [Perf] [AMD] qwen3: Fuse QK-norm + RoPE + KV cache write for dense Qwen3 decode #44267 does not apply.test_qk_norm_rope_hip.py(new, AMD CI): fused vs unfused path on strided views over head/rotary dims, GQA ratios, neox and GPT-J, 0 to 4096 tokens, fp16, int32 positions; the fused result must be no farther from an fp64 reference than the unfused one; the kernel gate rejects the two variants above on HIP; YaRN, mRoPE, eps mismatch, fp32 norm weights, deterministic, on-policy, inductor and--no-enable-fused-qk-norm-ropefall back.test_qknorm_rope_out_of_place.py: register for AMD CI; skip the two-rows-per-warp comparison on HIP, which builds only the one-row-per-warp kernel.Accuracy Tests
MI355X (gfx950), torch 2.11.0+rocm10.0.0, sglang-kernel 0.4.10, aiter
285579b62,SGLANG_USE_AITER=1, decode CUDA graphs on.Qwen/Qwen3-8Bbf16, otherwise default server args. Base is main7fe5b127e4. GSM8K withpython -m sglang.test.run_eval --eval-name gsm8k --num-examples 1319 --temperature 0 --max-tokens 4096(1314 questions after the 5 few-shot examples):The fused kernel reads the fp32 cos/sin table, while the unfused HIP RoPE rounds it to bf16 first, so outputs differ by 1 to 2 ulp and are closer to fp64.
Unit tests, AMD CI (MI300X) on this head: jit-kernel-unit-test-amd runs
test_qk_norm_rope_hip.py(130 passed) andtest_qknorm_rope_out_of_place.py(1 passed, 12 skipped); stage-a-test-1-gpu-small-amd runstest_qwen3_aiter_fused_qk_norm_rope.py(decode path, 6 passed). With main'sqknorm_rope.cuhthe kernel fails to build for head_dim 128 neox, socan_use_jit_qk_norm_ropereturns False andtest_qk_norm_rope_hip.pyfails at its gate assertion.MI325X (gfx942), torch 2.11.0+rocm10.0.0, aiter
285579b62, maincb55d77d37with this patch: GSM8K Qwen3-8B 0.9437 on main vs 0.9475 here.--enable-torch-compilealready fails on main with a memory fault during decode graph capture in the inductor output for aiter's decode opfused_qk_norm_rope_cache_pts_quant_shuffle, which this PR does not touch, so torch.compile is not covered.Speed Tests and Profiling
Microbench of one Qwen3-8B layer on this PR's code,
apply_qk_norm+rotary_embvsapply_qk_norm_rope(use_jit=True)(32 q / 8 kv heads, head_dim 128, bf16, q/k strided views of qkv), CUDA events, median of 50 after 10 warmup:Prefill profile,
python -m sglang.benchmark.one_batch --model-path Qwen/Qwen3-8B --attention-backend aiter --batch-size <B> --input-len <ISL> --output-len 1 --profile --profile-stage prefill, one process per shape, GPU kernel time summed over 36 layers:End to end, Qwen3-8B on one MI355X, with a fresh server for every run:
<N>is 64 at concurrency 4 and 256 at concurrency 16; at concurrency 64 it is 1024 for W2 and 320 for W3. Blocks of 4 runs in the order base, PR, PR, base; the first run of each block is dropped and each block reports the median of runs 2-4. Base and PR are the means of their two blocks; noise is the second base block vs the first.TTFT improves by 2.5% to 6.4% in every workload, above the base-to-base noise of at most 1.1%; throughput gains are 0.6% to 1.9%, and TPOT changes by at most 1.3%.
Checklist
Found by Hyperloom.
CI States
Latest PR Test (Base): ⏳ Run #41993879604
Latest PR Test (Extra): ❌ Run #41993879563
Latest PR Test (AMD ROCm 10): ✅ Run #41993879438
Tip
Base CI only covers the core cases, and its workload may not exercise your change. Use
/rerun-testto run the tests your change touches, or copy this prompt for an agent: