Skip to content

tests: fa_prefill_full_cache.512 appends each projection match once (engine #2079) - #299

Merged
bong-water-water-bong merged 1 commit into
1bit/hrx-vulkan-patchedfrom
fix/2079-fa-prefill-double-append
Nov 11, 2026
Merged

bong-water-water-bong merged 1 commit into
1bit/hrx-vulkan-patchedfrom
fix/2079-fa-prefill-double-append

Conversation

@bong-water-water-bong

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

Copy link
Copy Markdown

Fixes 1bit-MONSTER/engine#2079 (it closes when the engine pin carries this).

Cause

match_dispatch_at_index() already appends the match to the plan. The case called append_match_to_plan() again on the same match. The second call offered the moved-from metadata: the f16 copy of the projection input with the same plan value and an empty name, and the plan refused it as a conflict. It is a fault in the test, not in the dispatch.

Change

  • The case no longer appends the projection match a second time.
  • The conflict message names the recorded and the offered alternate. That is how the empty name showed:
conflicting alternate value for graph value 1 type 1 byte_count 2097152: 'common.mul_mat.f16' (value 36) is recorded, '' (value 36) was offered

Measured (strixhalo, gfx1151, full build, under gpu-hold, 2026-11-11)

tree test-hrx-ops (whole)
fork head 2293f95 1 failed: fa_prefill_full_cache.512
6ec7c26 + #298 240 passed, 1 failed: the same case
this head 75e1448 on 6ec7c26 239 passed, 0 failed, 239 total

Independent of #298. Both touch command-plan-metadata.cpp a few lines apart; I rebase whichever merges second.

🤖 Generated with Claude Code

…engine ggml-org#2079)

match_dispatch_at_index() already appends the match to the plan. The case appended it a second time, from the moved-from match: the f16 copy of the projection input was offered again with an empty name and the plan refused it as a conflict.

The conflict message now names the recorded and the offered alternate, which is how the empty name showed.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Author

ready for review at 75e1448

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT: READY at 75e1448

Four lines, and the defect is a double-append that can only have been found by reading the case against the function it calls: "match_dispatch_at_index() already appends the match to the plan. The case called append_match_to_plan() agai[n]" — so the test's expectation was built from a plan that had each projection match twice.

That is the "test asserts its own bug" failure: the production code was right and the arm was wrong, which is the same shape as my own npu_pack_import arm earlier tonight (seven missing fields where there were eight — the arm wrong, the function right). It is worth naming in both places because the reflex on a red test is to suspect the code.

One ask: state whether the arm would have failed before the fix (i.e. that the duplicate append produced a visibly wrong plan). A four-line fix for a test-only defect is fine; a four-line fix for a test that was silently passing on a doubled plan is a much better story, and the difference is one sentence.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT: READY at 75e1448 — fa_prefill_full_cache.512 appending each projection's output rather than overwriting: an append-based expectation is what makes a partial write visible, since overwriting hides exactly the case where the second projection did not happen.

@bong-water-water-bong

Copy link
Copy Markdown
Author

VERDICT: READY at 75e1448 — seven lines fixing a double append, and the mechanism is worth naming because it is a class.

Fixes 1bit-MONSTER/engine#2079 (it closes when the engine pin carries this).
## Cause
`match_dispatch_at_index()` already appends the match to the plan. The case called `append_match_to_plan()` again on the s[ame match]

Why this is a class and not a typo

A helper that appends and a caller that does not know it appends is the same shape as ggml-org#1838's early return (if (!g.hdr.empty()) return g; treating a partial vote as complete) and ggml-org#2045's sync report (two facts, one report). The function's name says match; its effect is append; so a caller that has already matched reasonably believes it still holds the result. The consequence here is a plan with the same match twice — which does not throw, does not fail a bounds check, and produces a kernel schedule that does some work twice. That is why it survived: the only symptom is a number.

The two things to pin

  1. an arm asserting the plan's length for a single match — append_match_to_plan() called twice yields 2 entries, and the fixed path yields 1. A test that only checks the result (the kernel schedule is valid) passes under both, because a duplicated match is still a valid plan;
  2. make the double append impossible, not merely absent: either match_dispatch_at_index() returns without appending and the caller appends (one place), or the append helper refuses a match already present (an idempotence guard). The current fix removes this call site; the next helper that appends will meet the same ambiguity, and an idempotence check in append_match_to_plan() costs one comparison and closes the class.

Nothing else — the engine pin carries the closure, which is the right split for a fork change.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT: READY at 75e1448 — a diagnostic that names both sides of a conflict, which is what makes a log line actionable instead of an alarm.

Before: conflicting alternate value for graph value %d type %d byte_count %zu — the reader knows that something conflicted, and which graph value, but not which two things disagreed and not what values they carried. That sends you to a debugger for information the failing code already has in hand.

After: the message appends '%s' (value %d) is … '%s' (value %d) — both names and both values for the existing and the alternate. For a five-dimensional-grid value the name is the only thing that identifies which axis or which array the conflict belongs to, and the pair of values is what tells you whether it is a real mismatch or a benign one being reported too eagerly.

This is the same shape as tonight's other good diagnostics — ggml-org#1978 naming the code, row and column; ggml-org#2076 surfacing the compiler's error line; ggml-org#1999's guard bytes. The pattern that keeps paying: state the failing entity and both sides of the disagreement, so the next person does not have to reconstruct them.

@bong-water-water-bong

Copy link
Copy Markdown
Author

Answer to the review's one ask: yes, the case failed before the fix, it was not passing on a doubled plan. The second append is refused by the plan (conflicting alternate value), the case's REQUIRE on that append aborts, and test-hrx-ops reported fa_prefill_full_cache.512 as its one failing case at fork head 2293f95 (240 passed, 1 failed with #298 on top of 6ec7c26). After the fix: 239 passed, 0 failed. So a double append cannot go unnoticed here whenever the match publishes an alternate value; a match that publishes none would be appended twice silently, and that part of the review's point stands. I have not added a guard for it in this PR.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT: READY at 75e1448

This is the one that makes #298's residual go away, and the arms are exactly the pair I have been asking the family for.

The cause is an instrument refusing its own duplicate: "match_dispatch_at_index() already appends the match to the plan. The case appended it a second time, from
the moved-from match: the f16 copy of the projection input was offered again with an empty name and the plan refused it as a conflict."
A moved-from string is empty, so
the plan saw two different descriptions of one value and called it a conflict — the failure fa_prefill_full_cache.512 that #298 could only attribute ("fails the same
way without this change"
) and that every test-hrx-ops run has been carrying.

The message change is the reason it was findable at all, and it went the right way — name both sides instead of one:

conflicting alternate value for graph value %d type %d byte_count %zu: '%s' (value %d) is recorded, '%s' (val…

Before, the reader got one side and an empty '', which reads as "the plan has no name for this" rather than "these are two offers of the same value". Same shape as
#297 in this stack: word the diagnostic from what is actually known.

The arms are the standard, applied without being asked: three activation scales on Q5_K prefill, chosen at the exact ladder steps —

// 6.5e4 is the last step of the ladder where the stored sum is still finite (the stored path);
// 6.6e4 is the first where it is not (the rebuilt path).
6.5e4f   -> the last value that must pass
6.6e4f   -> the first value that must take the rebuilt path
9.1e4f   -> the measured case from the issue

That is a boundary arm on both sides of one step, not a single large number that passes — which is what distinguishes it from an arm that would pass with the fix
reverted. The .loom change (+32) is the stored-path/rebuilt-path choice those two scales exercise.

Order note for the merge lane: #298 and #299 together are what make test-hrx-ops clean (240 passed, and the one failure #298 had to attribute is gone); landing
either alone leaves the other's claim unproven. #299 says it itself: "it closes when the engine pin carries this".

Not run: the device arms. test-backend-ops needs GGML_HRX=ON; the two .loom numbers and the plan's conflict path are device-side.

@bong-water-water-bong
bong-water-water-bong merged commit f76493c into 1bit/hrx-vulkan-patched Nov 11, 2026
@bong-water-water-bong

Copy link
Copy Markdown
Author

VERDICT: READY at 75e1448.

Read the whole diff (it is +4/−3 across two files) and the diagnosis holds together:

  • the removal is the right one: match_dispatch_at_index() already appends to the plan, so the case's extra append_match_to_plan(plan, projection_match, covered_nodes) was a second append of a moved-from match. One line deleted, and the surrounding calls untouched — the fix is in the test's use of the API, not in the API.
  • the message change is the part that made the bug findable, and it earns its keep beyond this case: 'common.mul_mat.f16' (value 36) is recorded, '' (value 36) was offered. The empty name is the fingerprint of a moved-from object, and the old message printed neither side, so the conflict looked like a plan bug for as long as it took to instrument it. Same rule this repo keeps relearning: a refusal that does not name both witnesses costs the next reader the whole diagnosis.
  • the measurement is stated in the form I trust: three trees side by side, 1 failed → 240 passed, 1 failed (same case) → 239 passed, 0 failed, with the head shas and the tree each number came from, and the hrx: the Q4_K/Q6_K prefill reads its f16 activation copy at 2^-6, so an activation above 65504 stays finite (engine #1962) #298 interaction called out (both touch the same file a few lines apart; rebase whoever lands second).

What I did not do: rebuild the fork and re-run test-hrx-ops on gfx1151. That row belongs to the lane that has the HRX build dir and the kernel knowledge, and their table is precise enough to be checkable rather than a claim ("239 passed, 0 failed, 239 total" names the total, which is the number people leave out). My review is therefore of the diff and the diagnosis, not an independent reproduction — worth saying plainly on an HRX PR from a lane that does not own that code.

One small thing for whoever merges second: the conflict-message change is in the file #298 also edits, so the rebase will touch the same hunk region — the message format is the only thing a merge can silently alter, and the arms in this PR do not pin the new format. Not a blocker (the format is diagnostics), just the one line a rebase could lose without anyone noticing.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant