vulkan : copy strided f16 KV once on AMD so reads spread across memory channels - #27703
vulkan : copy strided f16 KV once on AMD so reads spread across memory channels#27703Nathanw1014 wants to merge 4 commits into
Conversation
Assisted-by: Claude (Opus 4.8 / Opus 5)
Assisted-by: Claude (Opus 4.8 / Opus 5)
Assisted-by: Claude (Opus 4.8 / Opus 5)
|
This seems fine. Might need broader testing to make sure it doesn't regress other AMD devices. Do you know what Qwen3.6-35B-A3B UD-Q4_K_XL didn't benefit? |
0cc4m
left a comment
There was a problem hiding this comment.
Looks good overall and I can confirm the performance improvement. Great work.
| layout(local_size_x = 256, local_size_y = 1, local_size_z = 1) in; | ||
|
|
||
| layout (binding = 0) readonly buffer A {f16vec4 data_a[];}; | ||
| layout (binding = 1) writeonly buffer D {f16vec4 data_b[];}; |
There was a problem hiding this comment.
Since you're just copying memory you can probably just use regular ivec4. You don't need float16 explicit arithmetic types here.
| } | ||
| // Strided-copy counterpart for f16 KV (same scratch layout, no dequant). | ||
| if (tname == "f16") { | ||
| string_to_spv("dequant_f16_transpose", "dequant_f16_transpose.comp", {}); |
There was a problem hiding this comment.
I don't think dequant is right, it's just a transpose copy shader. It's not immediately f16-specific either, e.g. bf16 kv could be handled by the same shader. It could also be written in a generic way to handle copies of any (reasonable) width.
There was a problem hiding this comment.
I had also thought about commenting that this could just use the normal copy shader, no new shader needed. But this dedicated shader might perform a bit better? I'm not sure it will, it may be worth doing the experiment of just using the normal copy shader.
There was a problem hiding this comment.
Thanks, the existing copy shader had the same performance as a dedicated shader.
|
Does this effort cover bf16 as well? |
The f16 clause needs only a strided copy, not a dequant, so drop the dedicated shader and dispatch the existing copy pipeline instead. The copy is described in source memory order so the strided access lands on the writes, which stream, instead of the reads, which would concentrate on a few memory channels. Assisted-by: Claude (Opus 4.8 / Opus 5 / Fable 5)
|
No, but there probably isn't a reason why it couldn't. |
|
@Nathanw1014 Please rebase to fix the test conflict and check bf16. |
Overview
f16 KV layout does not engage all memory channels. This PR engages all channels by copying each layers f16 KV once into a scratch so that the KV cache reads are sequential. Reuse of scratch introduced in PR #25494
Additional information
Gating set to AMD where effect was measured, RTX 3070 did not benefit, Intel and any other vendor have not been tested.
Consecutive KV rows of one head are read
n_kv_heads × head_dim × 2bytes apart. Typical model KV geometry spacing is an even multiple of the 256B channel-interleave block that Mesa documents, causing only a subset of memory channels to engage.The tradeoff is scratch size, 1 layer of KV
TG is measured unaffected, stridding tax increases at depth. Worst case model (hybrid, 10/40 attention layers) regress ~1% in prefill @ 32k, back within run variance @ 64k.
All runs:
llama-bench -m <model> -fa 1 -p 512 -n 0 -d 0,16384,32768,65536 -r 1on Radeon 8060S (RADV STRIX_HALO), f16 KV. Stock = master 3af988f.Qwen3-Coder-30B-A3B UD-Q6_K_XL - stock:
Qwen3-Coder-30B-A3B UD-Q6_K_XL - patched:
Qwen3.8-27B Q8_0 - stock:
Qwen3.8-27B Q8_0 - patched:
Qwen3.6-35B-A3B UD-Q4_K_XL - stock:
Qwen3.6-35B-A3B UD-Q4_K_XL - patched:
Requirements
Assisted with benchmarking, analysis, and review; design + implementation directed by me