Repository navigation
ci: the ratchet also covers f16 accumulators, the half-precision shape no truncation scan can see (engine#2186, #2167) - #2231
Conversation
…e no truncation scan can see (engine#2186, engine#2167) The ratchet in #2114/#2220 matches `fptrunc` -- a value converted to f16. That is the first two of the three shapes that lose a value to half precision, and it is blind to the third **by construction**: an f16 accumulator has no conversion instruction anywhere, because the accumulator already IS f16. The sum leaves 65,504 inside `vector.mulf`/a WMMA accumulate. engine#2167 is exactly that: a plain MUL_MAT_ID on bf16 fails at `activation_scale=10000`, which is well below the f16 overflow point, because 256 terms of that magnitude accumulate past it. engine#2186 item 1 is the same class in the routed gate/up. So this adds a second baseline beside the body list: ACC_INIT `vector.fragment<init> … vector<Nxf16>` ACC_MUL `vector.mulf`, `template.apply<@…wmma…>`, `vector.fma|contract|dot|outerproduct`, `wmma.mma` ACC_WINDOW 250 lines past the init The window is the part that matters and it is measured, not chosen: at 120 lines, `mul_mat_id_f16_f16_wmma_core`'s four `vector<8xf16>` accumulators read as unaffected -- their update sits ~250 lines below in a long Q4_K/Q6 loop body -- and a gate that reports a real overflow as unaffected is the failure mode this whole family keeps producing. The self-test has a fixture whose update is 200 lines below the init, plus a WMMA-update case and an add-only negative. The baseline is 7 files, and three of them are worth naming: `flash_attention_f32_f16_wmma.loom`, `flash_attention_decode_f32_f16_wmma.loom` and their qwen_moe siblings accumulate in f16 as well. For attention that is a *sum over the context*, so the longer the prompt the more room the accumulator needs -- a candidate this issue family had not had named for the long-context NaNs (#1536, #1346), and one the body-granularity ratchet could never have surfaced because attention's logits are f32 and its narrowing is not the thing overflowing. Verified end to end against a real corpus through a temp root: with the baseline, `51 narrowing bodies, none new, none stale; 7 f16 accumulator bodies, none new, none stale`; adding one new accumulator file fails with `NEW f16 accumulator … add the line and say what the accumulator holds`. Refs #2186, #2167
PR Reviewer Guide 🔍(Review updated until commit a7051bc)Here are some key observations to aid the review process:
✅ Resolved findingstools/ci/hrx_f16_narrowing.py:144-174Missing Fix Implementation The PR introduces a ratchet to detect f16 accumulators, which is a necessary step towards fixing issues #2186, #2167, and #1536. However, the actual fix implementations for these issues are not included in this PR. The ratchet detects the presence of f16 accumulators but does not implement the scaling or other fixes needed to prevent overflow or NaN issues. tools/ci/hrx_f16_accumulators.txt:1-20Baseline File Creation The PR creates a new baseline file |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at cf2528f: CHANGES
The third class belongs in the gate, and the 250-line window is the right size. But ACC_MUL does not know vector.mma, the instruction most of these accumulators are actually updated by. The baseline therefore misses four real f16-accumulator bodies, including the one #2186 measured overflowing.
Measured on main's pin 2b7960bae (clean git archive of the corpus, this head's tools/ci/hrx_f16_narrowing.py and both baselines, --check --root):
ACC_MUL |
result |
|---|---|
| as in this PR | rc 0, 7 f16 accumulator body(ies), none new |
plus vector\.mma\b |
rc 1, 4 new, 0 stale of 7 |
The four, each an f16 vector.fragment<init> whose loop-carried value is the third operand of vector.mma … : vector<16xf16>, vector<16xf16>, vector<8xf16>:
qwen_moe/qwen3_moe/routed_down_quantized_f16_wmma.loom init :261 vector.mma :428 (%half_acc00 …) <- #2186 item 1, the routed down
qwen_moe/qwen3_moe/dense_linear_quantized_f16_wmma.loom init :367 vector.mma :418
loom-libs/ops/flash_attention_decode_split_f32_f16_wmma.loom
qwen_moe/qwen3_moe/flash_attention_decode_split_f32_f16_wmma.loom
The three flash-attention files the baseline does list are listed by coincidence. Their accumulator (%product_init, vector<8xf16>) is updated by vector.mma at flash_attention_f32_f16_wmma.loom:507 / decode:277. What ACC_MUL actually matched is an unrelated f32 vector.mulf 180 and 130 lines below (%scaled_current_sum = vector.mulf … %current_sum, %old_scale, the online-softmax rescale). The split-decode variants have no such line within 250, and that is the only reason they are missing. The rule is "an f16 init with any multiply nearby", not "this accumulator is updated by a multiply".
Ask:
- Add
vector\.mma\btoACC_MULand re-baseline. That gives 11 bodies here. - Better: follow the init through its
scf.foriter-arg and require the update to name it (the thirdvector.mmaoperand, or amulf/WMMA result yielded into it). That would make the flash-attention entries right for the right reason and stop an unrelated nearbymulffrom adding entries. - Add a self-test arm with a
vector.mma-updated f16 accumulator that has no other multiply in its window.
The same blind spot is on main: tools/hrx_f16_narrowing_check.py (#2204), WMMA = template.apply<@…wmma…>|vector.(fma|contract|dot|outerproduct)|wmma.(mma|load|store), so its "class 3: 9" undercounts too. On the overlap: two classifiers for one class will drift (this is that drift). I would make the inventory tool import the ratchet's predicate, or the reverse, rather than keep two regexes.
|
VERDICT: READY at Why a second ratchet is needed, stated as a construction argument"The ratchet from And the instance is real, not hypothetical: #2167 — a plain The 250-line window is the part I would keep in the body for a year"The window is measured, not chosen. At 120 lines, Two asks
|
…s sibling (CI caught this) Two failures in one CI step, both worth recording because both are the guard doing its job: 1. `no comment style known … tools/ci/hrx_f16_accumulators.txt` -- the tool refuses an unknown style rather than passing it (the behaviour its own comment praises). A `.txt` baseline is data, so it needs a line beside `hrx_f16_narrowing.txt` in the known-style list. 2. `missing copyright notice` -- my header had the copyright and SPDX lines plus an explanation and no licence body. `--fix` supplied it. Then `--fix` collapsed the blank `#` separator lines in my explanation (the defect engine#2228 fixes), so the paragraphs were rewritten by hand afterwards; the notice is unchanged and `copyright.py --check` is clean. That is the second and third time this guard has caught this lane tonight -- a missing header on a new .py, an unknown style on a new .txt, then a missing licence body -- and each time the right response was to run the step's own tool locally, which now takes ten seconds.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY -- with one measured ask: the 250-line window is a heuristic that already runs within 15 % of its own limit, and a function scope would be exact
The framing is the valuable part and it is right: an f16 accumulator has no conversion instruction because the accumulator already is f16, so a fptrunc scan is blind to it by construction -- that is why #2167 and #2186 item 1 were found by hand and why neither could trip the existing gate. Splitting the baseline (hrx_f16_accumulators.txt, same --write symmetry) rather than widening the narrowing list is also the right call: they are different shapes and a single count would hide which one grew.
What I measured on the corpus at the registry pin 2b7960bae (124 .loom bodies), using this PR's own two regexes:
| init -> multiply pairs found | 14, across 6 bodies |
| distance | min 31, median 146, p90 211, max 213 |
| pairs more than 60 lines apart | 10 of 14 |
pairs crossing a func.func boundary |
0 |
ACC_INIT with no ACC_MUL inside 250 lines |
17 |
Two things follow.
1. ACC_WINDOW = 250 is nearly binding in practice. The real distances are median 146 and max 213, so the margin between the longest true pair and the cut-off is 37 lines. An edit that moves a fill or adds a comment block between the init and the multiply silently drops that body out of the ratchet -- the failure mode is "the gate stops looking" with no diff to show it. Since 0 of 14 pairs cross a function boundary, the search can simply be scoped to the enclosing func.func instead of counted in lines: that is exact rather than heuristic, drops the constant entirely, and costs nothing measurable. Line windows are the same shape of instrument as a bare grep count, which is what #2219 was.
2. The 17 unmatched inits should be named, or the invariant asserted. I spot-checked the first one (flash_attention_f32_f16_wmma.loom:736): it is inside a check.case, a check.generate.fill value(1.0) : tensor<…xf16> fixture that never multiplies. If all 17 are of that kind -- f16 test fixtures rather than accumulators -- one comment saying so is enough, because otherwise a third of the init corpus is being excluded by a rule nobody wrote down, and the next person to add a kernel will not know whether their in-range-less init is exempt or broken.
Everything else: ACC_MUL covers vector.mulf, vector.fma|contract|dot|outerproduct, the wmma template.apply and the wmma.mma|load|store forms, which is the right set; the ACC_INIT capture of the lane count is the useful part for a future "how wide is the accumulation" check; and the baseline lives beside the narrowing one rather than inside it.
Not blocking -- the ratchet as written finds the #2167 and #2186 items -- but the window is the one line in this PR I would not want to inherit.
|
Persistent review updated to latest commit a7051bc |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at a7051bc: READY
Accumulators get their own ratchet rather than being folded into the narrowing one, and that is the right call for a reason today made concrete. ACC_INIT (declares an f16 fragment) and ACC_MUL (is updated by a multiply / WMMA / contract) are two patterns with a window between them, plus their own baseline file — so "this body accumulates in f16" is a conjunction of a declaration and an update, not a substring.
That separation matters because I got the neighbouring ratchet wrong today (#2220, and I closed my own #2225 over it): matching a name like truncf broadly caught a body that truncates f32 to f32 and narrows nothing. A conjunction of two specific facts is harder to over-match than any single marker, and keeping it in a separate file means the two enumerations cannot drift into each other.
It also lands with baseline lines rather than an assertion of zero, which is the same discipline the narrowing ratchet uses: the point is to see the set change, not to declare it empty.
|
VERDICT: READY at The argument is structural and I want to record it because it is the reason a ratchet needed extending rather than duplicating: the existing check matches The shape is consistent with what already works: a second baseline file ( Two asks:
Scope of my check: fresh configure rc 0, the 15-arm self-test (green), and a read of the new regex, the second baseline and the reuse of |
|
VERDICT: READY at The number is explained in the code, not only in the bodyThat is the right home for it. A constant with a comment naming the exact case it was chosen for ( One line still worth addingThe limitation, stated as a limitation: an accumulator initialised more than 250 lines before its first multiply is not reported. The two comments say why 250 is right for this corpus; they do not say what falls outside it. That sentence matters for the same reason as the If there is a cheap way to make it self-reporting — "N ACC_INIT fragments had no ACC_MUL within the window; see Why this and #2232 belong in the same hourBoth are the same finding from different ends: the instrument's definition was narrower than the artifact it reports on — a |
|
Verified from the CI log that the ratchet actually runs — and that it passes at the engine's pin, not just at the commit I generated the baseline from. Two things this settles:
The fragility, stated so it is not a surprise: the baseline is commit-specific while the pin moves, so a |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY -- the new fixture is the right idea, but it pins 200 while the corpus reaches 213, and the window's reach is 249 while the comment says 250
The added arms are exactly the coverage I asked for, and the add-only arm is the one that was missing: a fragment initialised to f16 and updated by vector.addf is not an f16 accumulator in the overflow sense, and excluding it is what keeps this list meaningful rather than "every f16 fragment".
Two numbers to correct, both cheap and both about the same constant.
1. lines[i:i + ACC_WINDOW] reaches 249, not 250. Python's slice end is exclusive, so with ACC_WINDOW = 250 a multiply at offset 250 is outside the window. The comment says the pairs "sit ~250 lines below ... 250 finds them", and the code cannot find an update at exactly 250. Either the constant becomes 251, or the comment says 249 -- the current text is a claim the code does not keep, which is the kind of sentence that later gets trusted.
2. The fixture sits at 200 lines, and the corpus's worst real case is 213. Measured at the pin this issue uses (2b7960bae, 124 .loom bodies), with this PR's own two regexes:
| pairs found | 14 across 6 bodies |
| distance | min 31, median 146, p90 211, max 213 |
So " // pad\n" * 200 asserts a displacement below the largest real one, which means the arm passes while the hardest real case has 13 lines less headroom than the fixture pretends. Padding to 220 (or to an explicit ACC_WINDOW - 25) makes the fixture test the thing the constant is supposed to survive, instead of a case comfortably inside it.
Neither is a defect in the finding -- the ratchet still finds #2167 and #2186 item 1 -- and both are one-line changes in the file this PR already touches. The reason I am raising them at this level of nit is that this constant is now load-bearing twice: once for the real corpus (worst case 213, i.e. 15 % of margin at 250) and once for the fixture (201). A future editor who lowers it to "the value the test needs" would go to 205 and pass, while quietly dropping two real bodies out of the gate.
Suggested wording for the comment, since it currently claims more than it measures: updates sit up to 213 lines below the init at the pin (measured over 124 bodies); the window is 250 for headroom, and the slice covers 249.
Refs #2186, #2167. The ratchet from
#2114/#2220matchesfptrunc— a value converted to f16. That is the first two of the three shapes that lose a value to half precision, and it is blind to the third by construction: an f16 accumulator has no conversion instruction anywhere, because the accumulator already is f16. The sum leaves 65,504 insidevector.mulfor a WMMA accumulate.Why this is not hypothetical:
#2167is exactly that — a plainMUL_MAT_IDon bf16 fails atactivation_scale=10000, far below the f16 overflow point, because 256 terms of that magnitude accumulate past it.#2186item 1 is the same class in the routed gate/up. Both were found by hand; neither could trip the existing gate.What it adds
The window is measured, not chosen. At 120 lines,
mul_mat_id_f16_f16_wmma_core's fourvector<8xf16>accumulators read as unaffected — their update sits ~250 lines below in a long Q4_K/Q6 loop body — and a gate that reports a real overflow as unaffected is the exact failure mode this family keeps producing. My own tool had the same defect and the same fix.The baseline is 7 files, and three of them are new information
Including
flash_attention_f32_f16_wmma.loom,flash_attention_decode_f32_f16_wmma.loomand theirqwen_moesiblings, which accumulate in f16 too. For attention that is a sum over the context, so the longer the prompt the more room the accumulator needs — a candidate for the long-context NaNs (#1536,#1346) that this family had not named, and one the body-granularity ratchet could never surface: attention's logits stay f32, so the narrowing is not the thing that overflows there.Verified end to end, through a temp root with a real corpus
--self-test: ok — including a fixture whose update is 200 lines below the init, a WMMA-update case, and an add-only negative (an add-only f16 accumulator needs a single ~65,504 term rather than a product, so it is not this class).Scope
Adds a check and a baseline; does not change any kernel. If a reviewer would rather this live in the inventory tool (
tools/hrx_f16_narrowing_check.py, merged in#2204) than in the ratchet, that is a fair call — but the ratchet is the thing that fails a PR, and this is the class that has slipped through twice.