Skip to content

kv-cache : keep rotation constants on the GPU - #31073

Merged
ggerganov merged 7 commits into
ggml-org:masterfrom
anton-iermolenko:perf/kv-rotation-resident
Oct 30, 2026
Merged

ggerganov merged 7 commits into
ggml-org:masterfrom
anton-iermolenko:perf/kv-rotation-resident

Conversation

@anton-iermolenko

Copy link
Copy Markdown
Contributor

Overview

Implementation of TODO to retain Haramard static matrices resident on GPU vs current host trips. I have RTX 3090 and GTX 1080 connected to an old laptop with PCIe 2.0 link, hence searched for opportunities to limit host <> GPU transfers with llama.cpp. It did deliver improvements, hence PR and retained for my local fork.

Additional information

Compared against master at edc353842c1b on Linux, RTX 3090, i7-2720QM with four threads, Qwen3.5 9B Q4_K_M, Q8_0 K/V, flash attention, batch 512, and microbatch 128. Both builds use --load-mode none and full GPU offload.

llama-bench was run in baseline/candidate/candidate/baseline order, with five repetitions per process. Means across ten samples per build:

Workload Master, tokens/s This change, tokens/s Change
pp512 2228.14 2216.04 -0.54%
tg128 71.23 77.43 +8.70%

WikiText-2 perplexity over four 512-token chunks is 7.5811 +/- 0.61829 for both builds. Two greedy completions of 128 tokens each return identical text. These are local checks on one model and one machine.

A separate CUDA API trace of a small Llama pp64/tg8 run counts 22 rotation-matrix upload calls on master and four with this change. The latter are the initial K/V uploads for two contexts.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES. I reviewed the code prepared for this PR and it reads straightforward. However, not an expert in llama.cpp architecture, so appreciate any feedback. Codex was used to locate this potential performance improvement opportunity and implement/test the fix. After that change was done locally, I used llama.cpp with Qwen for several weeks.

@github-actions github-actions Bot added the testing Everything test related label Oct 24, 2026
@ggerganov ggerganov self-assigned this Oct 24, 2026
@am17an

am17an commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

auto review:

Thanks, this looks correct to me. One more point in its favour: CUDA, Metal, Vulkan, SYCL and CPU all run these matmuls as a fast Walsh-Hadamard transform (GGML_HINT_SRC0_IS_HADAMARD) and never read src0. So on master, each ubatch does a blocking ~272 KiB host-to-device copy (for head dim 256) of data no kernel reads. On an RTX PRO 6000 with q8_0 KV, I see tg128 +2.4% on Qwen3.5-0.8B and +0.9% on Qwen3.8-27B. pp is unchanged.

Two things:

The test change doesn’t exercise the new path. On CPU CI the KV cache is in a host buffer, so init_device_rotation() returns early and the device copy is never made. On CUDA the FWHT kernel ignores the matrix. I replaced the upload with ggml_backend_tensor_memset(t, 0, 0, ggml_nbytes(t)), and both the rot case and perplexity still pass unchanged. So “clearing the cache changed the rotation constants” can’t fire on either backend. I’d drop that hunk, or keep it as a smoke test and change its description to match.
Partial offload and multi-GPU still use the host path. With partial -ngl, or -sm layer across GPUs, the KV layers sit in different buffer types. The whole cache then falls back, and the matrix is still uploaded every ubatch to each GPU that holds KV layers. These are the setups most limited by PCIe. The graph uses one self_k_rot for every layer, so covering them would need one copy per device, chosen per layer in build_attn. That’s fine as a follow-up, but worth mentioning in the PR description.

@anton-iermolenko

Copy link
Copy Markdown
Contributor Author

Thank you for pointing out the gap in the test. I worked with Codex to rewrite it and executed on GPU with/without the change - clearing out the rotation constants now fails the execution.

As for the partial offload and multi-GPU, left it as TODO comment. Don't plan to address it in this iteration if you agree, it is not my scenario.

@ggerganov

Copy link
Copy Markdown
Member

I'll help with this PR after I merge #31141. I think it's simpler to implement the full support for multiple devices + partial offloading. We just need to keep one set of matrices for each layer. The rotation matrices are small so we can do that for almost for free.

@ggerganov

Copy link
Copy Markdown
Member

@anton-iermolenko Could you rebase on latest master?

If my previous comment makes sense, you can try to do it. Otherwise let me know and I will implement it separately.

@anton-iermolenko

anton-iermolenko commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Yes, let me draft and test the solution for multi-device and partial + rebase on latest master. I do have two GPUs luckily.

@anton-iermolenko
anton-iermolenko force-pushed the perf/kv-rotation-resident branch from a7931ae to 6c0f5f2 Compare October 27, 2026 12:22
@github-actions github-actions Bot added the model Model specific label Oct 27, 2026
@anton-iermolenko
anton-iermolenko force-pushed the perf/kv-rotation-resident branch from 6c0f5f2 to 2f1ca5a Compare October 27, 2026 12:45
@anton-iermolenko

anton-iermolenko commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor Author

Multi-device layer splits and partial CPU/GPU offload are now supported. Rotation matrices are initialized once in init_rotation() and reused across cache clears and graph rebuilds. Added rot_mix unit test to verify mixed CPU/GPU placement and buffer reuse. Tested partial offload on both GPUs individually, CPU plus two GPUs.
Rebased the branch as well as. @ggerganov, please, check.

@ServeurpersoCom
ServeurpersoCom self-requested a review October 29, 2026 17:25
@ServeurpersoCom

ServeurpersoCom commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Needs a rebase on master, I'll do it: mistral-large4.cpp (self_k_rot_lid) and dflash.cpp (self_k_rot_swa) still use the single tensor and no longer build, they just need [il].

I checked the numerics on an RTX PRO 6000: KLD vs an f16 KV cache is bit-identical on LFM2 and Qwen3, and LFM3 gets better with a q4_0 cache (0.0256 to 0.0175) since master never applied the rotation on the hybrid SWA path. LGTM!

@ServeurpersoCom
ServeurpersoCom force-pushed the perf/kv-rotation-resident branch from b5b1afd to 3590d9d Compare October 29, 2026 18:03
@ServeurpersoCom
ServeurpersoCom requested review from a team as code owners October 29, 2026 18:03
@ServeurpersoCom

Copy link
Copy Markdown
Contributor

Rebased

@ggerganov ggerganov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just wondering if it is worth keeping the 2 new tests.

@ggerganov ggerganov added the merge ready A maintainer can use this label to indicate that they consider the changes final and ready to merge. label Oct 29, 2026
@ServeurpersoCom

Copy link
Copy Markdown
Contributor

I'm just wondering if it is worth keeping the 2 new tests.

I'd drop both: they check implementation details (tensor names, buffer placement, the matrix values) rather than behavior, rot_mix even reloads the model with a hand-built partial offload, and since the FWHT kernels never read the matrix they can't catch a numerical regression anyway. The existing state_rotation case plus a KLD check like the one above already cover what matters.

@ggerganov
ggerganov merged commit fb59972 into ggml-org:master Oct 30, 2026
19 of 21 checks passed
mclrcha added a commit to mclrcha/llama.cpp that referenced this pull request Oct 31, 2026
Integrate 54 upstream commits through 0ec4027.

The KV rotation constants now live in the cache buffers (ggml-org#31073), so the
attention inputs no longer upload k_rot/v_rot: keep the fork's
set_input_kq_mask_kv helper and its split prefill mask declarations, and
drop the set_input_k_rot/set_input_v_rot setters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge ready A maintainer can use this label to indicate that they consider the changes final and ready to merge. model Model specific testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants