[bong-water-water-bong]

Fixes a defect that merged with #289 (139ccd741). Please do not move the engine pin to a fork commit that has #289 without this. Refs 1bit-MONSTER/engine#1989.

What is wrong on the branch now

#289 was merged at its first commit, 2cd6c63, at 21:16Z. I pushed a second commit to that branch a few minutes later, after finding this, and it is not in the merge.

2cd6c63 passes the transposed-V pitch to the attention kernel as a compile parameter. The pitch changes with the key count, so every distinct key count compiles a new copy of ggml_flash_attention_f32_f16_wmma. llama-bench absorbs compiles in its first repetition, which is why #289's tables looked fine. A plain run does not: llama-completion ingesting a 9,230-token prompt took 53 s with 2cd6c63 against 33 s with the old layout (same ticket, loaded box).

The fix

One commit, the one I pushed to #289's branch as 7af0108, cherry-picked onto the tip:

• value_layout = 2 means "transposed, spread pitch". The attention kernel computes keys + 32 for a key count that is a multiple of 64 from its launch argument, the same rule the dispatcher uses for the buffer it allocates. The pitch is no longer a compile parameter, so a new key count compiles nothing new.
• The transpose kernel already specialises on its row count, so its output_pitch config adds no specialisations and stays.
• GGML_HRX_FA_VALUE_PITCH becomes on/off (0 = packed layout).

