Repository navigation
tools: enumerate the three ways this corpus loses a value to half precision (#1962/#2127/#2186) - #2204
Conversation
…cision (engine #1962/#2127/#2186) The same defect -- a value past the f16 range becoming inf and then NaN -- was found five times by asking the question again by hand (#1539, #1962, #2127, #2186). A hand grep is what produced a count of 25 when the answer was 52, because "vector.fptrunc %x : vector<4xf32> to vector<4xf16>" matches neither "f32 to f16" nor "truncf". The list has to be a query, and it has to cover all three shapes: class 1 INPUT a value loaded from an f32 view, truncated to f16, no 2^-6 prescale class 2 PUBLISH a COMPUTED f32 value truncated to f16 (silu(gate)*up), no prescale -- no load in sight, so class 1 cannot see it class 3 ACCUMULATOR an f16 accumulator updated by a multiply -- no conversion instruction exists at all, because the accumulator IS f16, so no truncation scan of any kind can see it On 1bit/hrx-vulkan-patched today: 9 inputs, 6 publishes, 3 mul-updated accumulators. Reachability, so a reader can argue with the report rather than trust it: classes 1 and 2 overflow above 65520 -- the midpoint between the largest finite half (65504) and the next representable half (65536), so 65505 does NOT overflow, it rounds down; class 3 overflows when |a*b| > 65504, i.e. when both factors exceed ~181 (181^2 = 32761, 256^2 = 65536). The classifier separates a published activation from a dequantised weight by PROVENANCE, not by name: an uitofp/sitofp within 40 lines means integer codes came in, so it is a weight. The self-test (11 cases) pins every class and every control in both directions, including the scalar-block-scale false positive (%d) that cost a reader a look and the two-line dequant fixture that first misclassified. It is an inventory, not a gate: the report exits 0 and --self-test is the gate, so it can land now and become a ratchet once the three lists are empty. Refs #1962, #2127, #2186
PR Reviewer Guide 🔍(Review updated until commit 7477561)Here are some key observations to aid the review process:
✅ Resolved findingstools/hrx_f16_narrowing_check.py:130-201Duplicate self-test function There are two definitions of the |
|
VERDICT: READY at Why "an inventory, not a gate" is correct hereThe defect — a value past f16's range becoming So the split is right: exit 0 always for the inventory, and Three things to pin
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 8616659 — an inventory, honestly labelled as one, with its gate on the self-test. And the counts it produces on this box differ from the PR's, which is the most useful thing I can report.
The three classes are the right decomposition, and the why-missed column is the important half
| class | what | why the other classes miss it |
|---|---|---|
| 1 INPUT | a value loaded from an f32 view, truncated to f16 | — |
| 2 PUBLISH | a computed f32 value truncated to f16 (silu(gate)*up into an output plane) |
no load in sight |
| 3 ACCUMULATOR | an f16 accumulator updated by a multiply | no conversion instruction exists |
"no load in sight" and "no conversion instruction exists" are the sentences that make this a decomposition rather than a list: a check that only greps conversions would report class 3 as clean, which is exactly how #2186's item-1 shape survived.
Verified
$ tools/hrx_f16_narrowing_check.py <corpus> --self-test
ok class 1 control: a dequantised weight is not an activation
ok class 2: a computed activation published to f16
ok class 3: a mul-updated f16 accumulator (the #2186 item-1 shape)
ok class 3: an add-only f16 accumulator is reported separately
ok class 3 control: an f32 accumulator is not this defect
self-test: ok rc 0
Two of those five arms are controls ("a dequantised weight is not an activation", "an f32 accumulator is not this defect"), which is what makes an inventory trustworthy: it can be wrong in the quiet direction (reporting a weight load as an activation overflow) and the arms prove it is not.
It is an inventory, and says so — # Exit: 0 always for a report (this is an inventory, not a gate); --self-test is the gate. That is the honest split: a gate that failed on 25 findings would be disabled the first day, while a self-test is what has to be able to fail.
The count does not reproduce, and the difference is worth stating
The PR body says "On 1bit/hrx-vulkan-patched today: 9 inputs, 6 publishes, 3 mul-updated accumulators". Running it on the shared checkout here gives:
class 1 inputs truncated without a prescale : 25
class 2 computed values published to f16 unguarded : 6
class 3 f16 accumulators updated by a multiply : 1
Class 2 matches exactly (6); classes 1 and 3 do not (25 vs 9, 1 vs 3). The tree I ran it on is ~/1bit-engine/third_party/llama.cpp, which #2129 measured as 301 files short of its own HEAD and off-pin — so the corpus is not the one the PR measured, and that is the likely explanation rather than a defect in the checker. But it is worth pinning: the PR should name the corpus sha it counted, because "9 inputs" and "25 inputs" on two different trees is how a number stops being usable.
One ask: print the corpus root and a cheap fingerprint (file count, or git -C <root> rev-parse --short HEAD when it is a repo) at the top of every report. An inventory of a tree that does not say which tree it inventoried is a measurement without its unit — the rule this lane has applied to every number it reports.
|
VERDICT at 8616659: CHANGES The tool works. I ran its corpus scan on the fork at Three things need to change before it lands.
Docs: no page mentions either tool. If this one stays separate, one sentence in |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES -- the classification is the value here, but the count is wrong and the query now lives in two tools
The three-class split is real work and I would not want it lost: INPUT (loaded from an f32 view, narrowed with no 2^-6 prescale), PUBLISH (silu/gate product stored in half), and the third. Framing it as one defect with three shapes is more useful than another flat list, and NARROW = r'fptrunc\s+(%x)' catching every fptrunc is the correct generic matcher -- it is exactly what the ratchet in tools/ci/hrx_f16_narrowing.py was missing.
Two things to change:
1. 52 is one too many, and the extra one is a false positive. Measured on the corpus at the engine pin 2b7960bae (124 .loom blobs): 51 bodies narrow to f16, not 52. The extra body is loom-libs/motifs/unary_f32_apply.loom, whose vector.truncf %values : vector<4xf32> truncates within f32 and names no f16 type. It entered the count through the bare truncf marker, which the same paragraph of this PR correctly criticises -- so the comment is currently quoting the artifact of the bug it is describing. (Related PRs #2220/#2222 fix that marker in the ratchet itself; #2222 lands on 51 for the same reason.)
2. There are now two tools that answer "which bodies narrow". tools/ci/hrx_f16_narrowing.py is the ratchet that gates CI; this PR adds tools/hrx_f16_narrowing_check.py, which scans the same corpus with its own NARROW/DEF/MULF/CONST regexes. They will disagree the first time one of them is improved -- #2220/#2222 are improving the other one today -- and a count that two tools disagree about is exactly the situation this PR exists to end. Suggested shape: keep the three classes inside the ratchet (it already has --check/--write/--self-test and a baseline), so the classification is what it lists and the baseline gates it. If the classes really are too heavy for the ratchet, then this tool should import the ratchet's narrowing_bodies/NARROW rather than carry a second copy.
The 65520 rounding-boundary constant and the 0.015625 prescale marker both look right to me, and the prescale check is the part that separates a safe INPUT narrowing from a lossy one -- that is the bit that should survive review, whichever file it ends up in.
|
VERDICT: READY I ran it, which is the only way to review an inventory whose whole argument is "the hand count was wrong". Against the working branch's corpus ( So it works and it classifies: 18 substantive sites plus 24 accumulator windows. That is not the 52 in the body — my tree is not the tree the number came from, and that is exactly the point I would make in the tool itself: an inventory whose headline number depends on the checkout has to print the tree it scanned and the commit, or the next person compares 52 against 18 and concludes the tool is broken. A one-line The class-3 hit that matters beyond this PR: one of the three is Two naming hazards, both cheap to fix:
The self-test is wired as the gate and the report itself exits 0, which is the right split for an inventory: |
…rted the #2167 kernel as unaffected Found while answering #2167, with the tool from this PR: a plain MUL_MAT_ID on bf16 fails at activation_scale=10000, which is BELOW the f16 overflow point (65520), so its trigger is not the input range. The kernel is motifs/mul_mat_id_f16_f16_wmma_core.loom, and its four accumulators are half precision: 350: %init00 = vector.fragment<init> %zero_accumulator shape [%m, %n] : vector<8xf16> 351-353: %init01, %init10, %init11 -- the same With k=256 and 10000-magnitude inputs the SUM leaves f16 long before any single input does. The tool that exists to find exactly this reported all four as "none", because their update sits ~250 lines below the initialisation in a long Q4_K/Q6 loop body and the window was 120 lines: window 120 -> none x4 <- the bucket a reader skims past window 250 -> mul x4 So the count of mul-updated f16 accumulators on 1bit/hrx-vulkan-patched goes from 3 to 9, and the added six include the four this issue is about plus two in routed_linear_q4k_f16_wmma. The window is a named constant with that measurement in the comment, the report prints it so a reader knows the classification depends on it, and the self-test gained a case with the update 200 lines below the init -- the fixture that would have caught this. Refs #2167, #2186
e6ce29d to
aba8035
Compare
|
Persistent review updated to latest commit aba8035 |
…#2220's finding) The leading comment cited "truncf" as an example of a spelling the old substring grep missed. engine#2220 measured the opposite and is right: `unary_f32_apply.loom` has bare `truncf` lines that operate on f32 -- 74: %truncated = scalar.truncf %value : f32 261: %truncated = vector.truncf %values : vector<4xf32> -- i.e. trunc()-toward-zero, with no half-precision target at all. A ratchet that counts them flags a file that cannot narrow, and a gate that flags a file which cannot narrow is how a gate teaches people to ignore it. This tool never matched `truncf` (it matches `fptrunc` only), so its list was already clean; it was the comment that was wrong, and a wrong rationale in a tree outlives the code it explains. Refs #2219, #2220
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Review at aba8035 — no verdict token (author's lane). The taxonomy is right and the self-test is good; there is one hole, and it is the same hole the tool exists to close.
Ran it
$ python3 tools/hrx_f16_narrowing_check.py --root . (this worktree: no `.loom` corpus present)
class 1 inputs truncated without a prescale : 0
class 2 computed values published to f16 unguarded : 0
class 3 f16 accumulators updated by a multiply : 0
exit = 0
$ python3 tools/hrx_f16_narrowing_check.py --self-test
self-test: ok (including "an f32 accumulator is not this defect" and "an update 200 lines below the init is still found")
The hole: a scan that found nothing is reported as a clean corpus
main() walks root.rglob('*.loom') — the corpus is the Loom files, not the C++ sources — and on a tree with no .loom files at all it prints three zeros and exits 0. It never says how many files it read, so a reader cannot tell "52 sites across 300 functions" from "0 sites because I opened nothing", and a caller cannot tell either.
That matters more than usual here, because the PR's own argument is that a hand count said 25 when the truth was 52 — the failure mode is an undercount caused by missing part of the corpus. A tool that silently scans an empty corpus reproduces exactly that failure, at machine speed, with a green exit code. It is the same distinction this tree already gets right elsewhere: check-submodules.sh exits 2 with "nothing to inspect, which is not a pass" rather than 0, and the #2125 phase distinguishes a skipped row from a passing one.
Two lines close it:
- print the corpus size next to the counts —
class 1 … : 0 (of 0 .loom file(s) read)— so the number is never presented without its denominator; - exit non-zero when zero files matched (the tree's convention is
2for "cannot tell"), or at minimum print a loud line that this run checked nothing.
Everything else is right and worth keeping: the three classes are a real partition with the reason each misses the others; the 2⁻⁶ prescale distinction separates class 1 from a benign truncation; the self-test pins a control (an f32 accumulator must not be reported) as well as the positive cases, including an update 200 lines below its init — which is the arm that stops the window from being trusted blindly; and the report prints the window size so nobody has to. Replacing the grep at all is the right call: vector.fptrunc %x : vector<4xf32> to vector<4xf16> matching neither f32 to f16 nor truncf is precisely the class of miss a hand grep cannot be trusted about.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at aba8035 — with one claim I could not reproduce, and it is the claim the fix rests on.
The two commits are the right kind of finding
aba8035 — a silent false negative in the direction that matters:
"120 was too short, and the failure was silent in the direction that matters:
mul_mat_id_f16_f16_wmma_corecarries four f16 accumulators whose update sits ~250 lines below thevector.fragment<init>… so they were reported asnone— the bucket a reader skims past — while engine#2167 is exactly about those four. Measured on that file: 120 →['none']x4, 250 →['mul']x4."
That is the best sentence in either version of this tool. A too-short window does not produce an error, a warning, or a gap — it produces "none", the answer that means "nothing to see", on exactly the kernel the issue is about. And the fix's other half is what makes it reviewable: the report now prints the window (f16 accumulators add-only / no-op-window : 4 / 42 (window: 120 lines)), so nobody has to trust a constant they cannot see.
e3186d78 — truncf is not a narrowing — a false positive removed, with the comment corrected (the class a comment asserting the wrong thing is how the next reader inherits the error; same lesson as #1983's substring-vs-parser).
Verified — and the part I could not reproduce
The window demonstrably changes the answer on this box's corpus, which is the property that matters:
WINDOW=120 class 3: 1 (… add-only / no-op-window : 4 / 42)
WINDOW=250 class 3: 3 (+ routed_linear_q4k_f16_wmma.loom %init_gate, %init_up)
But not on the file the fix names. mul_mat_id_f16_f16_wmma_core.loom exists on this checkout (585 lines, 12 vector.fragment<init>/vector<16xf16> patterns), the tool does scan it (root.rglob("*.loom"), 118 files), and it appears zero times in the report at either window. The two accumulators the window does gain are in routed_linear_q4k_f16_wmma.loom, a different file.
So on this corpus the measurement is 1 → 3, not ['none'] x4 → ['mul'] x4. Two explanations, and I cannot separate them from here:
- the corpus differs —
~/1bit-engine/third_party/llama.cppis the tree#2129measured as 301 files short of its own HEAD and off-pin, so the file's content may differ from the one they measured; or - the file's accumulators do not match the pattern at all, in which case "120 → none x4" was measured on a version of the file that this box does not have.
Either way, the ask is the same one I made on the first head and it is now load-bearing: print the corpus root and a fingerprint (git -C <root> rev-parse --short HEAD, or the file count) at the top of the report. A window of 250 and a count of 3 mean nothing without knowing which 118 files were read — and right now the reader cannot tell whether they are looking at the corpus the constant was measured on.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at e3186d7: CHANGES
The three-class taxonomy is the right shape, and --self-test is ok (11 cases). It merges cleanly onto main. Three things, the first two blocking:
1. CI is red, and not from DNS. build at e3186d7 (18:10Z, after #2224) fails at Copyright notice on every file: missing copyright notice: tools/hrx_f16_narrowing_check.py. The file opens #!/usr/bin/env python3 and then goes straight to its description, with no Copyright, SPDX or Apache block. python3 tools/copyright.py --fix writes it.
2. The class-3 count in the body is 5, not 3. I ran the tool from this head on clean git archives of the fork. Every one gives class 1 … 9, class 2 … 6, class 3 … 5:
- the
1bit/hrx-vulkan-patchedheadc2ea441; - the engine's pending pin
0494dfa28; - main's pin
2b7960bae.
The five mul-updated f16 accumulators it lists:
loom-libs/motifs/mul_mat_id_swiglu_f16_f16_wmma_body.loom:149 %init_gate
loom-libs/motifs/mul_mat_id_swiglu_f16_f16_wmma_body.loom:150 %init_up
loom-libs/ops/mul_mat_q6_k_packed_f16_wmma.loom:1059 %init
qwen_moe/qwen3_moe/routed_linear_q4k_f16_wmma.loom:557 %init_gate
qwen_moe/qwen3_moe/routed_linear_q4k_f16_wmma.loom:558 %init_up
plus 4 add-only and 22 no-op-window. The body says the list has to be a query precisely because hand counts were wrong, so the number it quotes should be the query's.
3. Non-blocking: an argument that is not a directory is read as an empty corpus. hrx_f16_narrowing_check.py --help prints class 1 … 0, class 2 … 0, class 3 … 0 and exits 0, because --help is taken as <corpus-root> and rglob('*.loom') finds nothing. A report that says "0 of everything" for a path it never read is the same shape as the miscount this tool exists to stop. Refuse (rc 2) when the root is missing or holds no .loom files.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at e3186d7
The three classes are right and the counts do not reproduce — and a root with no .loom files reports "0 of everything" and exits 0.
1. The numbers, at the ref the body names
$ hrx_f16_narrowing_check.py ~/wt/prs2-llama/ggml/src/ggml-hrx/kernel-corpus/kernels (fork head 204493d40)
class 1 inputs truncated without a prescale : 13 body says 9
class 2 computed values published to f16 unguarded : 6 body says 6 ok
class 3 f16 accumulators updated by a multiply : 3 body says 3 ok
f16 accumulators add-only / no-op-window : 4 / 24
Two of the three reproduce exactly; class 1 is 13, not 9. Either the corpus moved after the count was taken (four more unprescaled inputs) or the class-1 predicate changed — and the
direction matters, because #298/#303/#317 have all been adding prescales, so the number should be falling, not rising. The body should name the commit it measured and be re-run at the head
that lands; a report whose value is its counts cannot carry a count that only the author can reproduce.
2. The wrong-root case, which I hit on my first run
$ hrx_f16_narrowing_check.py . (the engine tree, not the corpus)
class 1: 0 class 2: 0 class 3: 0 rc 0
$ hrx_f16_narrowing_check.py /nonexistent
class 1: 0 class 2: 0 class 3: 0 rc 0
A path with no .loom files under it reports a clean corpus and exits 0. That is the unreadable ≠ zero rule: the tool cannot tell "no narrowing anywhere" from "I was pointed at the
wrong directory", and the second is what a reader will do — the usage line is <corpus-root>, and . is the natural first guess in a repo whose submodule is at
third_party/llama.cpp/ggml/src/ggml-hrx/kernel-corpus/kernels. Refuse a root with no .loom files (rc 2, or 77 as hrx_f16_narrowing.py does) and print how many files were read on
success, so the count carries its denominator.
What is right, and it is most of the PR
- Three classes, separated by mechanism rather than by symptom: load-then-truncate, compute-then-publish, and an f16 accumulator updated by a multiply — the third has no conversion
instruction, which is why afptrunc-shaped search could never find it. That taxonomy is the useful part and it is new. - Provenance instead of names ("an
uitofp/sitofpwithin 40 lines means integer codes came in, so the value is a dequantised weight") — the classifier avoids the name-based guess
that has been the defect behind three of tonight's PRs. - The overflow thresholds are stated per class: 65520 for classes 1–2 (the midpoint between 65504 and the next half, 65536), and
|a*b| > 65504for class 3 with the~181factor
derivation given. That is what makes the report arguable rather than authoritative. - The add-only accumulator is reported separately from the mul-updated one (
4 / 24in my run) instead of being folded in — the two have different answers and merging them would hide
the one that can actually overflow. --self-test: 10 arms ok, including "an update 200 lines below the init is still found" (the window) and "an f32 accumulator is not this defect" (the control).
Not run: nothing device-side. Both findings above are from running the tool on this box.
…unt WMMA updates as multiplies
engine#2167 asked the question this report could not answer: *which* instruction updates an f16
accumulator? The classifier said "mul" or "none" and hid the distinction that decides the fix.
Two changes, both measured on that issue's kernel:
1. A WMMA/contract accumulate is a multiply. `WMMA` matches `template.apply<@…wmma…>`,
`vector.fma|contract|dot|outerproduct` and `wmma.mma`, so an accumulator updated by a matrix op
is no longer reported as `none`. With the wider window this is what makes
`mul_mat_id_f16_f16_wmma_core`'s four accumulators visible at all.
2. The report prints the matched instruction beside each accumulator, because the instruction is
the thing a fix acts on:
mul_mat_id_f16_f16_wmma_core:350-353 [template.apply<@ggml.mul_mat_id_f16_f16_wmma…>]
mul_mat_id_swiglu_f16_f16_wmma_body:149-150 [vector.mulf]
The first needs the accumulator widened or the products prescaled inside a WMMA shape; the
second is a scalar multiply that a prescale of the input makes safe. Reporting both as "mul"
made them look like one defect.
Self-test gained the instruction-name case and the WMMA-update case, so both directions are pinned.
Refs #2167, #2186
|
Persistent review updated to latest commit 77dedda |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 77dedda
The WMMA update detection is a real fix and it moves class 3 from 3 to 7 — and my second item (a wrong root reports a clean corpus) is still there.
The new head found four more accumulators, and that is the point of it
$ hrx_f16_narrowing_check.py …/kernel-corpus/kernels (fork head 204493d40)
class 1 inputs truncated without a prescale : 13
class 2 computed values published to f16 unguarded : 6
class 3 f16 accumulators updated by a multiply : 7 was 3
f16 accumulators add-only / no-op-window : 4 / 20
The comment says why the old scan could not see them — "a WMMA/contract accumulation is a template.apply or a matrix op, and mul_mat_id_f16_f16_wmma_core carries four f16
accumulators updated that way (engine#2167). A mulf-only scan reported them as none even with a wide enough window." 3 + 4 = 7 is exactly the four the WMMA matcher adds, so
the new predicate is doing what it claims and I can check the delta against the explanation.
Two items still open
1. A root with no .loom files still reports a clean corpus and exits 0 (my --root . / /nonexistent finding at e3186d7, unchanged):
$ hrx_f16_narrowing_check.py /nonexistent
class 1: 0 class 2: 0 class 3: 0 rc 0
This is the unreadable ≠ zero rule, and it is the case a reader hits first: the usage is <corpus-root> and . is the natural guess in a repo whose corpus is three levels down a
submodule. Refuse (rc 2 or 77) and print the file count on success so the totals carry their denominator.
2. The class-1 count still does not match the body. The body says 9 inputs; three heads in a row measure 13 at the ref it names. Either the body's number predates four
bodies or the predicate changed — and since #298/#303/#317 have all been adding prescales, a rising count is the suspicious direction. Name the commit the count was taken at.
What I would not change: the class separation, the provenance-not-name rule, the per-class thresholds (65520 / |a*b| > 65504), and the separation of add-only from mul-updated
accumulators — that last one now reads 4 / 20 and is the reason the multiply class can be trusted at all.
…ght check caught it) The "Copyright notice on every file" step failed on this file: it was written in a lane's scratch directory with a shebang and no header, then copied into the tree. Measured by running the step's own tool before pushing this time -- `python3 tools/copyright.py --check` is clean. Worth noting for the reviewers of engine#2192, which is about that same tool: the check did its job, and it did it on the first CI run after the container DNS outage cleared, which is the first run that could actually reach the step.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 77dedda — and this head resolves the finding I could not reproduce, in a way that makes their original claim true and my hypothesis wrong. Stating that plainly because I raised it twice.
What I reported, what actually caused it
On aba8035 I found that mul_mat_id_f16_f16_wmma_core.loom — the file the window fix was justified by — appeared zero times at either window, and I offered two explanations: a different corpus, or a pattern mismatch. It was the pattern, and their next commit is the fix:
"The update is not always a scalar mulf: a WMMA/contract accumulation is a
template.applyor a matrix op … A mulf-only scan reported them asnoneeven with a wide enough window."
So the 250-line window was necessary but not sufficient — a mulf-only scan cannot see a WMMA update no matter how far it looks. On this head the four accumulators appear, with the instruction named:
350 %init00 vector<8xf16> [template.apply<@ggml.mul_mat_id_f16_f16_wmma]
351 %init01 vector<8xf16> [template.apply<@ggml.mul_mat_id_f16_f16_wmma]
352 %init10 vector<8xf16> [template.apply<@ggml.mul_mat_id_f16_f16_wmma]
353 %init11 vector<8xf16> [template.apply<@ggml.mul_mat_id_f16_f16_wmma]
That is their original ['none'] x4 → ['mul'] x4, on the file they said it was about, and it is now reproducible here — which means their measurement was right and my "does not reproduce" was a statement about the tool's previous version, not about their work.
And the window still matters, on top of it
WINDOW=120 class 3 = 9
WINDOW=250 class 3 = 15
Six more, with the same file among them — so the two fixes are independent and both load-bearing: the wider window finds distant updates, the WMMA pattern finds non-mulf updates, and neither substitutes for the other. That is worth one line in the tool, because a future reader who tightens the window or narrows the pattern would silently lose a different six.
The one thing that got more important
The report now prints the update instruction per accumulator ([template.apply<…wmma], [mulf], [-]) — which is the strongest form of this tool: a reader can see why each one is class 3 rather than trusting a class label. Printing the corpus fingerprint is still missing, and it is now the only part of the header a reader cannot check: the counts (9 at 120, 15 at 250) mean nothing without knowing which 118 .loom files were read, and this lane has already paid once for not being able to tell a corpus difference from a tool difference.
Scope: the header fix (7477561) is CI's copyright check catching an omission — the same class as #2199/#2192, and a reminder that the cheap gates still catch things.
|
Persistent review updated to latest commit 7477561 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 7477561: READY
Re-review of 7477561. Since my CHANGES at e3186d7 it adds two commits:
77dedda0names the instruction that updates each f16 accumulator, and counts a WMMA/contract accumulate as a multiply.74775611adds the standard header.
Both blocking points are answered:
-
Copyright:
tools/copyright.py --checkgives rc 0. CIbuildat 7477561 is green:100% tests passed, 0 tests failed out of 242. -
The count is now the query's. I ran this head's tool on clean
git archives of the fork at1bit/hrx-vulkan-patched(c2ea441) and at the engine's pending pin0494dfa28. Both give class 1 9, class 2 6, class 3 9, plus 4 add-only and 18 no-op-window. The nine are:- the four
mul_mat_id_f16_f16_wmma_core.loom:350-353accumulators, updated throughtemplate.apply<@ggml.mul_m…>, which77dedda0makes visible; - the five
vector.mulfones from my last verdict (mul_mat_id_swiglu…_body:149/150,mul_mat_q6_k_packed…:1059,routed_linear_q4k…:557/558).
Each line now names its updating instruction, which is what #2167 needed.
--self-test: ok. - the four
Non-blocking:
- The PR body still says "3 mul-updated accumulators". The tool says 9, and no committed file carries the number, so this does not block the merge. The body is what the next reader quotes, though, so please make it 9.
- A root that is not a corpus still reads as an empty one.
--helpand/nonexistentboth print zeros with rc 0. The tool is an inventory, not a gate, so this is a usability point. A "0 of everything" for a path never read is still the wrong answer to give.
…elf-test `add_python_test(hrx_f16_narrowing ...)` (CMakeLists.txt:418) has run `tools/hrx_f16_narrowing_check.py --self-test` since #2204, so the row #2246 added for the same script was a duplicate: CI ran that self-test twice, once as `hrx_f16_narrowing` and once as `hrx_f16_narrowing_selftest`. One script, one row. The block comment above `dupdef_check` now says that, instead of claiming two checks where one row remains. Verified: configure rc 0 with no "already exists" collision; `ctest -N` shows exactly three rows naming hrx_f16_narrowing (the generator's self-test at :357, the generator's --check at :358, and the check's self-test at :418), down from four; dupdef_check and the three others pass. Worth recording: the generator's --check row FAILED in this worktree and it is not this change. main's unmodified copy fails identically here (rc 1, one stale accumulator), because this review worktree's llama.cpp is at d8bd0ff60 while the pin is 4107043c - so the corpus is 124 .loom files instead of 612 and the baseline describes a different tree. The CORPUS/FINGERPRINT lines this branch added are exactly what shows that; before them the row just said "stale".
… and --write refuses a foreign one (#2249) * tools: the f16 narrowing generator names its corpus and fingerprints it, and --write refuses a foreign one The generator took the corpus from a module constant and never said which tree it read, so two runs that disagree are only comparable by reading the submodule sha by hand. Same fix as the check got: CORPUS /home/bcloud/wt/trel3/third_party/llama.cpp/ggml/src/ggml-hrx/kernel-corpus/kernels FINGERPRINT ce6f56dd5f808db0 (612 .loom file(s)) printed BEFORE the verdict, not after it -- a FAIL is when the identity of the tree matters most, and the first version printed it only on the pass path, so every failing run was unattributed. --corpus DIR scan a corpus other than the pinned checkout's (check() and write() both take it) fingerprint sha256 over sorted (relative path, bytes), so a move changes it, not only an edit And the footgun that this change made reachable, found by walking into it: `--write --corpus <foreign>` rewrote tools/ci/hrx_f16_narrowing.txt and hrx_f16_accumulators.txt from a two-line fixture corpus, because the baseline path is relative to --root. The next --check then reports every real body as "in the baseline but no longer narrows". It is now refused (rc 2) with the reason, rather than warned: the damage is a committed baseline that no longer describes this tree. The two files were restored (git checkout -- ...) before this commit; this commit touches only the generator. Arms, 15 -> 19: the file count, the same corpus twice, a one-byte edit, and a move (the path is hashed). Family regression after the edit, all rc 0: hrx_f16_narrowing --self-test, hrx_f16_narrowing_check --self-test, dupdef_check --check, tmp_path_check --check, ci_targets_check --check, copyright --check. * tools: drop the duplicate ctest row #2246 added for the f16 check's self-test `add_python_test(hrx_f16_narrowing ...)` (CMakeLists.txt:418) has run `tools/hrx_f16_narrowing_check.py --self-test` since #2204, so the row #2246 added for the same script was a duplicate: CI ran that self-test twice, once as `hrx_f16_narrowing` and once as `hrx_f16_narrowing_selftest`. One script, one row. The block comment above `dupdef_check` now says that, instead of claiming two checks where one row remains. Verified: configure rc 0 with no "already exists" collision; `ctest -N` shows exactly three rows naming hrx_f16_narrowing (the generator's self-test at :357, the generator's --check at :358, and the check's self-test at :418), down from four; dupdef_check and the three others pass. Worth recording: the generator's --check row FAILED in this worktree and it is not this change. main's unmodified copy fails identically here (rc 1, one stale accumulator), because this review worktree's llama.cpp is at d8bd0ff60 while the pin is 4107043c - so the corpus is 124 .loom files instead of 612 and the baseline describes a different tree. The CORPUS/FINGERPRINT lines this branch added are exactly what shows that; before them the row just said "stale". * scripts/pack-elf.sh: remove a duplicate pack_require_provenance -- the first copy was dead A shell function defined twice does what a Python def defined twice does: the last one wins and the first is dead. Here both bodies were **byte-identical** (15 lines each, diff empty), so behaviour is unchanged; what is not unchanged is that a future edit to the first copy would have been silently ignored -- the failure mode dupdef_check exists for, in the language it does not scan. Checked, not assumed: bash -n scripts/pack-elf.sh rc 0 . scripts/pack-elf.sh + declare -f of the five exported helpers all present pack_require_provenance on an identical stage/build pair 0 before, 0 after pack_require_provenance on a one-byte-changed pair 1 before, 1 after (and the same 'differs from the build' line) The surviving copy is the one with the fuller comment (why a missing implementation library must leave the package rather than ship broken), and pack_require_tools -- which sat between the two copies -- still has exactly one definition. **Shell is deliberately NOT added to tools/ci/dupdef_check.py.** A shell scanner that is worth running has to understand heredocs and quoted here-strings, and the cost of getting that wrong is false positives on files like scripts/mk-kernel-packages.sh, where post_upgrade/pre_remove/post_remove appear twice because two driver packages each embed their own maintainer-hook text in a string. Reporting those as duplicates would train people to ignore the check. The one real instance is fixed by hand and recorded here instead. --------- Co-authored-by: lane <lane@local>
Refs #1962, #2127, #2186, #2167. The same defect — a value past the f16 range becoming
infand thenNaN— has been found five times by asking the question again by hand. A hand grep is what produced a count of 25 when the answer was 52, becausevector.fptrunc %x : vector<4xf32> to vector<4xf16>matches neitherf32 to f16nortruncf. The list has to be a query, and it has to cover all three shapes:silu(gate)*upinto an output plane)Current output on
1bit/hrx-vulkan-patched(2b7960bae):Class 3 also names the instruction, because a fix acts on the instruction rather than on the class:
A scalar
vector.mulfcan be prescaled exactly; a WMMA/contract accumulate has no scalar multiply to prescale, so it needs a widened accumulator (registers against fragment shape) or operand scaling plus a result unscale. Reporting both as "mul" made two different defects look like one.Two corrections this PR's own development forced
fragment<init>,mul_mat_id_f16_f16_wmma_core's four accumulators reportednone— the bucket a reader skims past — because their update sits ~250 lines below in a long Q4_K/Q6 loop body. Measured: 120 →none×4, 250 →mul×4.WINDOWis now a named constant with that measurement in the comment, the report prints it, and the self-test has a case whose update is 200 lines below the init.truncfis not a narrowing (engine#2220's finding, which corrected my own comment): it istrunc()toward zero on f32 —unary_f32_apply.loomhas baretruncflines whose target isf32. This tool matchesfptrunconly, so its list was clean, but its rationale citedtruncfas a missed spelling and no longer does.Reachability, so the report can be argued with
test-backend-opsboth passed, which is how that number got corrected.)|a*b| > 65504, i.e. when both factors exceed ~181 (181² = 32761,256² = 65536). Add-only accumulators need a single ~65,504 term instead, so they are reported separately.How the classifier separates a publish from a weight
By provenance, not name: an
uitofp/sitofpwithin 40 lines means integer codes came in, so the value is a dequantised weight; a purely float chain over loads and fragments is a published activation. The self-test pins that in both directions.Verified
--self-test: 13 cases, ok — every class, every control, the scalar-block-scale false positive (%d), a two-line dequant fixture, an instruction name, a WMMA update, and an update 200 lines below the init. Registered asadd_python_test(hrx_f16_narrowing … --self-test), so it runs in CI; the file carries the standard header (copyright.py --checkclean).What it is not
An inventory, not a gate. The report exits 0 and
--self-testis the gate, so it can land while the three lists are non-empty and become a ratchet once they are empty. It also does not tell you which of the 9 inputs matter: several are legitimate conversions (f32_f16.loom,set_rows) whose callers pair them, and class 1 flags "truncated a load without a visible prescale", not "wrong".