Skip to content

Bump llama.cpp (hrx) 4107043c1 -> 962507f8e: no weight held twice, long or short prompt (#2137) - #2241

Merged
bong-water-water-bong merged 2 commits into
mainfrom
cl-hrxmem/pin-322
Nov 12, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
cl-hrxmem/pin-322

Conversation

@bong-water-water-bong

@bong-water-water-bong bong-water-water-bong commented Nov 12, 2026 •

Copy link
Copy Markdown
Collaborator

Moves the llama.cpp (hrx) pin 4107043c1 -> 962507f8e (gitlink + registry/architectures.json source 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 (pin 228e1f652, 2 READY) to e4a075a: 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; files dispatch-ssm-conv.cpp, dispatch-kquant-decode.cpp, tests/test-hrx-ops.cpp, tests/hrx-backend-test.cpp only, so the registry counts are unchanged):

Measured (at #322's head a591023, against fork head 5816f7b; same build, only libggml-hrx swapped)

Qwen3.8-27B UD-Q4_K_XL, llama-completion -c 4096 -n 48, long prompt, default settings:

before after
device copies 866, 16,181.4 MiB 850, 15,718.5 MiB
weights in two layouts 16 (462.9 MiB) 0

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-ops 267/267, hrx-backend-test host 81/81 + device 12/12; the new ssm_conv arm 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 / 3afe171 libs, same build, only libggml-hrx swapped; 3afe171 has the same tree as 962507f)

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-test host 85/85, and its two tied-weight rows fail on the 87e6330 lib. At 5db8443: device 12/12, MUL_MAT 641/641, MUL_MAT_ID 852/852, test-hrx-ops 268/269. The one failure is flash_attention.accumulator_range.128.1024.v60000 from #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

…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>
@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit e4a075a)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

2137 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • The PR does not include specific measurements or verification of the claimed performance improvements (prefill tok/s, peak GTT, device copies)

Requires further human verification:

  • The claimed performance improvements (prefill tok/s, peak GTT, device copies) need to be verified through actual testing on the hardware

2150 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • The PR does not provide specific verification that the issue has been resolved

Requires further human verification:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Documentation Clarity

The documentation update mentions that the issue is resolved since pin 228e1f652, but does not explicitly state that the specific bug in the draft head's logits step (issue #2150) has been fixed. It would be helpful to clarify whether this issue has been addressed or if it remains a known issue.