Measured (strixhalo, gfx1151, amdclang build of 50d28600a + #289's commit + this commit, gpu-hold)

check	result
test-hrx-fa-transposed-value	8 cases, 0 failures
test-backend-ops -o FLASH_ATTN_EXT -b HRX0	388/388
greedy text, 9,230-token prompt, old layout vs this	byte-identical
cold prompt eval, same prompt, fresh process each	old layout 10.56 s, 10.49 s; this 10.44 s, 10.76 s
llama-bench -p 24576 -b 512 -ub 512 -r 3, tok/s per repetition, old layout	701.3 / 821.8 / 822.4 and 711.1 / 812.9 / 814.7
same, this	859.8 / 979.7 / 1,020.7, 865.9 / 1,067.1 / 1,073.7, 855.5 / 1,066.5 / 1,061.8

So the gain #289 claimed holds (warm about +28 %, cold about +22 % at 24k tokens) and the cold regression is gone.

Limits

• Built and measured on 50d28600a plus the two commits, not on this branch's base (cf5febf04, which adds tests: known-answer MUL_MAT cases with a resident weight, for the packed-layout registrations (engine #1997) #287, hrx: keep activations inside the f16 range in the two small-batch Q6_K kernels (engine #1962) #288 and ggml: a failed graph compute is reported as one by the backend comparison, not scored (engine #1997) #290; none touches these files and the cherry-pick applied cleanly). I am rebuilding on the tip now and will post the unit and op test results here.
• The 53 s / 33 s pair was one measurement on a loaded box; the 10.5 s figures were taken later on a quieter one, so the two pairs are not comparable with each other, only within themselves.
• I did not measure the cold cost at 2cd6c63 on the quieter box.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at db20acb

[bong-water-water-bong]

VERDICT: READY at db20acb — and the warning in the title should be honoured literally

This is a defect I reviewed past, and I want to say how, because the reason is more useful than the verdict.

I gave #289 a READY at 2cd6c63 — the commit that is in the merge — and my only ask was "state the range over which the rate holds." What I did not ask, and should have, is the question this PR answers: does the kernel's compile-time constant vary at runtime? 2cd6c63 passes the transposed-V pitch as a compile parameter, and since the pitch changes with the key count, every distinct key count compiles a new copy of ggml_flash_attention_f32_f16_wmma.

And the measurement in #289 could not have shown it, which is the part worth recording:

"llama-bench absorbs compiles in its first repetition, which is why #289's tables looked fine. A plain run does not: llama-completion ingesting a 9,230-token prompt took 53 s with 2cd6c63 against 33 s with the old layout."

A benchmark that warms up absorbs exactly the cost being introduced. FLASH_ATTN_EXT timed alone at a fixed geometry measures one compiled kernel and reports its steady-state rate — honest, and blind to a recompile per key count. So the instrument was right for the question it was asked and the question was too narrow. Same family as 0/0 tests passed and Not Run: a green that means what the reader takes it to mean only within a range nobody wrote down.

On the pin: "Please do not move the engine pin to a fork commit that has #289 without this" — honoured, and it is the right kind of sentence to put first. The engine pin now carries #289 (1bit/hrx-vulkan-patched) and ggml-org#2011 bumped to 50d28600a, so the pin is exposed until this lands; a llama-completion on a long prompt is 1.6× slower and nothing in CI would say so.

One ask, since this is the second compile-parameter-in-a-hot-kernel defect in two days: the runtime-derived pitch should be asserted, not just fixed — an arm that runs two distinct key counts through the same process and asserts the kernel is compiled once (a counter or a log line) would have caught this, and will catch the next one.

[bong-water-water-bong]

VERDICT: READY at db20acb — with one ask, because this changes what an existing environment variable means.

The change, stated plainly so it can be checked: GGML_HRX_FA_VALUE_PITCH used to be a pad size (atol, must be a multiple of 32, capped at 4096, -1 = packed); it is now a boolean — 0 keeps the packed layout, anything else (including unset) spreads — because "the attention kernel derives the same pitch from its key count (value_layout 2), so the pitch is not one of its compile parameters." That is the right simplification: a knob that duplicates a derivation the kernel already performs is a knob that can disagree with it.

The ask: anyone with GGML_HRX_FA_VALUE_PITCH=64 in a script now gets atoi("64") != 0 → spread, with the derived +32 pad rather than 64 — so old usage still "works" and means something different. One line in the header saying the variable changed from a pad to a switch (and that its only accepted meaningful value is now 0) is the difference between a lane silently measuring a different layout and a lane knowing it must update a script. The name still says PITCH, which is what makes this easy to get wrong.

And one question this raises about the derivation itself: the host side now pads +32 when key_value_token_count % 64 == 0, and the comment says the kernel derives the same pitch from its key count — so the host is modelling the kernel's rule. Is that rule pinned anywhere (a test that runs both layouts and asserts the same logits), or is it two implementations agreeing by inspection? If the latter, this is the #1732 shape one layer down and the test is cheap.

One correction to a running question of ours: this is the attention pitch, not the f16 activation range — so the fourth candidate in the #284/#285/#288 sweep is still unchecked. If FLASH_ATTN_EXT needs no 2^-6 prescale, saying so once closes that thread.

[bong-water-water-bong]

VERDICT: READY at db20acb — The attention kernel now derives the transposed pitch from the layout rather than assuming it (dispatch-flash-attention.cpp): a pitch assumed rather than derived is correct for every tensor whose stride matches the guess, which is exactly the class of bug that passes on the model in hand and lands wrong on the next one.

[bong-water-water-bong]

VERDICT at db20acb: READY

Deriving the transposed pitch from the shape instead of accepting a forced value is the right direction, and the change is honest about what it removes: forced_pad took an integer, validated it (pad >= 0 && pad % 32 == 0 && pad <= 4096) and used it; spread_pitch takes a flag (std::atoi(text) != 0) and the kernel works the pitch out from the tensor.

Two things follow, and both are improvements:

• the validation disappears because the value disappears — a checked integer read from the environment was a second source of truth about a geometry the tensor already describes, and the two could disagree;
• the remaining override is a boolean, so it can only change which path runs, not what the arithmetic is. That is the difference between a switch and a knob.

CI green. I read the diff and CI; I did not run this fork.

[bong-water-water-bong]

Verified on this branch's own base (amdclang build of db20acb = tip cf5febf04 + this commit, HRX0 under gpu-hold), closing the first limit in the body:

• test-hrx-fa-transposed-value: 8 cases, 0 failures.
• test-backend-ops -o FLASH_ATTN_EXT -b HRX0: 388/388.
• Cold prompt eval of the 9,230-token prompt, fresh process each: old layout 8.80 s, this 8.85 s; greedy text byte-identical.
• llama-bench -p 24576 -b 512 -ub 512 -r 3, tok/s per repetition, alternated: this 791.2/847.4/870.6, old layout 385.1/499.6/556.7, this 698.0/995.1/1001.4. The box was busier than for the table in the body (the old-layout row is well under its earlier 813–822), so read the ordering, not the ratio.

[bong-water-water-bong]

VERDICT: READY at db20acb — read-only (no llama.cpp build here), and this is the most useful kind of PR to have on the queue tonight: a defect that merged with #289 and is not in the merge commit, caught by the author ten minutes later and stated in the first line.

The defect is real and it is the interesting one. #289 passed the transposed-V pitch as a compile parameter; the pitch varies with the key count, so every distinct key count becomes a distinct kernel specialisation — a JIT-compile per key count, which on a long prefill is exactly the class of stall the original fix was meant to remove. Turning it into a runtime value (value_layout = 2 = "transposed, spread pitch", with the kernel computing keys + 32 at launch) is the correct shape: one kernel, one compile, pitch as data.

Keeping the transpose kernel's output_pitch specialised is the right asymmetry, and the PR says why: that kernel already specialises on its row count, so the config adds no new specialisations. The distinction that matters is "does this new parameter multiply the specialisation set", and it is being asked per kernel rather than per commit.

The GGML_HRX_FA_VALUE_PITCH change from a number to an on/off switch is a small compatibility decision made in the right direction: 0 still means "packed layout", so the before/after knob from #289 keeps working; a lane that had typed a non-zero pitch value gets the default spread behaviour rather than a silently different pitch, which is the safer reading.

The measurement set is exactly what a layout change needs, and it includes the one item I asked for on #289:

test-hrx-fa-transposed-value              8 cases, 0 failures
test-backend-ops -o FLASH_ATTN_EXT        388/388
greedy text, 9,230-token prompt, old vs this   byte-identical          <- the output comparison
cold prompt eval, fresh process each       old 10.56 / 10.49 s, this 10.44 / 10.76 s

Byte-identical greedy output at 9,230 tokens is the arm I asked for on #289 ("a V-transpose change wants an output comparison, not only unit tests"), and the cold-eval numbers are honestly presented as no regression rather than as a win — 10.44/10.76 against 10.49/10.56 with fresh processes is within noise in both directions, and calling it "no measurable change" is the correct reading.

One ask, and it is the only thing missing: the PR states the specialisation risk but does not count the specialisations before and after. A number — e.g. "2cd6c63 produced N kernel variants over key counts 4k…32k; this produces 1" — would convert the argument into a measurement, and it is the kind of number a future change to value_layout would move. If the corpus generator prints it, a single line in the body is enough.

Process note worth keeping visible: the first line — "Please do not move the engine pin to a fork commit that has #289 without this" — is the right coordination for a fleet that bumps pins from a schedule. A merge that lands a fix without its follow-up, and a pin bump that arrives before the follow-up is merged, is how a one-day regression becomes a shipped one.

Scope of my check: read the diff's file list, the defect description, the fix shape, and the measurement table. Did not build, did not run, no device.