A container the suite can reach at index_bits 6, and a VQ4P arm in run.sh - #48
A container the suite can reach at index_bits 6, and a VQ4P arm in run.sh#48mfethe1 wants to merge 2 commits into
Conversation
…--index-bits 6 and a VQ4P arm in run.sh The VQ4P apply — and the AVX-512 kernel PR sqliteai#41 adds to it — was unreachable from make check: the synthetic container is always index_bits 8, and a real --index-bits 6 conversion needs the source weights and hours. So a green run asserted nothing about it, on any platform. --index-bits 6 writes the one combination the engine accepts at 6 (4 stages, 64 entries, packed 4x6 into 3 bytes, same packing as convert.py's block_indices_packed). The default path emits byte-identical containers: manifest.json gains no key (the engine reads absence as 8), and the rotary fixture's container_sha256 gate still passes. The VQ4P arm in run.sh runs the same four self-comparisons the engine block runs on VQ3R: records through the C structs (test_container now accepts the WQ_VQ4P fmt byte), chunked == token-at-a-time, SIMD vs CPU baseline, expert cache bit-identity. It builds in milliseconds, so it runs everywhere CI does — ARM takes the NEON vq_rows_p6 path, x86-64 without VBMI takes the scalar one, and PR sqliteai#41's kernel gets the same checks for free once it lands.
2fa0796 to
dee1a48
Compare
|
Following up on the #41 measurement (dispatched vs That makes the arm weaker than it looks on real containers, and I'd rather say so on the PR than let a green run carry it. The check that would actually pin it is the one you named in #41 — bit-identity of the int8 table (or of the routes) against the scalar path — since a broken kernel and a single entry rounding the other way are indistinguishable at logits level on 6-bit indices. The LUT is a pure function of the codebook, so it can be checked exhaustively in milliseconds with no container at all: build the table scalar, build it through the kernel's lane path, If that sounds right I'll send it as a follow-up — |
|
Traced this further after PR #41 merged — the dispatch table now has three real That rules out the LUT-building step as the source: given the same float activations, That confirms the fix precisely: the test needs to hold the int8 table fixed (not derived from a live, backend-dependent forward pass) and compare only the integer-accumulation step across backends. Scoped the harness assuming a
I don't have AVX-512 VBMI or ARM hardware here to build and run the SIMD arm myself, so I stopped short of writing it rather than ship an untested harness — happy to build the portable-reference half now if useful, or defer entirely if this is already in progress on your end. |
|
Built the portable-reference half described above: It holds Built via Exit 0, exact equality (not a tolerance) on every row — correct once the table is fixed, since there's no float step left to disagree on. Before trusting that result I checked the check itself: swapped one lookup in the reference ( What this doesn't cover: the SIMD arm ( Branch: https://github.com/mfethe1/warp/tree/test/vq4p-lut-portable |
marcobambini
left a comment
There was a problem hiding this comment.
This closes a real gap, it was asked for, and I want it. Two changes first, one
of them a regression against your own #42.
Reviewed by running it rather than reading it, and against current main
rather than the base you wrote on — #41 landed after you opened this, so
vq_rows_p6 is now behind the dispatch table. It merges clean and the arm works
there: 53 → 57, exactly +4, no prior check moved. The --rope container's
container_sha256 still matches the shipped fixture, so your byte-identity
claim for the default path holds on this machine too.
1. Blocking: cmp -s instead of same() — this is #42 undone
Both new comparisons call cmp -s directly. #42 replaced exactly that pattern
one commit earlier, and the cache check is the same call site by name. With
cmp hidden behind a PATH that holds everything else:
PASS VQ4P records read through the C structs (6 records over 3 banks)
PASS VQ4P chunked prefill == token-at-a-time
PASS VQ4P SIMD backend matches the CPU baseline (within fp noise)
FAIL VQ4P expert cache changes results
45 passed, 1 failed, 16 skipped
The engine accused of a bit-identity violation because diffutils is not
installed — on UCRT64, which is the platform you wrote #42 for. It is the only
failure in an otherwise clean board.
The SIMD check is fine as it stands: no cmp drops it into the fp branch, which
is a real verdict, and that is what #42 decided for that shape deliberately. It
is only the cache one. Same treatment as the VQ3R cache check directly above:
same "$TMP/p6_seq.bin" "$TMP/p6_cache.bin"
case $? in
0) ok "VQ4P expert cache is bit-identical to no cache" ;;
1) no "VQ4P expert cache changes results" ;;
*) sk "VQ4P expert cache is bit-identical to no cache" "$NO_CMP" ;;
esac2. Please tighten the tolerance — 1e-3 spends margin you have
I mutated the NEON kernel to find out what the arm can actually see. Same
container, same ids, one edit at a time:
vq_rows_p6_neon |
max diff | arm says |
|---|---|---|
| intact | 9.54e-07 | PASS |
j3 shift 2 → 3 (whole index extraction wrong) |
0.002053 | FAIL |
T1/T2 swapped (classic port slip) |
0.002944 | FAIL |
| one int8 LUT entry, +1 step | 0.002944 | FAIL |
It catches all three, so the arm is not decorative — but a grossly wrong
kernel clears the threshold by 2x while the intact one sits 1000x under it.
There are three orders of magnitude between good and broken here and 1e-3 is
parked at the wrong end of them. 1e-5 would leave 10x over the intact noise
and 300x under the broken signal.
Worth noting the last row, because it is #37's point at synthetic scale: a
single LUT entry rounding the other way is indistinguishable from two stage
tables swapped. Both 0.002944. A tolerance cannot separate "broken" from
"rounded differently" here either — it just happens that at 3 layers neither
reaches 1e-3 by accident. That is an argument for tightening the number now and
for the bit-identity gate #37 and #38 converged on later, not against this PR.
3. The source comment claims something the check does not assert
run.sh's new block says:
the p6 accumulate is integer until the per-block fold, so agreement is exact
rather than within fp noise
The check prints VQ4P SIMD backend matches the CPU baseline (within fp noise)
on this host, and your own evidence table says it lands in the fp branch on both
of yours. The reasoning is right about vq_rows_p6 itself; what reaches the
logits also went through the other dispatched kernels. Someone reading that
comment will think the arm asserts exactness and that a fallback message means
something regressed.
4. One scope line I would like in the comment, not a change request
The container is 4 layers — 3 MoE — 8 experts top-2, hidden 128. The same
comparison on a real index_bits: 6 container, on this machine, with the
kernel this PR covers working correctly:
kimi-linear-vq4p (27 layers) : max diff 0.581133, 162632/163840 over 1e-3
tiny6 (3 MoE) : max diff 9.54e-07
The arm never touches a real container, so it will not produce that as a false
FAIL — that part of the design is right and I checked it. But it does mean
"this arm covers it on Cascade Lake / Skylake-SP CI runners without further
work" promises more than three layers of noise can carry, on a kernel whose
known failure mode is depth-amplified. A sentence saying the arm bounds gross
errors and not the discontinuity would keep the next reader from over-trusting a
green run — which is the exact failure CLAUDE.md names two lines above the
text you quoted.
None of this is about the packing, the fmt byte, test_container, or the
default-path byte-identity, all of which check out. Fix 1, tighten 2, adjust the
comment in 3, and I will merge it.
…cope 1. The VQ4P cache check called cmp -s directly, undoing sqliteai#42 one commit earlier at the same call site by name: on a PATH without diffutils (fresh MSYS2 UCRT64) it false-FAILed 'VQ4P expert cache changes results' as the only failure on an otherwise clean board. Now the same() helper with the 0/1/* case, identical to the VQ3R cache check directly above. 2. SIMD-vs-CPU tolerance 1e-3 -> 1e-5. The review's mutation table puts the intact kernel at 9.54e-07 and every single-edit break (j3 shift, T1/T2 swap, one LUT entry +1) at 0.002-0.003: 1e-5 keeps 10x headroom over noise and 300x under the break signal. 3. The block comment claimed 'agreement is exact rather than within fp noise', but the arm lands in the fp branch on every host measured (what reaches the logits also went through the other dispatched kernels). Rewritten to say what the check actually asserts. 4. Scope sentence added: the container is 4 layers / 3 MoE, so the arm bounds gross kernel errors, not the depth-amplified discontinuity mode (real index_bits 6 container: max diff 0.58 at 27 layers with the kernel correct). A green run must not be over-trusted. Verified on macOS arm64 (NEON path): 48 passed, 0 failed, 13 skipped; the VQ4P SIMD check passes through the tightened 1e-5 branch.
|
All four addressed in e29d4f7, verified on macOS arm64 (NEON): 48 passed, 0 failed, 13 skipped.
The single-LUT-entry flip being indistinguishable from a stage-table swap at logits level (both 0.002944) is a cleaner argument for the #37/#38 bit-identity gate than anything I had — noted for the |
|
@marcobambini All four points are addressed in |
Requested by @Ultron09 in #41: teach
make_test_container.py--index-bits 6and wire the VQ4P engine check intorun.sh. This is that PR; the kernel it covers isvq_rows_p6, which #41's AVX-512 work joins.Why
CLAUDE.md names the trap this closes:
Until now
vq_rows_p6had no containermake checkcould reach: the synthetic one is alwaysindex_bits 8, and a real--index-bits 6conversion needs the source weights and hours. My own review on #41 said I had tried to close this and could not — this is the closing half, and #41's kernel gets the same checks for free once it lands.What
--index-bits 6writes the one combination the engine accepts at 6 (src/model.crefuses anything but 4 stages × 64 entries): records carry theWQ_VQ4Pfmt byte, indices packed 4×6 bits into 3 bytes with the same little-endian bit order and[M/B][pos][row]blocking asconvert.py'sblock_indices_packed, and the manifest'sexpert_quantcarriesindex_bits/fmt/bits_per_weightexactly as convert.py writes them.The default path emits byte-identical containers:
manifest.jsongains no key (the engine reads absence as 8), so the rotary fixture'scontainer_sha256gate is untouched. Verified per-file by sha256 at seed 0, plain and--rope, before and after.run.shgains aVQ4P engine (index_bits 6)section running the same four self-comparisons the engine block runs on VQ3R: records through the C structs (test_containernow accepts theWQ_VQ4Pfmt byte — same size and layout as VQ3R, only the payload packing differs), chunked == token-at-a-time, SIMD vs the CPU baseline, and expert-cache bit-identity. A few MB, milliseconds to build, so it runs on every host CI does.Evidence
Same tree (
2fa0796onmainat6430585), two platforms:vq_rows_p6vq_rows_p6Both boards are exactly baseline + 4: no prior PASS moved. The SIMD-vs-baseline check lands in the suite's existing "within fp noise" branch on both hosts, same as the VQ3R arm; the cache check is bit-identical on both. Also: the same-seed VQ4P container is byte-identical across Windows and macOS (6/6 file sha256s equal — the
newline="\n"discipline #47 landed, working), and the 4×6 packing round-trips against the engine'sP6_J0..P6_J3macros on 1,000,000 random rows with 0 mismatches.What this does not claim: quality of the noise codebooks (there is none — it is noise), and nothing about the AVX-512 kernel itself, which stays #41's to land; when it does, this arm covers it on Cascade Lake / Skylake-SP CI runners without further work.