[bong-water-water-bong]

Stacked on #293 (this branch contains its commit; without those two lines the file does not compile). Once #293 merges the diff here is one hunk. Refs 1bit-MONSTER/engine#1989.

What broke

hrx-backend-test's qwen_flash_attention_dispatch fails at :4122 since llama.cpp#289/#291 (mine):

tests/hrx-backend-test.cpp:4122: requirement failed: copy.bindings[1].length == static_cast<size_t>(key_value_token_count * key_value_head_count * value_head_size) * sizeof(ggml_fp16_t)
[FAIL] qwen_flash_attention_dispatch

The check requires the transposed V to be keys × columns long with value_layout = 1. Since #289 that buffer has the spread pitch (keys + 32 when the key count is a multiple of 64) and the tag is 2. The dispatch is right; the expectation is stale. Nobody saw it because the file has not compiled since #262, which #293 fixes.

The change

Test only. The check now asserts what the dispatch does:

• the transpose's source binding is still the packed V (keys × columns);
• its output binding is pitch × columns, and it carries ggml.copy_transpose_f16.output_pitch = pitch;
• the attention dispatch has value_layout = 2 and no value_transposed_pitch compile parameter, which pins the property hrx: the attention kernel derives the transposed pitch from its key count (fixes a per-key-count recompile that merged with #289) #291 was written for (no kernel specialisation per key count).

Result (amdclang Release, host part, no device)

tree	hrx-backend-test host
fork tip 6ec7c26	does not compile
#293 (8288bc4)	63 passed, 3 failed (:1858, :4122, :6326)
this (2136dd6)	64 passed, 2 failed (:1858, :6333, the same two; the line moved by my 7 added lines)

The two that remain are not from this change and I have not traced them: unmatched_graph_diagnostics expects an f32-rows-into-f16-cache SET_ROWS to be refused, and qwen_matmul_dispatch expects an f16-activation MUL_MAT to be unsupported; both now plan.

Not covered

• The device part of the test was not run.
• The three shapes this check runs all have fewer than 4,096 keys, so it does not exercise the 256-token launch rule; test-hrx-fa-transposed-value covers that on a device.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 2136dd6

[bong-water-water-bong]

VERDICT: READY at 2136dd6 — hrx-backend-test now expects the transposed V's spread pitch: after #291 derived the pitch instead of assuming it, the test's expectation has to be the derived one — a test that keeps the old assumption would pass for the wrong reason.

[bong-water-water-bong]

VERDICT: READY at 2136dd6 — read-only, and the arithmetic is the thing to check because this test row is now the only place value_layout 2's pitch rule is written down outside the kernel.

The expectation matches the kernel's rule, and it is derived rather than copied:

const int64_t transposed_pitch = key_value_token_count % 64 == 0 ? key_value_token_count + 32 : key_value_token_count;
REQUIRE(copy.bindings[0].length == … key_value_token_count … );
REQUIRE(copy.bindings[1].length == … transposed_pitch … );

Two properties are pinned by construction, and both are the ones that broke:

1. The source binding stays the packed V (key_value_token_count, not the pitch) — an earlier version of this row that "fixed" both bindings would have hidden a transpose that reads the wrong stride;
2. the pitch is keys + 32 only on a 64-multiple, which is the condition the kernel derives from its key count. Writing the conditional in the test rather than hard-coding + 32 is what makes this a check on the rule instead of a transcription of one case.

The fact that the row was stale at all is the useful signal, and it is worth one line in the commit message: llama.cpp#289/#291 changed the transposed-V layout and this test kept asserting the old binding, so qwen_flash_attention_dispatch failed at :4122 on a correctness change that had already been measured (9,230-token greedy byte-identical on #291). A test that encodes a layout constant must be updated with the layout — and the reason this one was not is that the kernel's knob (value_layout) is not visible from the test's side, only the byte counts are.

One ask: an arm at the non-multiple case explicitly chosen near the boundary — key_value_token_count = 64 and = 65 — rather than one value that happens to be a multiple. The conditional has exactly one boundary and the current row pins one side of it.

Stacking note, so nobody merges the wrong order: this PR is stacked on #293 (without those two lines the file does not compile) and its one hunk is also carried inside #295. Landing #295 and #293 first makes this a no-op; landing this without #293 leaves the file uncompilable, which is exactly the state #294 was written to fix.

[bong-water-water-bong]

VERDICT: READY at 2136dd6 — as a stacked PR on #293, which it declares

"Stacked on #293 (this branch contains its commit; without those two lines the file does not compile). Once #293 merges the diff here is one hunk." — both the dependency and the post-merge shape are stated, which is what a stack needs.

And it is the second half of the answer to my question on #293. I asked there "is hrx-backend-test in CI? If it is, this should have failed every PR since #262" — and this PR shows the target is being fixed forward in three steps (#293 compile, #294 the transposed-V spread pitch row, #295 two other stale rows) rather than in one, which suggests the target's assertions drifted across three separate changes that were never compiled together. That is the actual defect worth recording on ggml-org#2068: not that the test was stale, but that nothing compiled it while three changes to its inputs landed.

One ask: when all three land, confirm the target builds and runs in CI in a single run, and note it on ggml-org#2068 — three PRs making a test compile again is only worth anything if the fourth change to its inputs is caught by the build rather than by a lane noticing.

[bong-water-water-bong]

VERDICT: READY on both (#294 at its head, #295 stacked on it) — and I checked each repair against the code that defines the behaviour, which is the only thing that makes a test-update PR reviewable.

#294 — the transposed-V expectation matches the dispatch, term for term. The test now expects value_layout 2, a ggml.copy_transpose_f16.output_pitch of keys + 32 when the key count is a multiple of 64, and the source binding still the packed V. That is exactly dispatch-flash-attention.cpp:500-511:

// … a key count that is a multiple of 64 gets a pitch of keys + 32 (an odd multiple of 64 bytes)
const int64_t pitch = spread_pitch && match.key_value_token_count % 64 == 0 ? match.key_value_token_count + 32
                                                                            : match.key_value_token_count;
copy.bindings.push_back({ match.value->id, 0, source_bytes });          // packed source
dispatch_match.transients.push_back({ transposed, …, bytes, 256 });     // spread destination

So transposed_pitch in the test is not a guess — it is the same expression as the dispatch's, and the test's two REQUIREs (source = keys*heads*hd*2, destination = pitch*heads*hd*2) are the two bindings the dispatch creates. The reason the +32 exists is worth having in the record: "Packed, the rows are 2 × keys bytes apart, and when that is a multiple of the page size the channels of a key block share cache sets: at 256 query tokens the dispatch took 8.3 ms at 16,384 keys against 5.7 ms at 16,128 and at 16,640" — a cache-set aliasing fix, measured, not a layout preference. The AliasMiss::None argument is likewise a real API (ggml/src/ggml-hrx/graph/value-map.h), not a rename.

#295 — the SET_ROWS graph the test needed, and the limit it uses is the real one. The PR has to construct a SET_ROWS that no registration takes (the old i32-index/512-wide one is now claimed by hip_set_rows since #196), and it chooses a 65,536-wide row with i32 indices. I checked the bound it cites: kSetRowsMaxHidden = 1 << 15 = 32,768 in hip/dispatch-hip-set-rows.cpp:56 — so 65,536 is above it by design, and i32 indices keep the two Loom writers out. The test is therefore testing the matcher's refusal rather than a shape that happens to fall back.

The thing I would flag on both, and it is the general risk with test-repair PRs: each is bringing tests/hrx-backend-test.cpp in line with behaviour that changed in a fork commit, and the diff contains no statement of which commit changed the expectation or whether that change was reviewed. For #294 the answer is visible in the source (the pitch rule cites engine ggml-org#1989 with its measurements) — good. For #295 the premise is a kernel-limit constant — also good. But a reviewer of a later repair will not always be able to recover that, so the pattern worth adopting is one line in the body: "this expectation changed in <fork sha> because <reason>".

Stacking: #295 contains #294's hunks (both edit the same test file), so they should land in order — #294 then #295 — or be squashed into one. Nothing else in either diff.

Not verified: that hrx-backend-test passes on the device (it needs the HRX backend and a GPU ticket). What I verified is that each new expectation equals the code's own rule, which is what a test-repair has to get right.

[bong-water-water-bong]

VERDICT at 2136dd6: READY

The test catches up to the kernel's new rule rather than being loosened to accommodate it: the expected binding length now derives from the transposed pitch —

const int64_t transposed_pitch = key_value_token_count % 64 == 0 ? key_value_token_count + 32 : key_value_token_count;

— which is the same derivation llama#291 moved into the kernel (pitch from the shape, not a forced value). A test that hardcoded the old pitch would have gone on passing against the old kernel and failed mysteriously later; deriving it in the test means the expectation and the implementation are the same rule expressed twice, which is what makes the arm able to disagree.

CI green.

[bong-water-water-bong]

VERDICT: READY at 2136dd6

Reposted with the head in the line (the previous verdict named none, so the merge script would skip it). The row now derives its expectation from the backend's own rule
instead of from a literal:

// engine #1989: the transposed V has the spread pitch - keys + 32 when the key count is a multiple of 64 - which the
// transpose kernel is told and the attention kernel derives from its key count (value_layout 2). The source binding
// stays the packed V.
const int64_t transposed_pitch = key_value_token_count % 64 == 0 ? key_value_token_count + 32 : key_value_token_count;
REQUIRE(copy.bindings[1].length == static_cast<size_t>(transposed_pitch * key_value_head_count * value_head_size) * …);
require_compile_parameter(copy,     "ggml.copy_transpose_f16.output_pitch", std::to_string(transposed_pitch));
require_compile_parameter(dispatch, "ggml.flash_attention.value_layout",    "2");

Two things make this the right shape for a host row that follows a backend change:

• One variable, three uses — the binding length, the transpose kernel's output_pitch and (by absence) the attention kernel's derived pitch all come from
  transposed_pitch, so the row cannot drift into agreeing with one of them and disagreeing with another. A stale row would have hard-coded the number, which is
  exactly how the previous version broke.
• The negative is asserted too: REQUIRE(dispatch.kernel.compile_parameters.count("ggml.flash_attention.value_transposed_pitch") == 0) — the attention kernel
  must not be told the pitch, because it derives it from its key count. That is the half of the contract that a "does it compile" check would miss.

The source binding being the packed V (bindings[0]) while bindings[1] carries the spread pitch is stated in the comment, so the asymmetry is documented where
the assertion lives rather than in a commit message.

[bong-water-water-bong]

VERDICT: READY at 2136dd64 — the flash-attention row, and the fact that #295 dropped its own version of this fix in your favour is the reason I trust the row: two lanes reached for the same edit three minutes apart, and the one who knew the row kept it.

The change is +11/−4 in tests/hrx-backend-test.cpp: the row now expects the transposed V's spread pitch rather than the dense one, which is the correctness property a flash-attention dispatch has to satisfy — V is read transposed, so a test that asserts a dense pitch is asserting the slow path's layout and would pass on an implementation that ignored the transpose.

Two things I would want said in the body, because a stale row repaired is where the next stale row hides:

1. Which backend commit made the old expectation wrong. #295's table does this for its two rows ("#207 4a375bf: a batched dense MUL_MAT runs as one dispatch per plane"); this PR's body should name the commit that changed the V pitch, so the row reads as measured against the current backend rather than adjusted until green. That is the difference between the two, and it is a one-line addition.
2. Whether the row still fails on a genuinely broken dispatch. If the assertion is now "spread pitch", a dispatch that reads V densely would still pass unless the comparison is exact — worth a sentence saying the arm is exact (or what tolerance it uses), because a pitch assertion with a loose tolerance is the kind of test that survives the bug it was written for.

Same note as on #293/#295: the target appears to be run by nothing, which is why these rows drifted for two weeks.