**Since pin `228e1f652` the 27B holds no weight twice** (llama.cpp #322). The last 16 were all read by the fused projection+conv
prefill kernel: 15 Q4_K weights it reads only in the packed rows, and one Q6_K weight it bound stored while the rest of prefill reads
it packed. For a Q4_K weight decode already holds stored, that kernel now steps aside (the projection runs on the stored 8-bit-block
kernel and the conv runs separately), and it binds Q6_K in the packed rows where prefill reads that weight packed. Same
`llama-completion` run with `-n 48`, default settings, fork head `5816f7b` against `a591023`:

| Qwen3.8-27B UD-Q4_K_XL | before #322 | since #322 |
|---|---:|---:|
| device copies | 866, 16,181.4 MiB | 850, 15,718.5 MiB |
| weights in two layouts | 16 (462.9 MiB) | 0 |

The text is identical. Over six interleaved arms (trace off), prompt eval was 320.9, 308.4 and 306.2 tok/s before against 328.5,
323.3 and 306.0 since, and decode 10.86 to 10.91 against 10.86 to 11.01: not slower, no gain claimed. `GGML_HRX_Q4K_PACKED_TAIL=1`
(or `GGML_HRX_PREFILL_FOLLOWS_DECODE=0`) restores the fused kernel for Q4_K, `GGML_HRX_Q6K_PACKED_ROWS=0` the stored Q6_K binding.
Before either, making decode step aside instead was measured and does not help:
the same duplicates, and decode 8.5 % slower. flint UD-Q4_K_XL (routed experts) reads every weight stored and holds one copy per
weight with or without the default.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 6c06868 — the bump is auditable per commit, and the control isolates the library.

Three commits, each named, and one of them is the test I reviewed an hour ago

compare 4107043c1...228e1f652: 3 ahead, 0 behind
  6dac7e9  llama.cpp#322: the fused conv4 prefill shares the weight copy prefill already reads  (engine #2137)
  5816f7b  llama.cpp#321: the shortlist sequence expectation stands down when the route is declined
  228e1f6  tests: the attention accumulator bound

228e1f6 is #323 — the exact, model-free accumulator oracle (Q = 0, K = 0 ⇒ every softmax weight exactly 1 ⇒ output exactly 100, against a n_ctx × max|V| = 102,400 ceiling past f16's 65,520). So this bump carries both halves of the night's work: the fix for #2137 and the arm that pins the accumulator bound. A pin bump that says which commits it brings, mapped to the PRs that reviewed them, is the difference between a version change and a change nobody can audit.

Both pins move

"gitlink + registry/architectures.json source sha" — the submodule and the registry's record of it. A bump that moves one and not the other leaves the registry describing a tree that no longer exists, which is exactly the class the pin checks were built for (#2202). Doing both in one PR is right.

And the measurement isolates the library

"same build, only libggml-hrx swapped" — the only control that separates a library bump's effect from everything else in the build. Any other comparison mixes the bump with whatever else changed.

One ask

0 behind is the claim to keep explicit: forward-only is what makes this a bump rather than a merge, and compare is the evidence. If the fork ever gains a commit that is not on main's lineage, the same command will show behind > 0 and the bump becomes a rebase — say that the command is the gate, so the next person runs it rather than assuming.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Pre-checked the one thing this bump could break, and it does not break it: the f16 narrowing ratchet passes at 228e1f65 with no baseline edit.

#2231 (merged) added a ctest ratchet with two committed baselines, and its known fragility is that a baseline names the bodies in the pinned corpus — so a pin bump that adds or fixes one of them turns this PR red and needs a two-line edit. Checked before you find out the hard way:

git diff --stat 4107043c1 228e1f65 -- ggml/src/ggml-hrx/kernel-corpus/     →  no output (no corpus change at all)

at 228e1f65:  51 narrowing bodies, 7 accumulator bodies
baseline    :  51 narrowing bodies, 7 accumulator bodies
  NEW narrowing / STALE narrowing / NEW accumulator / STALE accumulator  →  none, in all four directions

So: the corpus is byte-identical between the two pins for these purposes, the two body lists match exactly, and hrx_f16_narrowing_check + hrx_f16_narrowing will pass as they are. (I extracted the corpus at 228e1f65 with git archive and ran the tool's own narrowing_bodies()/accumulator_bodies() against it rather than reasoning about the diff.)

And one thing this bump carries that is worth knowing: 228e1f65 is the merge of llama.cpp#323 — two flash_attention.accumulator_range.* rows (Q=0/K=0 so every softmax weight is exactly 1; V constant) that pin attention's output accumulator to max|V| rather than to the context length. They were written to refute a mechanism on #1536, and they pass; they stay because the per-block normalisation they measure is a multiply that a future reader can easily mistake for redundant.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 6c06868 — fresh configure rc 0, the pin verified independently (compare/4107043c1...228e1f652 → ahead 3, behind 0), and the three commits are exactly the three the body lists.

This is the cleanest bump shape I have reviewed tonight, because it says what makes it safe to be small: the three commits touch dispatch-ssm-conv.cpp and tests/test-hrx-ops.cpp only, so "the registry counts are unchanged" is a claim a reviewer can check from the file list rather than take on trust — and it is the reason registry/architectures.json here changes only the source sha, not any count.

The behaviour commit is named and its blast radius is stated: 6dac7e9 — the fused conv4 prefill shares the weight copy prefill already reads — is a prefill path change for the SSM/conv route, i.e. it can move TTFT on SSM models and nothing on the transformer routes. The other two are tests (5816f7b shortlist sequence, 228e1f652 attention accumulator bounded by max|V|). For a bump that closes the last part of engine#2137, saying "one behaviour commit, on the conv4 prefill" is exactly the attribution a later bisect needs.

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 4107043c1; any SSM/conv prefill number in the tree is from before this commit, so either it gets re-measured or the doc should say the row is stale. One line, and it is the difference between a bump and a silent invalidation.

Scope of my check: fresh configure rc 0, the fork compare (direction, count, subjects), main's pin, and the two hunks (gitlink + the source sha in registry/architectures.json). No build at the pin and no device: the conv4 change's effect is the fork commit's own.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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>
@bong-water-water-bong bong-water-water-bong changed the title Bump llama.cpp (hrx) 4107043c1 -> 228e1f652: the 27B holds no weight twice (#2137) Bump llama.cpp (hrx) 4107043c1 -> 962507f8e: no weight held twice, long or short prompt (#2137) Nov 12, 2026
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit e4a075a

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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;   // <- new

That 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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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
bong-water-water-bong merged commit c91988d into main Nov 12, 2026
12 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the cl-hrxmem/pin-322 branch November 12, 2026 05:42

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Answering the tree-proof ask in the 05:31 review, now that this has merged. a591023 (the #322 head the long-prompt block was measured at) has the same tree as 6dac7e9, the #322 squash on the pin's line: git diff --stat a591023dc 6dac7e9 is empty, and both trees are 1b4e12634116. From 6dac7e9 to the pin 962507f8e, only dispatch-kquant-decode.cpp and two test files change. That matcher returns early above 8 tokens (kMaxTokens), so long-prompt prefill takes the same path at the pin. Both blocks therefore apply to the bytes this pin ships.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hrx: after a long prompt each weight still has two device copies, because prefill reads q4_k/q6_k in the stored layout (remainder of #856)

1 participant