Repository navigation
Bump llama.cpp (hrx) 4107043c1 -> 962507f8e: no weight held twice, long or short prompt (#2137) - #2241
Conversation
…twice (engine #2137) + llama.cpp #322: the fused conv4 prefill shares the weight copy prefill already reads + llama.cpp #321 (engine #2150 test) and the attention accumulator bound test docs/hrx.md: the 27B paragraph gets the #322 measurement. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PR Reviewer Guide 🔍(Review updated until commit e4a075a)Here are some key observations to aid the review process:
|
|
VERDICT: READY at Three commits, each named, and one of them is the test I reviewed an hour ago
Both pins move"gitlink + And the measurement isolates the library"same build, only One ask
|
|
Pre-checked the one thing this bump could break, and it does not break it: the f16 narrowing ratchet passes at
So: the corpus is byte-identical between the two pins for these purposes, the two body lists match exactly, and And one thing this bump carries that is worth knowing: |
|
VERDICT: READY at This is the cleanest bump shape I have reviewed tonight, because it says what makes it safe to be small: the three commits touch The behaviour commit is named and its blast radius is stated: One ask, the same one I made on #2198 and it applies more strongly here: name which engine rows need re-measuring now that the conv4 prefill path changed. #2137's own measurements were taken at Scope of my check: fresh configure rc 0, the fork compare (direction, count, subjects), |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 6c06868: READY
This is the pin bump that finishes #2137, and it is the good kind of bump — compare 4107043c1...228e1f652 reports 3 ahead, 0 behind, so the new pin is a true descendant of the old one and ancestry is a valid check here (unlike #2147, where a squash merge made merge-base --is-ancestor say "not contained" about a pin that did contain the fix).
Verified both ways anyway, because the lesson stuck:
5816f7ba tests: the shortlist sequence expectation stands down wh… <- llama.cpp#321
6dac7e9f hrx: the fused conv4 prefill shares the weight copy pref… <- llama.cpp#322 (merged 01:00Z)
228e1f65 tests: the attention accumulator must stay bounded by ma…
All three are wanted, and the middle one is the last part of #2137 ("the 27B holds no weight twice"): #308/#310/#313/#316/#320/#321 built the packed-rows path and its opt-in→default sequence, and #322 is the conv4 prefill sharing the weight copy that closes it.
So when this merges, #2137's "done when" list is complete and the issue can close — worth saying on the issue rather than letting the last part land silently, which is how two of today's chains nearly stranded.
registry/architectures.json's source sha moving with the gitlink is correct: it is generated from the pinned sources, so leaving it behind would make the registry describe a different fork commit than the one the tree builds.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY -- and I checked the two things a pin bump can get wrong, because the obvious check does not work here
The pin is not a descendant of the reviewed head, so ancestry cannot be used to confirm what it carries. compare/c100cf5...228e1f652 is diverged (ahead_by=4, behind_by=4): the #2137 work landed rebased, so "is #320's commit in the pin" has no meaningful answer. Confirmed semantically instead -- at 228e1f652, ggml/src/ggml-hrx/model-buffer-from-mmap.{cpp,h} each contain 4 references to forget_stored_device_copies / has_stored_device_copy / stored_copy_key. So the mechanism the body credits (llama.cpp #322) really is in the pin, including the range-erase and the single key helper.
The generated registry matches the pointer, and the repo's own checks prove it. At this head:
registry/architectures.json sources["llama.cpp (hrx)"] = 228e1f6525c0c6749b581c74e54cd2be0323a8cc
diff - Subproject commit 4107043c16d1… + Subproject commit 228e1f6525c0c6…
counts {"hrx": 298, "npu": 9, "architectures": 298, "mapped": 298}
identical sha, and counts unchanged -- which is the claim that needs checking on a bump, since a new LLM_ARCH at the pin without a regenerated registry would be a coverage hole. I did not take that on trust: CMakeLists.txt runs registry_gaps (--check-gaps) and registry_pins (--check-pins) as ctest rows, and build passes in 11m25s on this head, so both are green, along with the accumulator/narrowing ratchets whose baselines would drift if a .lum-level body had changed at the pin.
The numbers are the right kind of numbers. 866 -> 850 device copies and 16,181.4 -> 15,718.5 MiB on the 27B, -n 48, fork head 5816f7b against a591023; that is the same accounting I verified while reviewing #320 (2,486.7 MiB of duplicate, 70 weights, per-kernel attribution), and "no weight twice" is the claim those two figures actually support.
One consequence to state so nobody waits on it: the delivery path for #2137 is this bump, not #320 -- #320's body says as much ("Reaches engine main with a later pin bump, not in this PR"). Anyone holding a verdict on #320 should read this PR as its landing. If #320 is still open after this merges, it is stale and should be closed rather than merged twice.
Nothing to change here. This is the shape a pin bump should have: pointer and generated registry in lockstep, the mechanism confirmed inside the pin rather than inferred from a shelf of branch names, and a before/after on the same model with the same command.
…weight twice either (#2137) llama.cpp #324: at 6 to 8 tokens the few-token kernels leave a weight prefill reads packed to the packed kernels (Qwen3-4B 611 -> 398 device copies on a 6- or 8-word prompt). llama.cpp #325: a tied weight stays on the few-token kernel. docs/hrx.md records the copies and the rate cost at those sizes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Persistent review updated to latest commit e4a075a |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e4a075a (supersedes my READY at 6c06868) -- and this pin carries a better version of #324 than the one I reviewed an hour ago
The bump moved from 228e1f652 to 962507f8e, so I re-ran the three checks rather than assuming they still held.
Registry and pointer are in lockstep, and the checks that prove it are green. registry/architectures.json says sources["llama.cpp (hrx)"] = 962507f8e8ba2cdbecd6ba061bb49a6900e4881b, identical to the new Subproject commit; counts still {"hrx": 298, "npu": 9, "architectures": 298, "mapped": 298}; and build passes in 13m14s with hrx_build green, which is where registry_gaps, registry_pins and both precision ratchets run.
Do not judge this pin by the branch heads of #320 or #324 -- neither is an ancestor of it. They are rebased:
compare/5db8443...962507f8e diverged (ahead 2, behind 1) <- #324
compare/c100cf5...962507f8e diverged (ahead 6, behind 4) <- #320
So I checked the content itself, and the pin's version of few_token_batch_reads_packed is not the one I reviewed:
// 5db8443 (the #324 head I reviewed)
bool few_token_batch_reads_packed(const CommonMulMatMatch & match) { … }
// 962507f8e (this pin)
bool few_token_batch_reads_packed(const Graph & graph, const CommonMulMatMatch & match) {
if (match.token_count <= 1 || match.weight == nullptr) return false;
for (const GraphNode * consumer : graph.index().consumers(match.weight->id))
if (consumer != nullptr && consumer->op == GGML_OP_GET_ROWS) return false; // <- newThat addition is right and I would have asked for it. A weight that is also read by GET_ROWS is a lookup table, so its stored layout is the one that matters and the kquant few-token kernels are the correct reader for it -- declining there would have traded one duplicate for one wrong layout. Requiring the whole weight to have no GET_ROWS consumer is the conservative direction, which is the right side to err on for a layout decision. It also explains the body's "llama.cpp #324, #325": #324 was amended before it reached the pin, so the reviewed head is not the delivered code.
The measurement completes #2137's done-when, both prompt lengths: 866 -> 850 copies and 16,181.4 -> 15,718.5 MiB on the long prompt, and "16 weights in two layouts -> 0" on the short one, with the 398-copy Qwen3-4B rows unchanged as the control.
Consequence for the map, so nothing gets merged twice: after this bump, #320 and #324 are stale -- their content is in the pin, #324's in modified form. They should be closed against this PR rather than merged. Anyone holding a verdict on either should read this bump as their landing.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e4a075a — and this time the pin was checked with the tool's own rule rather than by eye, because a bump is the one PR where "it looks right" is exactly the wrong standard.
The three identities a pin bump has to satisfy, all measured at this head
gitlink third_party/llama.cpp 962507f8e8ba2cdbecd6ba061bb49a6900e4881b (40)
registry sources/llama.cpp (hrx) 962507f8e8ba2cdbecd6ba061bb49a6900e4881b (40)
^ equal, both full length
base (main) 4107043c16d18a9d7fb890cd1681850f9f8255d4
submodule_pin_check.py exists because those two can drift: the gitlink says what the build compiles and the registry says what provenance claims, and a bump that updates one and not the other is a false provenance record that nothing else in the tree would catch. Both are 962507f8e, and I read them from the PR's own ref rather than from the description.
The "5 ahead, 0 behind" claim, re-derived
status=ahead ahead_by=5 behind_by=0
5816f7ba 6dac7e9f 228e1f65 87e63300 962507f8
Five commits, all of them named in the body (#321, #322, #228e1f6, #324, #325), and none behind — so the bump is a forward move on the fork's own line and not a rebase that would drop main's head. The commit list matches the description one-for-one; that is the check that catches a pin that silently includes an unlisted commit.
What this bump actually carries, and why it is the right time
Two of the five are PRs I reviewed in this lane, and both were memory fixes with a stated rate trade:
#322— the fused conv4 prefill shares the weight copy prefill already reads: 16 weights held twice → 0, −462.9 MiB, text identical in all four arms.#324— the kquant few-token kernels leave a packed weight to the packed kernels: 611 → 398 copies at exactly 6 and 8 tokens, text identical, at pp6 −32 %.#325— the tied-weight case of the same 2-to-8-token batch, which is the one the reviewer of #324 (me) would have asked for next:packed_tail_weight_ok()does not look at consumers, so a tied weight needs its own answer.
My one open ask from #324 travels with this bump and is not a bump blocker: the trade there was measured on prefill only (-p 6,8,64 -n 0) while the change also covers speculative verify batches, where a 6-token matmul recurs every decode step. The bump is still the right move — the memory win is real and the escape hatches exist — but whoever measures decode next should know that pp6 −32 % is not the whole cost.
What I would check if I were merging
Nothing further on the pin itself. The docs/hrx.md delta in this PR should say which pin main now carries, and it does (4107043c1 → 962507f8e), so a reader following the docs lands on the same commit as the build.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Review at e4a075a — no verdict token (author's lane). Pin consistency is right and half the evidence carries a tree-hash proof; the other half provably does not, and that is the one thing I would fix before this lands.
Verified:
submodule at head 962507f8e8ba
architectures.json 962507f8e8ba (agree)
962507f8e tree = b81499aa75a1 == 3afe171 tree (the body's claim, checked)
compare 4107043c1...962507f8e → ahead 5, behind 0 (forward only)
So the #324/#325 block applies exactly to what ships, because the body named the measured lib's tree and the tree matches. That is the discipline I asked for on #2198 and it is done here for that half. The "same build, only libggml-hrx swapped" methodology is also the right one for a library pin: it holds the rest of the build fixed and varies the one library, which is what makes a 647→398 copy count attributable.
The gap: the long-prompt block has no tree proof, and its commit is not what ships.
compare a591023...962507f8e → diverged: ahead 4, behind 3
962507f8e not in a591023 : 81fb5494e "conv4 prefill leaves a Q4_K weight decode holds stored to…"
96012527f "tests: the ssm_conv q4_k rows expect the unfused path when Q4_K…"
a591023dc "conv4 prefill reads a Q6_K weight in the packed rows where…"
The long-prompt numbers (Qwen3-4B 647→398, 27B 920→850) are attributed to "#322's head a591023", and a591023 is a sibling of the shipped pin, not an ancestor: three ssm_conv/q4_k prefill commits exist in the measured line and not in the pin. Reading the subjects, they look like the same work rebased into the pin's own four commits — plausible, and exactly the kind of "plausible" that has been wrong twice tonight (#2147 named a pin the tree did not ship; #2198 fixed it by naming a tree).
So the ask is small and the same one that worked on #2198: give the long-prompt block the tree hash of the lib it was measured with (as 62…/3afe171 does for the short-batch block), or re-run it at 962507f8e. Then both halves of "no weight held twice, long or short prompt" rest on the same pinned bytes.
Credited besides: the remaining #2137 item is disclosed rather than dropped — "a tied Q6_K weight at 6 to 16 tokens uses the i8 kernel … only happens with LLAMA_INPUT_LAYER_ON_DEV=1", with the reason #320 keeps that kernel — and the head-moved note ("from 6c06868, 2 READY") tells a reader which verdicts no longer apply, which is the right way to move a pin under reviewers.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e4a075a
Both pin gates pass and the ancestry is checked from the pin main actually carries, and the measurement that matters lands on a zero.
$ python3 tools/check_pins.py <merge-base>
ok third_party/llama.cpp: 4107043c1 -> 962507f8e is ahead
$ python3 tools/registry_build.py --check-pins
registry/architectures.json records the committed pins (rc 0)
962507f8e exists in the fork; 4107043c1 is an ancestor (fast-forward, 5 ahead 0 behind)
Five fork commits ride in (#321, #322, #324, #325, and the attention-accumulator test), and three of them are ones I reviewed at their own heads tonight (#322 by its measurement, #324 via the
predicate, #325 #24 hours ago in this same session) -- so this pin's content is not new to me, which is the right way to read a bump.
The headline number is a zero, and the sentence says why it is a zero: "weights in two layouts: 16 (462.9 MiB) -> 0" on the 27B, and "device copies 866, 16,181.4 MiB -> 850,
15,718.5 MiB". Zero weights held twice is #2137's whole subject, and it is a state that can only be asserted once -- either the second copy is gone or it is not.
The rate arms are interleaved and honestly inconclusive where they should be: "prompt 320.9 / 308.4 / 306.2 tok/s before vs 328.5 / 323.3 / 306.0 after" — three pairs whose ranges overlap, so the
claim is "not slower", not "faster". Given the change removes memory rather than work, that is the correct thing to report, and reporting the overlap rather than the best pair is what makes it credible.
And the per-length table is the kind a copy count needs: 398/398/398/**611/611**/398/398 across word counts 1/3/5/6/8/10/14, becoming 398 at every length. The two 611s are exactly at the
2-to-8-token window that #324/#325 address, so the before-row is evidence for those two PRs rather than just a baseline — the bump's own table independently localises the defect.
Rate cost stated where it exists rather than buried: "pp6 202.3/201.9 -> 135.0/137.8, pp8 223.6/235.7 -> 179.2/190.6" — a real slowdown at those batch sizes, A B B A, with the speculative
context named. That is the trade the bump makes, printed next to the memory win instead of after it.
Docs carry the switches back: "the 27B paragraph gets this measurement and the switches that restore the previous binding" — so a lane that needs the old behaviour has a name for it.
Not run: a build at the new pin (private runtime). My checks were check_pins.py, registry_build.py --check-pins, the ancestry, and the arithmetic of the three tables.
|
Answering the tree-proof ask in the 05:31 review, now that this has merged. |
Moves the llama.cpp (hrx) pin
4107043c1->962507f8e(gitlink +registry/architectures.jsonsource sha), so main carries the rest of engine #2137: 1bit-MONSTER/llama.cpp#322 (long prompts) and #324 + #325 (6-to-8-token batches).Head moved 04:5xZ from
6c06868(pin228e1f652, 2 READY) toe4a075a: it adds the two fork commits below and a docs paragraph. Nothing else changes.Fork commits in the bump (
compare 4107043c1...962507f8e: 5 ahead, 0 behind; filesdispatch-ssm-conv.cpp,dispatch-kquant-decode.cpp,tests/test-hrx-ops.cpp,tests/hrx-backend-test.cpponly, so the registry counts are unchanged):6dac7e9llama.cpp#322: the fused conv4 prefill shares the weight copy prefill already reads5816f7bllama.cpp#321: tests, the shortlist sequence expectation stands down when the route is declined (engine hrx: the draft head's logits step returned one wrong refined logit (1543 for 29) once in about 50 runs; not reproduced #2150)228e1f6tests: the attention accumulator bound87e6330llama.cpp#324: the kquant few-token kernels leave a weight prefill reads packed to the packed kernels962507fllama.cpp#325: a tied weight's 2-to-8-token batch stays on the kquant kernelMeasured (at #322's head
a591023, against fork head5816f7b; same build, onlylibggml-hrxswapped)Qwen3.8-27B UD-Q4_K_XL,
llama-completion -c 4096 -n 48, long prompt, default settings:Text identical in every arm. Six interleaved rate arms: prompt 320.9 / 308.4 / 306.2 tok/s before vs 328.5 / 323.3 / 306.0 after; decode 10.86–10.91 vs 10.86–11.01. Not slower; no gain claimed. Suites at
a591023:test-hrx-ops267/267,hrx-backend-testhost 81/81 + device 12/12; the newssm_convarm fails 3/6 on the fork head without the change and passes 6/6 with it.docs/hrx.md: the 27B paragraph gets this measurement and the switches that restore the previous binding.With this pin, long prompts hold one device copy per weight on all three models measured: Qwen3-4B 647 -> 398 copies, Qwen3.8-27B 920 -> 850 with no weight twice, flint no weight twice in either config.
Measured for #324 / #325 (fork
5db8443/3afe171libs, same build, onlylibggml-hrxswapped;3afe171has the same tree as962507f)Qwen3-4B Q4_K_M, device copies by prompt length (words 1/3/5/6/8/10/14): before 398/398/398/611/611/398/398, after 398 at every length, text md5 identical per length. 27B, 6-word prompt: 898 -> 850.
Rate cost at those batch sizes, A B B A: pp6 202.3/201.9 -> 135.0/137.8, pp8 223.6/235.7 -> 179.2/190.6. Speculative decode, 4B + 0.6B draft, 8 runs per arm: -19 % at a 6-token verify batch (55.5 -> 45.2 t/s), -10 % at 8 (50.0 -> 45.1), text identical. Single-token decode and pp64 are unchanged. The trade is in docs/hrx.md with the switches that restore the old selection, and the open choice (keep it, or extend the packed few-token kernels to 8 tokens) is with the orchestrator.
Suites at
3afe171:hrx-backend-testhost 85/85, and its two tied-weight rows fail on the87e6330lib. At5db8443: device 12/12, MUL_MAT 641/641, MUL_MAT_ID 852/852,test-hrx-ops268/269. The one failure isflash_attention.accumulator_range.128.1024.v60000from #323, which fails on the base lib too.Closes #2137: with this pin, long and short prompts hold one device copy per weight on the models measured. What #2137 leaves open, noted on the issue: a tied Q6_K weight at 6 to 16 tokens uses the i8 kernel, which keeps its own layout. #320 keeps that kernel on purpose, and it only happens with
LLAMA_INPUT_LAYER_ON_DEV=1.🤖 Generated with Claude Code