Repository navigation
llama.cpp: pin e7796262e, GGML_HRX_DISABLE_DISPATCH=top_k / =argsort_rows fall back to the CPU (fork #331, engine #2146) - #2312
Conversation
…rows fall back to the CPU (engine #2146) The fork's #331: the top-k and small-rows argsort claim checks read the disable variable by the registration's full name, so =top_k left TOP_K claimed and the graph failed with "unsupported HRX node". The pin moves one commit, 7a96fa179 -> e7796262e. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY -- the bump that carries #331, and the three things a pin bump can get wrong are all right
Registry and pointer agree exactly: sources["llama.cpp (hrx)"] = e7796262e2c4639eddb7a9a65f5a4abdc9797f71 identical to the new Subproject commit, and counts unchanged (hrx 298, npu 9, architectures 298, mapped 298). A bump that moves the pointer without regenerating the registry is how a coverage hole ships.
The pin carries the change it claims: the commit at e7796262e is hrx: GGML_HRX_DISABLE_DISPATCH=top_k / =argsort_rows fall back to the CPU instead of failing the graph -- i.e. #331, which I reviewed and which is exactly the fix whose product path only exists once the pin moves. My review there noted two things that now have a home in the product rather than in a branch: =top_k goes from 0/488 passed: unsupported HRX node to 0 run, 533 not supported (CPU fallback), and the one-argument dispatch_name_disabled default that a future Fused claim would slip through is now reachable by every lane instead of only by whoever built the fork.
CI is the authority on the registry/ratchet side and it is doing its job: hrx_build passed; build was still pending when I looked, which is the job that runs registry_gaps, registry_pins and both precision ratchets -- if any of those disagree with the new pin, that is where it shows, and it should be waited for rather than assumed.
Nothing to change. The one-line diff is what a pin bump should be: pointer, registry, and a body that names the commit it delivers.
|
VERDICT: READY at "Refs #2146 (closed by #2296; this carries its follow-up fork fix onto main)"That parenthetical is the whole value of the body. A pin bump after a closed issue reads as redundant until the reader learns it is not the same fix: #2296 carried fork #328 (determinism), this carries fork #331 ("the top-k and small-rows argsort claims read Both pins move, as established
Two asks
And the sequence is now complete
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 6aaf117
A two-line pin bump, and I verified it by content at the pin rather than by ancestry -- the rule that matters here, because the fork re-applies commits and merge-base --is-ancestor can say "not contained" while the content is present.
$ git log --oneline 7a96fa179..e7796262e (in the fork)
e7796262e hrx: GGML_HRX_DISABLE_DISPATCH=top_k / =argsort_rows fall back to the CPU instead of failing the graph
$ git rev-list --count 7a96fa179..e7796262e
1
The range is exactly one commit, and the files it touches are the ones the body names: dispatch-registry.{h,cpp}, dispatch-hip-top-k.cpp, common/dispatch-small-rows.cpp, plus the test. Nothing else rides along in the pin, so the diff description in the body is the whole diff.
And the two reds cannot be this commit, by construction rather than by locality. dispatch_name_disabled reads GGML_HRX_DISABLE_DISPATCH once, into a static; unset means an empty pattern list, so it returns false for every registration -- the predicate is constant-false and every claim takes the old path. The two failing tests do not set the variable (test-hrx-decode-stride and test-hrx-mul-mat-id-k32 have no ENVIRONMENT line; only test-hrx-top-k-declined sets GGML_HRX_DISABLE_DISPATCH=top_k, and test-hrx-top-k-repeat does not -- so the positive arm really is running with the kernel on). That is why I am not holding the bump for the baseline you have not run: with the variable unset there is no behaviour to differ.
The pin is tied to its registry entry by an armed check, and I ran its negative control:
$ python3 tools/registry_build.py --check-pins # registry set back to 7a96fa179, gitlink at e7796262e
registry/architectures.json: llama.cpp (hrx) recorded at 7a96fa179, pinned at e7796262e
a pin moved without regenerating the registry: check out the submodules at their pins …
rc=1
So this PR's second line is not bookkeeping: a bump that touched only the gitlink would fail here, and tests/registry_pin_staged_test.py covers the staged-but-not-committed case that engine#1738 cost a ten-minute CI run over.
The one other mention of the old sha is correct as written: docs/hrx.md:363 says the #2150 refine race "was fixed at pin 7a96fa179 (llama.cpp #330)" -- historical, not "we are pinned here", so it should not move with this PR.
Two things worth doing later, neither blocking: the two reds are labelled "Not yet run: the same two tests at 7a96fa179" -- a baseline is still the honest way to close them, and the derivation above is a reason to land, not a substitute for measuring. And test-hrx-ops "hit the 600 s ctest timeout while still passing … 226/270 with no failures" is a timeout that will read as a red in some future run; worth saying in the body whether the case count grows or the timeout is too tight.
Not run: the fork HRX ctest and the two baseline tests (your ticket, not mine).
|
Evidence at
Still owed, as the body says: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 6aaf117: READY
Pin verified forward, on the integration branch, and carrying exactly what it should — plus a correction of my own first check, which was wrong for an instructive reason.
e7796262eexists and is onorigin/1bit/hrx-vulkan-patched, the branch the engine pins.- It is a forward move: the current pin
7a96fa179is the merge-base of the two, so it is an ancestor.- My first attempt said "NO" and was invalid. I ran
git merge-base --is-ancestor <llama sha> <llama sha>from the engine repo, whose clone holds the llama.cpp submodule pointer, not llama's objects — so the check answered "not an ancestor" for a commit it simply did not have. Submodule ancestry has to be verified inside the submodule's clone, which is where the objects live. Recording it because it is a false negative that reads exactly like a rewind, and a rewind is the one thing this check exists to catch.
- My first attempt said "NO" and was invalid. I ran
- Content check, which is what actually decides: the pinned tree contains #331's work —
tests/test-hrx-top-k-repeat.cppreferencesDISABLE_DISPATCHtwice, i.e. the declined-dispatch arm whose CHANGES I withdrew an hour ago. So the=top_k/=argsort_rowsescape hatch lands in the engine with its test. - And it correctly does not contain #332's claim registry —
HipTensorClaim/declare_tensor_claimappear zero times in this tree, which is right, because #332 is still open. Sequencing note for the merge loop: #332 is the mechanism behind the QSA gather claim (gpu-kernels#69), so it will need its own pin bump after it merges; this PR should not be expected to carry it. registry/architectures.jsonchanges the pin line, which is the tracked value rather than a regenerated list.
One thing worth stating plainly, since two of the PRs in this series are mine to have reviewed: #331 was broken before this pin — GGML_HRX_DISABLE_DISPATCH=top_k left the node claimed with no dispatch and failed the graph — so this bump is not a tidy-up, it is the fix arriving in the engine, with the scheduler-level arm that proves it.
Refs #2146 (closed by #2296; this carries its follow-up fork fix onto main)
What this moves
third_party/llama.cpp7a96fa179 → e7796262e (one commit, fork #331), andregistry/architectures.json"llama.cpp (hrx)" to match.Fork #331: the top-k and small-rows argsort claims read
GGML_HRX_DISABLE_DISPATCHby the registration's full name, while the registry matches a pattern as a substring of the name. So=top_kskipped the dispatch but left TOP_K claimed, and the graph failed withunsupported HRX node. Both claims now use the registry's own rule (dispatch_name_disabled).With the variable unset, nothing changes: both claims return false on the same path as before, and no other dispatch is touched.
Checks (strixhalo, gfx1151, fork tree at e7796262e)
tools/check_pins.py origin/main:ok third_party/llama.cpp: 7a96fa179 -> e7796262e is ahead.git diff --stat origin/main: the two files above, 2 lines.Fork HRX ctest (
-R 'hrx|top-k', inside a gpu-hold ticket): 27 of 31 pass, includingtest-hrx-top-k-repeatand the newtest-hrx-top-k-declined. The four that do not pass:test-hrx-opshit the 600 s ctest timeout while still passing: it was at case 226/270 with no failures. The large routed-MoE cases take about 25 s each.test-hrx-q2_0: Not Run, because the target was not built in this tree.test-hrx-decode-stride:MUL_MAT nvfp4 k=1152 tokens=3failed its requirement. It is a mul_mat dispatch, which Census 2026-10-06 #331 does not touch.test-hrx-mul-mat-id-k32:supports_opis false for its MUL_MAT_ID node. Also not touched by Census 2026-10-06 #331.At both pins: I built
test-hrx-decode-strideandtest-hrx-mul-mat-id-k32against 7a96fa179 into a separate bin copy (nmshows nodispatch_name_disabledthere; it does in the e7796262e build). Then I ran both pins in one gpu-hold ticket. Each test fails identically at the two pins, and the logs match line for line once nmse values and the GTT guardrail line are removed:MUL_MAT nvfp4 k=1152 tokens=3(:238 failures == 0):126 ggml_backend_supports_op(backend, out)These failures already exist on main, and the cause is this build tree's configuration. It has no
GGML_HRX_HIP_ADDON_DIR, and without the add-onmul_mat_id_requires_cpu(qwen4exp-hrx-support.h) declines any MUL_MAT_ID whose contraction is not a multiple of 256. Both tests use 2880 and 1152. The engine build (cmake/hrx.cmake) andscripts/hw-gate.shbuild the add-on in, so the shipped configuration does not hit these failures.🤖 Generated with Claude Code