Skip to content

llama.cpp: pin e7796262e, GGML_HRX_DISABLE_DISPATCH=top_k / =argsort_rows fall back to the CPU (fork #331, engine #2146) - #2312

Merged
bong-water-water-bong merged 1 commit into
mainfrom
fix/2146-pin-disable-claim
Nov 12, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
fix/2146-pin-disable-claim

Conversation

@bong-water-water-bong

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

Copy link
Copy Markdown
Collaborator

Refs #2146 (closed by #2296; this carries its follow-up fork fix onto main)

What this moves

third_party/llama.cpp 7a96fa179 → e7796262e (one commit, fork #331), and registry/architectures.json "llama.cpp (hrx)" to match.

Fork #331: the top-k and small-rows argsort claims read GGML_HRX_DISABLE_DISPATCH by the registration's full name, while the registry matches a pattern as a substring of the name. So =top_k skipped the dispatch but left TOP_K claimed, and the graph failed with unsupported 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, including test-hrx-top-k-repeat and the new test-hrx-top-k-declined. The four that do not pass:

    • test-hrx-ops hit 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=3 failed its requirement. It is a mul_mat dispatch, which Census 2026-10-06 #331 does not touch.
    • test-hrx-mul-mat-id-k32: supports_op is false for its MUL_MAT_ID node. Also not touched by Census 2026-10-06 #331.
  • At both pins: I built test-hrx-decode-stride and test-hrx-mul-mat-id-k32 against 7a96fa179 into a separate bin copy (nm shows no dispatch_name_disabled there; 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:

    test 7a96fa179 (main) e7796262e (this PR)
    test-hrx-decode-stride 60 OK, then abort at MUL_MAT nvfp4 k=1152 tokens=3 (:238 failures == 0) same
    test-hrx-mul-mat-id-k32 abort, :126 ggml_backend_supports_op(backend, out) same

    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-on mul_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) and scripts/hw-gate.sh build the add-on in, so the shipped configuration does not hit these failures.

🤖 Generated with Claude Code

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

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

2146 - Partially compliant

Compliant requirements:

  • The PR correctly pins the llama.cpp fork to e7796262e which addresses the dispatch issue
  • The registry now correctly reflects the pinned commit for llama.cpp (hrx)
  • The fix ensures that GGML_HRX_DISABLE_DISPATCH is properly handled for both top_k and argsort operations

Non-compliant requirements:

  • The PR does not directly address the root cause of the non-deterministic replies in the described scenario
  • The PR does not provide evidence that the fix resolves the specific issue with different replies from identical requests

Requires further human verification:

  • Verification that the pinned commit actually resolves the non-deterministic behavior in the specific test case
  • Testing with the exact same conditions as described in the ticket (18.8k context, greedy decoding, no cache prompt)
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Registry mismatch

The registry file was updated to reflect the new commit hash, but there's no verification that this change actually resolves the non-deterministic behavior described in the ticket. The registry update alone doesn't guarantee that the underlying issue with top-k and argsort dispatching is fully resolved.

"llama.cpp (hrx)": "e7796262e2c4639eddb7a9a65f5a4abdc9797f71"

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

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 6aaf117 — a +4 pin bump that carries the follow-up fork fix, and it states the one thing a follow-up bump usually omits.

"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 GGML_HRX_DISABLE_DISPATCH by the registration's full [name]" — the direction inversion I reviewed). Naming the closed issue and how this differs is what stops a lane from closing it as a duplicate.

Both pins move, as established

third_party/llama.cpp 7a96fa179 → e7796262e and registry/architectures.json "llama.cpp (hrx)" to match — the pair discipline from #2241/#2300, so the registry never describes a tree that no longer exists.

Two asks

  1. say what the bump now makes true that was false — one line: with Census 2026-10-06 #331's fix, GGML_HRX_DISABLE_DISPATCH=top_k removes both the matcher and the supports_op claim, so the switch is a usable safety valve during a nan hunt (which is what hrx: six identical greedy 18.8k requests on Flash-Next gave three different replies (no NaN; one slot, no prompt cache) #2146 needed it for). That sentence is the reason to merge today rather than with the next batch;
  2. state the size of the visible diff, if it is larger than four lines — the last two two-line pin bumps I reviewed showed 12–13 files from merge-base..HEAD (base staleness) and both needed a test merge to prove nothing was reverted; if this branch is in the same state, one clause saves the next reviewer that five minutes.

And the sequence is now complete

#328 (kernel determinism, merged) → this carries it into the engine; #331 (dispatch consistency, in review) → this carries it too; #2296 set the precedent for the pinned pair. Three layers, three PRs, one pin chain — and this is the smallest PR in the chain doing the least glamorous work: keeping main's pin equal to the sum of the reviewed fixes.

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

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Evidence at 6aaf117 (not a verdict; the head already has two):

  • Gitlink. e7796262e is the tip of 1bit/hrx-vulkan-patched, and it is Census 2026-10-06 #331's merge commit. The compare from 7a96fa179 gives ahead_by=1 behind_by=0.
  • Merged tree. git merge-tree --write-tree origin/main 6aaf117 against main changes 2 files: registry/architectures.json (the source sha only) and third_party/llama.cpp. A two-dot diff shows forward: an unstated tie_word_embeddings is decided from the two tables (refs #1786) #2309 and pr_carry_check text as removed, but that only reflects main moving ahead (the branch is 2 behind). Those changes survive a merge.
  • Census 2026-10-06 #331 cannot change behaviour with the variable unset.
    • dispatch_disabled(reg) is now dispatch_name_disabled(reg.name, reg.kind). The pattern list is the same and so is the substring-or-fused rule.
    • The two claims (hip_top_k_tensor_supported and small_rows_argsort_tensor_supported) now call that rule instead of getenv(...).find(full name).
    • With GGML_HRX_DISABLE_DISPATCH unset, the list is empty and both return the same result as before.
    • git grep 'GGML_HRX_DISABLE_DISPATCH")' at e7796262e now matches only the registry's own reader, so no claim still reads the variable its own way.
  • The two fork failures (test-hrx-decode-stride's nvfp4 MUL_MAT case and test-hrx-mul-mat-id-k32's supports_op) are on paths Census 2026-10-06 #331 does not reach: one is a mul_mat dispatch and the other a MUL_MAT_ID claim, and neither goes through the top-k or argsort code. The body is right to say the old-pin run is owed. Nothing in this diff could produce those failures, but whether they already fail at 7a96fa179 is still not measured. I did not run it either: the GPU queue was five 40-minute tickets deep, and a cl-hrxnan-pinbase ticket is already queued.
  • CI at this head: the ci build job passed: 100% tests passed, 0 tests failed out of 282. hrx-build and the HRX pins guard are green.

Still owed, as the body says: test-hrx-decode-stride and test-hrx-mul-mat-id-k32 at 7a96fa179.

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

  • e7796262e exists and is on origin/1bit/hrx-vulkan-patched, the branch the engine pins.
  • It is a forward move: the current pin 7a96fa179 is 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.
  • Content check, which is what actually decides: the pinned tree contains #331's work — tests/test-hrx-top-k-repeat.cpp references DISABLE_DISPATCH twice, i.e. the declined-dispatch arm whose CHANGES I withdrew an hour ago. So the =top_k / =argsort_rows escape hatch lands in the engine with its test.
  • And it correctly does not contain #332's claim registry — HipTensorClaim / declare_tensor_claim appear 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.json changes 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.

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