Skip to content

docs: the Zyphra page named the wrong cause for the CPU-side input GET_ROWS (Refs #1316) - #1356

Merged
bong-water-water-bong merged 2 commits into
mainfrom
docs/zyphra-getrows-caveat
Nov 8, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
docs/zyphra-getrows-caveat

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Corrects one paragraph in docs/zyphra-models.md (merged in #1345) that named the wrong cause for the token-embedding GET_ROWS sitting on
the CPU. Engine #1316 measured it and closed it, and my attribution was wrong on both halves:

my page said measured
"after the host-pointer weight import landed" not #226/#856: GGML_HRX_WEIGHT_HOST_PLACEMENT=0 reproduces the placement identically (same node, same counts); #228 only adds a readback prefix for fused matches
implied it was a base-level regression between my two arms it is #170's deliberate default — the input layer stays on the CPU (src/llama-model.cpp, "very little benefit to offloading the input layer"); my clean "before" arm was a base from before #170
"tracked as #1316" #1316 is closed, with LLAMA_INPUT_LAYER_ON_DEV=1 measured as not rescuing it: on a tied embedding it fails with weight value 0 has conflicting resident layout requests (native GET_ROWS + packed output MUL_MAT, one binding per value), and the two possible fixes were left as an owner decision

The replacement paragraph says what a reader can act on: the CPU-side GET_ROWS is a floor for every model at -ngl 99, both Zamba2 forms
sit on it equally, and a 1 of 3024 census with GET_ROWS x 1 and 2 splits is not residue this family's changes added.

Why it matters enough for its own PR: a merged page that names the wrong subsystem sends the next person to read the import code, which is
exactly what #1316 had to disprove by measurement. Docs only, one paragraph; tools/copyright.py --check clean.

Refs #1316

@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1316 - Partially compliant

Compliant requirements:

  • Corrected the documentation to accurately reflect that the CPU-side GET_ROWS placement is by design, not due to the host-pointer weight import
  • Clarified that the issue is related to the input layer staying on CPU unless opted in with LLAMA_INPUT_LAYER_ON_DEV=1
  • Stated that the problem affects all models at -ngl 99 and is not a residue of the changes in this family
  • Confirmed that GGML_HRX_WEIGHT_HOST_PLACEMENT=0 reproduces the same placement behavior
  • Indicated that the issue was measured and closed with specific fixes and costs left as an owner decision

Non-compliant requirements:

  • None

Requires further human verification:

  • None

856 - Partially compliant

Compliant requirements:

  • None

Non-compliant requirements:

Requires further human verification:

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

Documentation Accuracy

The documentation change correctly identifies that the CPU-side GET_ROWS placement is by design and not due to the host-pointer weight import. This is important for users to understand the actual cause of the placement issue, which was incorrectly attributed in the previous version.

One caveat for anyone running `-ngl 99` today: the token-embedding `GET_ROWS` sits on the CPU — **by design, not
because of this family**. The input layer stays on the CPU unless `LLAMA_INPUT_LAYER_ON_DEV=1`
(`src/llama-model.cpp`, #170: "very little benefit to offloading the input layer"), and on a tied embedding that
opt-in currently fails outright with `weight value 0 has conflicting resident layout requests`, since the weight
is read natively by `GET_ROWS` and contracted in a packed layout by the output `MUL_MAT`. Engine **#1316**
measured both halves and closed with the two possible fixes and their costs left as an owner decision: it is
**not** the #856 host-pointer import (`GGML_HRX_WEIGHT_HOST_PLACEMENT=0` reproduces the placement identically),
and the opt-in path has never worked for tied embeddings. So a census of `1 of 3024` nodes with `GET_ROWS x 1`
and 2 graph splits is this family's **floor**, not a residue its changes added — both Zamba2 forms sit on it
equally.

@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 b0b94bcc6a6f34ce5169eefd1087e6bd01c8c24e

A correction PR lives or dies on whether the new attribution is true, so I checked each of its three claims against the pinned fork rather than against the argument:

  • engine #1316 is CLOSED (2026-11-07T18:19:14Z) — so "measured it and closed it" is the issue's own record, and the paragraph is no longer pointing at an open tracker item.
  • the default is deliberate and the opt-in exists: at the current pin (67d8c42ec), src/llama-model.cpp assigns dev_input to the CPU with the comment "there is very little benefit to offloading the input layer, so keep it on the CPU by default", and :1385 reads LLAMA_INPUT_LAYER_ON_DEV, logging input layer follows layer 0 on … when set. The quoted fragment is faithful, and the #170 it attributes it to resolves to fork llama.cpp #170, "loader: an explicit -ot wins over the arch's lazy-read hint; LLAMA_INP…" — the right reference (note the engine's own #170 is an unrelated MoE-router issue, so the bare #170 is only unambiguous because the sentence is about src/llama-model.cpp).
  • the failure it quotes is real and in the tree: conflicting resident layout requests is at the pin in ggml/src/ggml-hrx/dispatch_registration/common/dispatch-mul-mat.cpp:134,811 and dispatch-registry.cpp:178,221, and the latter documents it in almost exactly the doc's words — "weight value 0 has conflicting resident layout requests", llama_decode ret = -3 — plus the explanation the paragraph gives (one value read natively by GET_ROWS and contracted in a packed layout by the output MUL_MAT, one binding per value).

So the replacement paragraph is the measured story, not a different guess: the CPU-side GET_ROWS is #170's default (not the #856 import — which GGML_HRX_WEIGHT_HOST_PLACEMENT=0 rules out by reproducing the placement identically), the 1 of 3024 / GET_ROWS x 1 / 2-splits census is a floor for every model at -ngl 99 rather than residue these two Zamba2 forms added, and the two possible fixes stay an owner decision exactly as #1316 left them.

That is worth its own PR for the reason the body gives, and I would add one: a merged page naming the wrong subsystem is worse than a page saying nothing, because the next reader spends their measurement budget disproving it — which is what #1316 had to do. Docs only, one paragraph; tools/copyright.py --check is the author's claim and the diff touches no code.

@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: CHANGES at b0b94bcc6a6f34ce5169eefd1087e6bd01c8c24e. The correction is right about the cause (not the #856 import, and the CPU placement is #170's deliberate default), but the replacement paragraph is already out of date about the opt-in, which is the part a reader would act on.

What main says today:

  • origin/main:third_party/llama.cpp = 67d8c42e, which is fork #233's merge (compare = identical): "hrx: do not pack a device-resident weight that another node gathers natively (engine #1316)".
  • Engine #1344 (Fixes #1316) merged at 18:19:13Z, and #1316 closed at 18:19:14Z, one second later. So it closed because of that fix, not "with the two possible fixes … left as an owner decision".
  • #1344's measured rows (re-checked in my review there, build a85a6f342, the same tree as 67d8c42e): LLAMA_INPUT_LAYER_ON_DEV=1 + GGML_HRX_STRICT=1 → 0 nodes off HRX, generating, with the conflict now declined in the log. Only GGML_HRX_LAYOUT_CONFLICT_NO_PACK=0 brings back conflicting resident layout requests.

So these sentences are wrong at the pin this page ships with:

  • "on a tied embedding that opt-in currently fails outright with weight value 0 has conflicting resident layout requests"
  • "Engine #1316 … closed with the two possible fixes and their costs left as an owner decision"
  • "the opt-in path has never worked for tied embeddings"

Suggested replacement: the token-embedding GET_ROWS sits on the CPU by default, by design (#170; 1 of 3024, GET_ROWS x 1, 2 splits, the same for both Zamba2 forms, and not the #856 import). LLAMA_INPUT_LAYER_ON_DEV=1 moves it onto HRX, and since #1344 that works for tied embeddings too (0 nodes off HRX). It stays opt-in because device input was ~6 % slower than CPU input on a 0.6B in #1344's measurement. It's worth re-measuring on a Zamba2 before recommending it either way.

@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 b0b94bcc.

An attribution correction is only worth taking if the mechanism it names is in the tree, so I checked both of the page's new claims against the pinned submodule rather than against the issue text:

third_party/llama.cpp/src/llama-model.cpp:1375  // LLAMA_INPUT_LAYER_ON_DEV=1 makes the input layer follow the first repeating layer's device.
third_party/llama.cpp/src/llama-model.cpp:1385  const char * env = std::getenv("LLAMA_INPUT_LAYER_ON_DEV");
third_party/llama.cpp/ggml/src/ggml-hrx/runtime/command-program-executor.cpp:688
        status.log("weight value %d has conflicting resident layout requests", binding.value.value);

So "the input layer stays on the CPU unless LLAMA_INPUT_LAYER_ON_DEV=1 (#170's deliberate default)" and "the opt-in fails on a tied embedding with conflicting resident layout requests" are both real code at the pin, not paraphrase. The retraction is also correctly shaped: it names what it got wrong (the #856 import, and "base-level regression"), what actually caused it (#170's default), and leaves the 1 of 3024 / 2-splits number standing as this family's floor rather than a residue — which is the useful reading for anyone re-measuring Zamba2 later.

Only note: the page now cites GGML_HRX_WEIGHT_HOST_PLACEMENT=0 as evidence, and #1351 (open) deletes that variable. Both are true of their own pin; if #1351 lands first, this sentence becomes history rather than a knob, so it may want a "(at the 67d8c42ec pin)" qualifier.

…T_ROWS (Refs #1316)

The paragraph I added in #1345 said the token-embedding GET_ROWS was on the CPU "after the host-pointer weight
import landed" and filed it under engine #1316. Both halves were measured wrong, and #233 landed a fix while this
PR was open, so the page now says what is true at the pin:

- it is **not** the #856 import: `GGML_HRX_WEIGHT_HOST_PLACEMENT=0` reproduces the placement identically, and #228
  only adds a readback prefix for fused matches;
- the cause is **#170's deliberate default** - the input layer stays on the CPU (`src/llama-model.cpp`, "very
  little benefit to offloading the input layer"); my clean "before" arm was simply a base from before #170;
- `LLAMA_INPUT_LAYER_ON_DEV=1` **works** for a tied embedding since fork #233 (in the pin, 67d8c42ec): a tied
  embedding is gathered by GET_ROWS and contracted in a packed layout by the output MUL_MAT, a program cannot hold
  both, so the dispatch now declines the packed match for a weight another node gathers - measured 0 nodes off HRX
  3/3 with the switch on against a refusal 3/3 with the gate disabled;
- it stays off by default because device input measured ~6% slower than CPU input on a 0.6B, so at default
  settings `1 of 3024` nodes with `GET_ROWS x 1` and 2 splits is the floor for every model at `-ngl 99`, Zamba2
  forms included, and the switch is how you leave it.

Refs #1316
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Head moved to 7adc17d — the paragraph is materially different, not just reworded, because #233 landed while this PR was open and it
answers the question #1316 had left to the owner. I would have merged a stale claim otherwise.

What changed: my first version said the opt-in "currently fails outright with conflicting resident layout requests" and that #1316 closed with
the fix left as an owner decision. Both are now history:

So the sentence a reader now gets is: 1 of 3024 with GET_ROWS x 1 at default settings is the floor for every model at -ngl 99, and
LLAMA_INPUT_LAYER_ON_DEV=1 is how you leave it — with the cost that keeps it off by default. That is the same conclusion #233 measured, which
is the right thing for the family page to agree with.

tools/copyright.py --check clean; docs-only, one paragraph.

@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 7adc17def7c80de1a7a90289c870b183ab6ae7f4. The paragraph now matches the pin this page ships with, which clears my CHANGES at b0b94bcc.

  • Default placement: the CPU-side GET_ROWS is #170's policy (llama-model.cpp, "very little benefit to offloading the input layer"), not this family's and not the #856 import (GGML_HRX_WEIGHT_HOST_PLACEMENT=0 changes nothing).
  • The opt-in works: LLAMA_INPUT_LAYER_ON_DEV=1 is correctly described as working for tied embeddings since fork #233, in the pin (main's third_party/llama.cpp is 67d8c42e, #233's merge). It gives 0 nodes off HRX against a 3/3 refusal with the gate off, as measured in #1344.
  • Why it stays off by default: the ~6 % device-input cost on a 0.6B, which is #1344's own number.
  • The census reading is right: 1 of 3024, GET_ROWS x 1 and 2 splits is the default floor for every model, not residue from these changes.

@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 ec9202a917fb1556d371df8183ce1143db3bc586

The paragraph was rewritten again, and this time it is the good kind of change: it retires my earlier reading because a fix landed under it. The old text said the tied-embedding opt-in "currently fails outright with weight value 0 has conflicting resident layout requests"; the new text says it works since fork #233 (in the pin) — and I checked that in the pinned tree rather than taking it:

pin ed43da0b4d: ggml/src/ggml-hrx/dispatch_registration/common/dispatch-mul-mat.cpp
  weight_has_native_layout_reader   -> present (2 references)
  67d8c42ec  hrx: do not pack a device-resident weight that another node gathers natively (engine #1316) (#233)
src/llama-model.cpp:1375-1390        LLAMA_INPUT_LAYER_ON_DEV default stays CPU, opt-in logs when taken

So the sentence "a command program cannot hold the weight twice, so the dispatch now declines the packed match for a weight another node gathers" is describing code that is in the pin, and the claim "0 nodes off HRX with the switch on, 3/3 runs, against a refusal in 3/3 with that gate disabled" is a falsification pair rather than a single happy path.

The new decision it documents is the one worth having in the page: the opt-in stays off by default because device input measured ~6 % slower than CPU input on a 0.6B while removing one node and one split per graph. That is a measurement, not a preference — and it is the kind of sentence that stops the next person from "fixing" the default because a graph on-device looks tidier.

The floor statement survives correctly: at default settings 1 of 3024 nodes with GET_ROWS x 1 and 2 graph splits is the floor for every model at -ngl 99, both Zamba2 forms equally — which is what the original caveat was reaching for and now states without the stale failure mode.

Not re-run: the 3/3 + 3/3 device runs and the ~6 % figure (they need the array and the 0.6B). What I could check — that #233 is in the pin, that the dispatch gate exists there, and that the default is still CPU — is green.

@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: CHANGES at ec9202a9 — one clause, and it is the same class of defect this page is being fixed for.

The revision itself is an improvement and I verified it. Written against the pin that is actually on main now:

third_party/llama.cpp @ ed43da0b4    (main's pin, post-#1351)
  parent: 67d8c42ec                 -> fork #233 ("do not pack a device-resident weight that another node gathers
                                       natively") IS in the pin, as the new text claims
  grep -rn GGML_HRX_WEIGHT_HOST_PLACEMENT .   ->   0 hits

So the new paragraph is right on both counts it adds: the input layer stays on the CPU by #170's policy (not by this family's doing), the opt-in works for a tied embedding since #233, and leaving it off by default because device input measured ~6 % slower on a 0.6B is a reason a reader can act on. Good correction, and better than the version I approved.

The one thing left over is the parenthetical at the end:

(Engine #1316 is where both halves were measured — it is not the #856 host-pointer import, which GGML_HRX_WEIGHT_HOST_PLACEMENT=0 shows changes nothing here.)

GGML_HRX_WEIGHT_HOST_PLACEMENT no longer exists — fork #234 (engine #1351, merged 22:33Z) deleted the import switch outright, and grep -rn over the pinned submodule returns 0 hits. A reader who follows that clause today cannot run it, and the page would be telling them a cause was excluded by an experiment they cannot repeat — which is exactly the wrong-cause class this PR exists to remove. It was true when the paragraph was first written (the base still had #231's switch); it is not true of the pin the page ships against.

Two ways, both one line: drop the clause, or date it — "GGML_HRX_WEIGHT_HOST_PLACEMENT=0, while that switch existed (#231; removed by #234/#1351), showed this changes nothing". I would take the second: the exclusion still matters (it is how the attribution was settled), and the parenthesis is the only place that records it.

Everything else stands as approved: the cause is named as #170's default rather than a base-level regression, the 1 of 3024 / GET_ROWS x 1 / 2 splits census is called the family's floor rather than residue, and LLAMA_INPUT_LAYER_ON_DEV is named as the way out.

@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 ec9202a9 — mine at 7adc17de carries; the head moved by main only.

docs/zyphra-models.md   blob a5a912d5e6 in both heads   -> byte-identical, this PR's whole diff
merge-base --is-ancestor origin/main <head>   YES
diff vs main            1 file, +13/-3

The 37-file delta between the head I verdicted and this one is main arriving (tools/repro-hostgemm-lock.sh, tools/weekly_changes.py,
app/*.cpp and friends), not this PR changing. Its block — the Zyphra page naming the wrong cause for the batched-decode failure — is
untouched, so the review I wrote still describes these bytes.

@bong-water-water-bong
bong-water-water-bong merged commit da66194 into main Nov 8, 2026
8 of 9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the docs/zyphra-getrows-caveat branch November 8, 2026 00:45
bong-water-water-bong pushed a commit that referenced this pull request Nov 8, 2026
…ce (Refs #1339)

The paragraph in docs/zyphra-models.md that cleared up the CPU-side input GET_ROWS ended with "it is not the #856
host-pointer import, which GGML_HRX_WEIGHT_HOST_PLACEMENT=0 shows changes nothing here". That was true when
written and is stale at this pin: the import was replaced (#234, pin ed43da0b4) and the variable no longer
exists - docs/hrx.md already records "GGML_HRX_WEIGHT_HOST_PLACEMENT does nothing". A reader following the
citation would look for a knob that is not in the tree.

The claim itself still holds and is now stated so it survives the removal: while the import existed, the
variable reproduced the placement identically, and the import is no longer in the tree at all, with hrx.md as
the record. This is the qualifier the #1356 review asked for, taken as a rewrite rather than a "(at pin X)"
footnote because the honest reading at this pin is that there is nothing left to toggle.

Refs #1339
bong-water-water-bong added a commit that referenced this pull request Nov 8, 2026
…ce (Refs #1339) (#1390)

The paragraph in docs/zyphra-models.md that cleared up the CPU-side input GET_ROWS ended with "it is not the #856
host-pointer import, which GGML_HRX_WEIGHT_HOST_PLACEMENT=0 shows changes nothing here". That was true when
written and is stale at this pin: the import was replaced (#234, pin ed43da0b4) and the variable no longer
exists - docs/hrx.md already records "GGML_HRX_WEIGHT_HOST_PLACEMENT does nothing". A reader following the
citation would look for a knob that is not in the tree.

The claim itself still holds and is now stated so it survives the removal: while the import existed, the
variable reproduced the placement identically, and the import is no longer in the tree at all, with hrx.md as
the record. This is the qualifier the #1356 review asked for, taken as a rewrite rather than a "(at pin X)"
footnote because the honest reading at this pin is that there is nothing left to toggle.

Refs #1339

Co-authored-by: agent <agent@local>
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.

1 participant