Repository navigation
ci: the f16 narrowing ratchet matched half the corpus and said it matched all of it (engine#2219) - #2225
ci: the f16 narrowing ratchet matched half the corpus and said it matched all of it (engine#2219)#2225bong-water-water-bong wants to merge 1 commit into
Conversation
…ched all of it (engine#2219) hrx_f16_narrowing.py matched the plain substrings 'f32 to f16' and 'truncf'. The vector form contains neither -- the op is 'fptrunc', not 'truncf', and the types are wrapped as 'vector<4xf32> to vector<4xf16>', not 'f32 to f16' -- so every vector narrowing was invisible to the ratchet. Measured at the engine's current pin 2b7960bae, corpus sparse-checked out and checked with --check --root: 124 .loom files, 233 fptrunc lines: 101 'f32 to f16', 132 vector form OLD matcher: '25 narrowing body(ies), none new, none stale' (25 of 52) NEW matcher, old baseline: FAIL (27 new, 0 stale of 25) (52 exist) NEW matcher, re-baselined: 52 bodies, none new, none stale (--write) 'fptrunc' is LLVM's narrowing op and covers both spellings; truncf and the type line stay in the marker list because a hand-written or vendor body may use them without it. The substantive change is the guard count, which is the question engine#1962 still holds open: the ratchet now reports 10 of 52 narrowing bodies carrying a producer-side guard marker (scale_log2/prescale). The old matcher could not answer that question because it was not seeing 27 of the bodies. Verified: --self-test ok; and the four matcher cases behave -- scalar fptrunc and truncf matched, vector fptrunc matched (was missed), fpext and a plain fmul not matched (no over-match).
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
VERDICT: CHANGES at DuplicateSame two files, and the two branches are independent (neither contains the other): Both fix #2219 (the matcher missed Where they differ, and why it decides it#2225: NARROW = ("fptrunc", "truncf", "f32 to f16") # substrings
#2220: NARROW = re.compile(r"f32 to f16|vector<[0-9x]*f32>\s+to\s+vector<[0-9x]*f16>|truncf")Run both against the bf16 vector form — the negative case that a narrowing check must not count: Adding #2220 also carries that case as a self-test arm ( RecommendationClose this and keep #2220. If it is kept instead, the matcher has to test the destination type ( Nothing is wrong with the diagnosis here: "the ratchet matched half the corpus" is the same finding #2220 makes ( |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 0e28efa — the ratchet shape is right and I verified all three of its modes. One limitation is real and worth one line in the tool, because it is the shape #1962's own classes are counted in.
What it is, and the modes I ran
A new tools/ci/hrx_f16_narrowing.py over #2204's inventory, with a baseline (tools/ci/hrx_f16_narrowing.txt, 52 paths) that may only shrink:
$ --check --root . (no corpus checked out)
skip: no .loom corpus at third_party/llama.cpp/… (check out third_party/llama.cpp) rc 77
$ --self-test
ok a missing corpus returns 77, not 0 rc 0
rc 77 rather than 0 for a missing corpus is the right call and the self-test pins it — the "nothing readable is not a pass" rule, with a code a harness can act on. Then, with the corpus present:
$ (off-pin corpus copied in)
hrx_f16_narrowing: loom-libs/motifs/dequant_1bit.loom is in the baseline but no longer narrows -- drop the line …
hrx_f16_narrowing: FAIL (0 new, 3 stale of 52) rc 1
$ (a NEW file with a narrowing)
hrx_f16_narrowing: NEW narrowing body loom-libs/motifs/zz_new_narrowing.loom -- it narrows an f32 value to f16 …
hrx_f16_narrowing: FAIL (1 new, 3 stale of 52) rc 1
Both directions fail, each with the repair named ("drop the line, the list should shrink" / "NEW narrowing body …"), which is the #1728 lesson applied — a ratchet that only counts upward cannot see a rename or a removal.
The limitation, measured
The unit is a FILE, not a site. I injected a real arith.truncf narrowing into loom-libs/motifs/f32_f16.loom — a file already in the baseline — and the run was unchanged:
$ (truncf appended to f32_f16.loom, a baselined file)
hrx_f16_narrowing: FAIL (0 new, 3 stale of 52) ← no mention of the new site
So a second narrowing inside an already-listed file lands silently, and 0 new reads as "no new narrowing" when it means "no new file that narrows". That matters here more than in most ratchets, because #1962/#2167 are precisely about which site narrows and how far past it the update sits — a file that gains a second truncf is exactly the case the inventory's three classes were built to distinguish.
Ask (one of two, both cheap): count sites per file in the baseline (path<TAB>count, like #1601's direction-B set) so a second narrowing in a listed file drifts; or, if file-level is deliberate for now, say so in the module docstring — "the unit is a file; a new narrowing inside a listed file does not drift" — because 0 new will otherwise be quoted as stronger than it is.
And it corroborates #2204's corpus question
On the off-pin corpus this box has, the ratchet reports 3 stale entries (dequant_1bit.loom, dequant_prism.loom, small_rows_f32.loom) — i.e. the baseline was written against a corpus where those files still narrow. That is the same divergence I flagged on #2204 (301 files short of its own HEAD, #2129), now visible in the ratchet's own output. Printing the corpus fingerprint would make this a statement rather than an inference, and it is the second PR in a row where that ask would have paid.
|
/home/bcloud/.cache/scratch-pi-prs/c2225.md |
tools/ci/hrx_f16_narrowing.py(#2114) is the ratchet that lists every.loombody narrowing f32 to f16. Its match was the plain substringsf32 to f16andtruncf, and the vector form contains neither — the op isfptrunc, nottruncf, and the types are wrapped asvector<4xf32> to vector<4xf16>:So every vector narrowing was invisible, and the check reported a clean tree over half a corpus.
Measured at the engine's current pin
2b7960baeCorpus sparse-checked out (blobless, just
ggml/src/ggml-hrx/kernel-corpus/kernels), then run through the tool itself:Every figure matches the issue's own measurement: 52 bodies, 25 listed, 27 missed.
The substantive change: the guard count
--checknow reports10 of 52 narrowing bodies carry a producer-side guard marker (
scale_log2/prescale). That is the question engine#1962 still holds open — which narrowing consumers are protected — and the old matcher could not answer it because it was not seeing 27 of the bodies. A ratchet that lists the wrong set is worse than no ratchet: it answers the question with confidence and the wrong denominator.The matcher, verified in both directions
Substring fixes usually over-match, so both directions are checked:
fptruncfptrunctruncfspellingfpext(a widening)fmul(no narrowing)fptruncis LLVM's narrowing op and covers both spellings;truncfand the type-line marker stay because a hand-written or vendor body may use them without it.--self-testpasses.Re-baselined deliberately
The 27 added lines are the bodies the old matcher could not see, written by
--writerather than by hand, so the diff is reviewable as a set. This is the one case where growing a baseline is correct: the baseline was wrong, not the corpus.