Repository navigation
hrx_f16_narrowing: match the type pair, so the vector form is visible and an f32 truncation stops being listed (#2219) - #2222
bong-water-water-bong wants to merge 3 commits into
Conversation
…s "at their pinned commit" engine#2129 is already fixed on main by tools/ci/submodule_pin_check.py -- another lane's, registered in cmake/tests-submodule-pin.cmake as a ctest row -- and it reads `git submodule status`, which reports four states. This adds the fifth, which that command cannot report, and the wiring that runs the check before a build instead of only in CI. **The missing state: at the pin, not clean.** `git submodule status` prints ' ' for "the checkout IS the pinned commit" whatever the submodule's own work tree holds, and the build compiles that work tree. Measured on the box that motivated #2129: on top of the checkouts being off-pin, `third_party/llama.cpp` had 301 uncommitted paths, `xdna-driver` 32, `tokenizers` 20, and four others 4 each. Every one of those would read as "at their pinned commit" to the existing check. Reported always, with its count, and a failure only under the new `--require-clean`: a deliberate local patch in a submodule is a real thing a box does, and moving the pin is not always the caller's to do, so the default is to say it rather than to stop. Five self-test arms, including that a dirty '+' row is not double-counted (the commit is the failure there). **The wiring: nothing ran it before a build.** A ctest row runs in CI; the drift was measured on the box, where the release and the weekly build actually run. `scripts/weekly.sh`'s preflight now runs the check for the `bumps` and `build` stages through `preflight_submodules()` (a function of its own, testable against stub checkers like `preflight_runner_units`): exit 1 **blocks** the run -- the build would not be of the pinned sources -- and exit 2 (no submodule initialised, i.e. a fresh clone) only **warns**, saying that the check inspected nothing rather than implying a pass. tools/ci/submodule_pin_check.py --self-test 19 arms, 5 of them new tests/weekly_submodule_preflight_test.sh 6/6, the function cut out of weekly.sh, stub checkers tools/copyright.py --check clean tools/ci/tests_referenced_check.py 83/83 referenced `--require-clean` is deliberately not passed in the preflight: a dirty submodule is reported there and does not stop a build, while a wrong commit does. This supersedes #2177, which grew a parallel `scripts/check-submodules.sh` before main's tool landed. Two checks for one property is the failure mode the standing rules name, so that script is withdrawn rather than maintained beside it.
…it was a no-op that reported success
Found by checking my own wiring against where the variables are set rather than against whether the tests pass. `SRC=$W/src`
is created by `checkout`, which the `bumps` and `build` stages call, and `preflight()` runs before any stage -- so
`[ -f "$SRC/tools/ci/submodule_pin_check.py" ]` was false on every real run, the function returned 0, and the gate inspected
nothing while reporting success. A preflight row that cannot see the tree it is meant to inspect is worse than no row: it
reads as verified.
The gate now runs at the top of `build()`, immediately after `checkout` has updated `$SRC`'s submodules and before the first
compile -- the tree that is about to be compiled, stopped before the compile rather than after it. `build()` returning
non-zero fails the stage.
And the missing-checker path now says so. An older ref does not carry the tool, so that build's submodule drift is not
checked; it prints a WARNING naming the ref's reason instead of returning 0 silently. That was the same defect one level up:
the function's first version treated "no file to run" as "fine", which is the reporting shape this whole check exists to
remove.
tests/weekly_submodule_preflight_test.sh 6/6, including that a missing checker warns rather than passing quietly
tools/ci/submodule_pin_check.py --self-test 19 arms
tools/copyright.py --check clean
… and an f32 truncation stops being listed (engine#2219)
The ratchet matched the substrings `f32 to f16` or `truncf`, which sees the scalar narrowing and misses the vector one:
%x = scalar.fptrunc %y : f32 to f16 matched
%h = vector.fptrunc %v : vector<4xf32> to vector<4xf16> not matched
Measured on the corpus at the pin 2b7960bae (git archive of the submodule commit, then --root):
233 fptrunc lines = 101 scalar + 132 vector
bodies, old matcher = 25
bodies, type-pair = 51 (27 more, the gap the issue describes)
The `truncf` alternative is dropped, and that is a correction rather than a tidy-up: it was the only thing matching
loom-libs/motifs/unary_f32_apply.loom, whose narrowing is spelled
%a = scalar.truncf %value : f32
%b = vector.truncf %values : vector<4xf32>
-- a truncation that stays inside f32 and names no f16 type at all, listed as a narrowing to f16 for as long as this
ratchet has existed. No body in this corpus spells an f16 narrowing as a `truncf` the two type patterns miss (checked), so
dropping the alternative removes exactly that line and nothing else. That is why the count goes 25 -> 51 and not 25 -> 52:
the issue's 52 is the old matcher's output, one of which was never narrowing to f16.
Both arms are intact and the baseline was written by --write against the pin, not by hand: `--check` reports
"51 narrowing body(ies), none new, none stale; 10 of them carry a guard marker". Against the old matcher the same baseline
fails with "FAIL (1 new, 27 stale of 51)" -- the 27 the issue found, and the false positive as the 1 new.
Three self-test arms pin the distinction: the vector form is a narrowing, a truncf naming no f16 type is not, and the two
are decided by the type pair rather than by which opcode is spelled.
The header comment in tools/ci/hrx_f16_narrowing.txt is unchanged; the entries are regenerated.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY (canonical fix for #2219)
Head pkg/2219-f16-ratchet, 51 bodies. Verified against the corpus at the engine pin 2b7960bae (124 .loom blobs fetched from that sha):
- 51 narrowing bodies with this matcher,
--checkclean (none new, none stalein my equivalent run), 27 added over the old baseline of 25, and the staleloom-libs/motifs/unary_f32_apply.loomcorrectly dropped. - The negative test is the reason this is the canonical one: dropping the bare
truncfmarker is not cosmetic. The pinned corpus has exactly one body with atruncfthat is notfptrunc--unary_f32_apply.loom,vector.truncf %values : vector<4xf32>-- which truncates within f32 and names no f16 type. An alternation that keepstruncf(#2220) lists it as a narrowing, which is a false positive in a ratchet whose whole job is to be a list of true narrowings. vector<\d+xf32> to vector<\d+xf16>is the right shape: it matches the 132 vector lines and notvector<4xbf16>.
One remark, not a blocker: this PR also carries cmake/tests-submodule-pin.cmake, scripts/weekly.sh, tests/weekly_submodule_preflight_test.sh and tools/ci/submodule_pin_check.py, which are not part of #2219. If that preflight work has its own issue, splitting it keeps the ratchet change reviewable on its own; if it is deliberate (the corpus comes from a pinned submodule, so a pin preflight is the natural companion), ignore this.
I am closing my own duplicate (#2223) in favour of this one.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c6ab8d8
This is the correct version of the fix, and it caught an error in my own baseline that I made in #2114 and repeated in my review of #2220.
The truncf alternative had to go, and the reason is checkable in one file:
$ grep -n truncf …/loom-libs/motifs/unary_f32_apply.loom
74: %truncated = scalar.truncf %value : f32
261: %truncated = vector.truncf %values : vector<4xf32>
$ grep -c f16 …/loom-libs/motifs/unary_f32_apply.loom
0
Zero mentions of f16. So truncf there is a truncation within f32 — it was never a narrowing to f16, and my #2114 regex matched it as one. That is my defect: the ratchet has carried a false
positive since it landed, in the very list whose purpose is "which bodies narrow an f32 value to f16".
Ran it: --self-test ok (including the #-line and missing-corpus arms), and --check on the corpus gives
51 narrowing body(ies), none new, none stale; 10 of them carry a guard marker
with unary_f32_apply absent from the baseline (grep 0) — so the 51 is the set minus exactly that one body, which is what "25 -> 51, with the 52nd body being the one that was never
narrowing to f16" says.
And I owe a correction to #2220: I reviewed it as "52; the comment's 51 is stale by one". It is the other way round — 52 is the over-count, 51 is right, and the sentence in #2220's comment
that I called stale was the correct number. #2222 does what #2220 should have. #2222 supersedes #2223 as well (the same .txt, and #2223 keeps the truncf alternative, so its 51 comes
out of a different matcher and would re-acquire the false positive the moment a new truncf-only file appears).
What I would keep from the pair: #2222's comment is the better record, because it says why dropping truncf is safe ("No body in this corpus spells an f16 narrowing as a truncf that
these two patterns miss (checked)") rather than just removing it.
|
Closing as a duplicate of #2220, per the lane cap (at most 2 open PRs) and the duplicate rule. #2220 has the same fix, opened 19 minutes before this one; I claimed #2219 on the board without checking that a claim had just landed, which is the same mistake as #2177 earlier today and it is mine to stop repeating. Two things carried over so nothing is lost:
Branch |
Fixes #2219— and the measurement corrects the issue's own count, which is why the baseline came from--writerather than from arithmetic.The gap, measured at the pin
The matcher was
("f32 to f16", "truncf")— substrings. The corpus writes:And the 52nd body was never narrowing to f16
--writeproduced 52, not 51, and chasing the difference found the interesting half of this PR. Thetruncfalternative was matching exactly one body the type pair does not:A truncation that stays inside f32 and names no f16 type at all — listed as a narrowing to f16 for as long as this ratchet has existed. So the real count is
25 → 51, not25 → 52: the issue's 52 is the old matcher's output, and one line of it was a false member.Checked before removing it: no body in this corpus spells an f16 narrowing as a
truncfthat the two type patterns miss. Dropping the alternative therefore removes exactly that line and nothing else, and the rule becomes "the type pair decides, not the opcode spelling".Verification
--checkat the pin with the new baseline51 narrowing body(ies), none new, none stale; 10 of them carry a guard markerFAIL (1 new, 27 stale of 51)— the 27 from the issue, plus the false positive as the 1 new--self-testtools/ci/tests_referenced_check.pytools/copyright.py --checkThe three new arms pin the distinction the tool now draws: the vector form is a narrowing, a
truncfnaming no f16 type is not, and the two are decided by the type pair rather than by which opcode is spelled.Deliberately unchanged
The two bodies #2219 flags as unprescaled (
motifs/f32_f16.loom,qwen3_moe/attention_postprocess_f32_f16.loom) are both in the new baseline, so #2204's classification and this ratchet now agree on the set they are talking about.