Repository navigation
third_party: bump llama.cpp to 67d8c42ec — a tied embedding no longer forces one node onto the CPU per graph (Fixes #1316) - #1344
Conversation
|
#1344 ready for review at 36fc187 Pairs with fork #233. The numbers, and the one thing worth checking closely: the gate is scoped to device-resident weights ( |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 36fc187f794d4f27dbf6d8083dfafaa8626bc53b. This bump would silently revert #1339's mitigation: the new pin is built on the fork commit before #231, so the hanging weight-mapping import would come back on by default. The #1316 change itself reads fine; this is a base problem, and the fix is a rebase plus a re-pin.
Measured from the fork:
new pin 37f89ee41559 parent a3e8d42c9984 (the pre-#231 branch tip; the body also says "a3e8d42c9 -> 37f89ee4")
main pin 092eb9dbcffc = fork #231's merge = current branch tip (from #1343)
compare 37f89ee4...092eb9db: commits in main's pin but NOT in the new pin:
092eb9dbcf hrx: make the weight-mapping import opt-in until it stops hanging large loads (engine #1339)
compare 37f89ee4...1bit/hrx-vulkan-patched: diverged, branch 1 ahead / pin 1 ahead
fork #233: OPEN, base 1bit/hrx-vulkan-patched, head 37f89ee4
And at the new pin, ggml/src/ggml-hrx/ggml-hrx.cpp:161-164 is the pre-#231 switch:
static bool import_host_pointer_enabled() {
static const bool disabled = [] { … return env != nullptr && env[0] == '0'; }(); // on unless =0So after this merge, every -ngl 99 load would import the mapping again, and a dense 27B or Flash-Next would stall at load (11 min and 72 min in #1339's rows), which #1343 pinned out an hour ago.
Fix:
- rebase fork #233 onto
1bit/hrx-vulkan-patched(092eb9db), so it carries #231; - merge #233, and pin the commit it produces on the branch (re-pin if the squash rewrites the sha);
- regenerate
registry/architectures.json, and fix the body's "from" sha to092eb9db; - re-run at least the default-path row on the rebased pin. With #231 in, the import is off by default, so "default, gate on / off" now measures the device-copy path, not the import path. The
GGML_HRX_LAYOUT_CONFLICT_NO_PACKgate is scoped to device-resident weights, which under the new default is more of them, so the default row is worth re-taking rather than assuming.
The #1316 analysis reads right: the embd GET_ROWS is on the CPU by design (llama-model.cpp:1372, LLAMA_INPUT_LAYER_ON_DEV=1 to override), the import is not the cause, and the override's tied-embedding "conflicting resident layout requests" is a real pre-existing refusal that the gate removes. I haven't reviewed #233's code in depth, because the base has to change first.
Known-answer coverage for the routing change (
|
36fc187 to
27ecb2c
Compare
PR Reviewer Guide 🔍(Review updated until commit dbb8454)Here are some key observations to aid the review process:
✅ Resolved findingsthird_party/llama.cpp:1Possible Regression in Host Placement Logic The change in |
|
#1344 re-pointed at a85a6f342 (re-verdict requested) The fork branch advanced while this was open: Re-verified after the rebase (2 runs per arm, one ticket): strict census with |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 27ecb2c3063693de74af0e2e9f3da2c2129b324a. One item left, and it's sequencing. The blocker from my CHANGES at 36fc187f is fixed, and the re-take checks out, but the pin is still the head of the OPEN fork PR #233.
Fixed and verified
- #231 is in the pin now:
compare a85a6f34...092eb9dbshows 0 commits missing, and ata85a6f34the switch is #231's (ggml-hrx.cpp:163-166,enabledonly for=1). So this bump no longer reverts #1339's mitigation. The body's "from" is now092eb9dbc, andregistry/architectures.jsonnamesa85a6f34. - The re-take on the rebased base is real (
~/wt/i1316-rv-1340, every run reportsbuild b71-a85a6f342):defaulthas 0 decline lines and runs at 175.1 / 307.3 t/s;dev-stricthas 2 declines each and runs at 182.5 / 307.9;dev-nogatehas 3conflicting resident layoutlines and 0.0 t/s. That matches the body's table, including "the gate does not fire" on the default path, which under #231's default is the device-copy path I asked to be re-measured. The decline lines name the registration and the weight, which is a good log line.
Remaining: compare a85a6f34...1bit/hrx-vulkan-patched = behind (pin 1 ahead, branch 0), and fork #233 is OPEN. Merge #233 onto the branch, then pin the commit it produces (re-pin if the squash rewrites the sha), as #1343 did with #231. Then this is READY with no further review.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 27ecb2c30636 — with one title fix and one sequencing note, both of which the release lane will care about.
The #1343 trap does not bite here: the pin resolves. Same command, this head, rc 0:
$ git submodule update --init --depth 1 third_party/llama.cpp
* branch a85a6f34258e948fe1e1b9aa4cfa6fd19061ef86 -> FETCH_HEAD
Submodule path 'third_party/llama.cpp': checked out 'a85a6f34258e948fe1e1b9aa4cfa6fd19061ef86' rc 0
$ python3 tools/registry_build.py --check-pins
registry/architectures.json records the committed pins rc 0
registry: a85a6f34258e… gitlink: a85a6f34258e (agree — the #1267 trap satisfied)
1. The title names a sha that is not what is pinned, and it is not fetchable. The title says "bump llama.cpp to 37f89ee4";
the gitlink, the registry and the body all say a85a6f342 (the body even explains why: "the fork branch advanced while this was
in review … my commit is rebased onto that tip, so this bump goes 092eb9dbc → a85a6f342"). And 37f89ee4 is a divergent
pre-rebase commit: compare gives 1 ahead / 1 behind against the pin branch and 2 ahead / 1 behind against a85a6f34, a bare-sha
fetch refuses it, and its own registry patch is +49/−1 where the pinned commit's is +65/−1. So the title names a commit that
is in neither the branch nor this pin. Edit the title to a85a6f342 — this cycle has now produced three of these (a hash, an
address, and a title) and each one was quoted somewhere before it was caught.
2. The pin is fork PR #233's head, which is still OPEN (base 1bit/hrx-vulkan-patched, head a85a6f342), one commit on top of the
branch tip 092eb9dbc — so it is reachable today only because that PR ref is alive. That is the same shape as #1343, and it differs
only in that #1343's commit could not be fetched at all while this one can. If you would rather main not depend on an open PR's ref,
merge #233 first and the pin becomes the branch tip; I will re-verdict that head (it is a re-point, so the delta check is small).
Otherwise this is mergeable now and the re-point-after-#233 churn is a known cost.
3. "Was the proof run at that pin?" The body says "measured on the pin", and its pin is a85a6f342 — but because the title
names a different, divergent commit, that sentence is one ambiguity away from being unverifiable. One clause naming the sha the
measurement was taken at would close it, and it matters here more than usual: the #1316 fix is a dispatch/registry change
(dispatch-mul-mat.cpp +19/−1, dispatch-registry.cpp +65/−1), so a measurement at the pre-rebase commit would be a measurement of a
different patch.
Everything else is right: Fixes #1316 is the correct reference, only the pin line moves in the registry (counts stay 298/298), and
the fix is scoped to the case the issue names — a tied embedding no longer forcing one node onto the CPU per graph.
…one node onto the CPU per graph (engine #1316)
27ecb2c to
a0c6d5f
Compare
|
Persistent review updated to latest commit a0c6d5f |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at a0c6d5f6bfda5aa91aceebae750f0c6757095a1f. This clears the last item from my CHANGES at 27ecb2c3.
- The pin is the branch commit:
third_party/llama.cpp=67d8c42ec65ff6ec963b0d55d166ac888da2b438= fork #233's merge commit, andcompare 67d8c42e...1bit/hrx-vulkan-patched= identical (0/0).registry/architectures.jsonnames the same sha. The engine diff is 2 files, +2/−2. - The squash is what I verified: a direct tree diff of
a85a6f34(the head I checked at27ecb2c3) against67d8c42eis empty. So everything verified there carries over unchanged: #231's opt-in import is present (enabledonly for=1), and the re-take on the rebased base holds (builda85a6f342; default path 0 declines;LLAMA_INPUT_LAYER_ON_DEV=10 nodes off HRX with 2 logged declines; gate off fails with the layout conflict).
Fixes #1316 is right: the report's symptom is explained as by-design CPU placement, and the opt-in override that was broken for tied embeddings now works.
|
#1344 re-pointed at the merged sha — re-verdict requested at Fork #233 is merged as
Proof re-run at
So the squash changed nothing observable: acceptance case green, the gate provably the fix, and the default path never reaching it (zero declines logged, which is mechanism-level rather than a timing comparison). Known-answer |
|
Persistent review updated to latest commit dbb8454 |
third_party/llama.cpp:092eb9dbc→67d8c42ec65ff6ec963b0d55d166ac888da2b438— the merge of fork hrx: do not pack a device-resident weight that another node gathers natively (engine #1316) llama.cpp#233 on1bit/hrx-vulkan-patchedWhere the measurement ran: the proof below was measured at
a85a6f342(the rebase head of #233) and re-run at themerged sha
67d8c42ecafter the squash — a squash rewrites the commit, so the numbers are attached to the sha this PRpins, not to the one it was authored at. The merged diff is my patch byte-for-byte (
+/-line hashca51bc22a8f40054forboth
67d8c42ec^..67d8c42ecanda85a6f342^..a85a6f342).What #1316 actually was
The report said the token-embedding
GET_ROWSleaves the device after the #856 import. It does not, and the two suspects inthe report are both cleared by measurement:
GGML_HRX_WEIGHT_HOST_PLACEMENT=0reproduces the pre-import placement in the same binary and thestrict census is identical (
1 of 986 nodes off HRX (GET_ROWS x 1), 2 splits).src/llama-model.cpp:1372: "there is very little benefit to offloading the inputlayer, so keep it on the CPU by default", with
LLAMA_INPUT_LAYER_ON_DEV=1as the override. The reporter's "before" basepredates HRX: fix the MoE router expert-id fault (engine#123) #170.
And the override was broken for a tied embedding, for reasons older than either commit:
token_embdis gathered natively byGET_ROWSand contracted in a packed row layout by the outputMUL_MAT, and a command program holds one binding pervalue, so the executor refused the program —
weight value 0 has conflicting resident layout requests,llama_decode ret = -3, measured with the import on and off. The kernel is not at fault:test-backend-ops -b HRX0 -o GET_ROWS→ 64 cases OK, 0 FAIL against the CPU reference.The fix (in the pinned commit)
The dispatch registry declines a match that requests a packed layout of a weight another node gathers natively, so the next
registration for that root op (the native-format one) takes the node and both consumers share one resident copy. It is scoped
to device-resident weights (
!ggml_backend_buffer_is_host), because the conflict needs both readers to want a residentbinding — so the shipped path cannot reach the gate.
GGML_HRX_LAYOUT_CONFLICT_NO_PACK=0is the A/B control, and the declineis logged once per (registration, weight) so it is greppable:
Measured at
67d8c42ec(2 runs per arm, onegpu-holdticket; 0.6B,-dev HRX0 -ngl 99 -c 512)LLAMA_INPUT_LAYER_ON_DEV=1+GGML_HRX_STRICT=1GGML_HRX_LAYOUT_CONFLICT_NO_PACK=0)conflicting resident layout requests…ON_DEV=0)gate cannot fire. That is stronger than "the timing was the same" — it is the mechanism not being reached.
Known-answer coverage after the routing change:
test-backend-ops -b HRX0 -o MUL_MAT→ 564 cases OK, 0 FAIL(
2/2 backends passed), so a placement win is not being bought with an untested kernel.Scope
tools/registry_build.pyregenerated from the merged tree — only the pin line moves; counts stay 298/298 (npu 9 followsmain). The change touches neither the host-pointer import nor its opt-in (#1339): it changes only which layout a
MUL_MATasks a device-resident weight for.
Fixes #1316