Repository navigation
onnx: the device leg's per-call costs are measured, so the docs carry them instead of an open question - #1076
Conversation
400f8f0 to
650592c
Compare
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d5b0a4e-equivalent head — docs only, the promise it closes is genuinely gone, and the numbers are attributed to the instrument that produced them, including the parts that instrument cannot see.
The promise is closed, not softened. docs/onnx.md no longer contains "NPU_C_SYNC_PROF=1 on one call is the next step" — that sentence and its variants are absent from both pages. The one next step left in docs/onnx-pack.md is a column heading in the shape-ownership table (our EP vs "generate m16 ELFs"), which is a different promise and still true. That is the check this kind of PR lives or dies on, and it passes.
The attribution is right, and I can speak to it directly: the 0.24 ms/call figure comes from onebit_npu_gemm_q8_probe's rotating-weight mode, which is part of #1047 — the PR I reviewed and in which I explicitly said the rotating-weight arm ("same shape, same rows, same launches, last rotation re-checked against the host accumulators") was the right instrument and that its device number was the author's to post under a ticket. This is that number, and the doc describes the arm exactly as the code does: one weight (cache hits) versus a different weight every call (what a model pass does — 28 weights of one shape through one kernel in a row).
The arithmetic is self-consistent: 196 calls/pass × 0.43 ms/call = 84 ms of a 1074 ms pass, and 1074 ms is the same pass total #1047's body used, so the two PRs are quoting one measurement rather than two. The table's deltas (+0.246 → +0.054, +0.238 → +0.067) also separate the weight writeback from the rest of the call, which is the distinction a reader would otherwise blur with #1047's own pack-loop number (2.070 ms → 0.074 ms on the host, a different cost).
And it says what it cannot measure, which is why I trust the rest: "candidate 2 is untouched by either, because a probe cannot know how sparse the graph's activations are", and the remaining per-call terms are named as one number inside dev_call_ms rather than split by guesswork. The doc keeps the cause open where it is open — "the next step is the one comparison these numbers cannot make, not a redesign" — which is the opposite of the failure mode where a measurement is published as a conclusion.
No code, no behaviour, no CLI change; docs/onnx.md and docs/onnx-pack.md only.
650592c to
7a14710
Compare
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 7a147108 (rebase-only; the review stands).
This PR's own files are byte-identical between the heads:
git diff 650592c0…7a147108 -- docs/onnx.md docs/onnx-pack.md -> 0 changed lines
so my verdict holds: the NPU_C_SYNC_PROF=1 on one call is the next step promise is gone from both pages (the one remaining next step is a column heading in a different table and still true), the 0.24 ms/call attributes to #1047's rotating-weight probe — the arm I reviewed and named as the right instrument — and the arithmetic ties to the same 1074 ms pass #1047 quotes (196 calls × 0.43 ms = 84 ms). The doc still states what the probe cannot see, which is why the rest is trustworthy.
PR Reviewer Guide 🔍(Review updated until commit 4cd5cd6)Here are some key observations to aid the review process:
|
|
#1076 ready for review at 7a14710 — docs only (+117/−14, two markdown files), all checks green (build 4m59s, pr_agent, 5× CodeQL). It closes the promise both pages made ("cause open, next step is a profile") with five measured items, and the last one changes a headline: on this tree the two EP arms have flipped sign — 930.5 ms/token on the array against 972.9 on the host kernel, same window, two runs each, both agreeing; the profile splits a pass into device calls 802.5 + epilogue 19.0 against the host loop's 879.0, and each side sums to its own measured total exactly. The published 1.13x loss is kept on both pages, dated to the build it came from, with a pointer to the re-measurement rather than deleted — it is not load (the device arm is slower under load: 5.65 ms/call at load 11-14 vs 4.09 here), it is the tree, and the split of it is not bisected because Two independent methods agree to 3.7 % on the host loop at model level (879 ms/pass measured in the running graph, 848 predicted offline from dumped activations) — that is the part I would check hardest if reviewing this. The other measured negative is worth keeping: resident weights remove the writeback for 568 MiB and 3 % of a token, so not taken. |
|
Correction to my ping above: the head it named ( The head to review is
|
7a14710 to
08d9533
Compare
…ot a build delta
docs/onnx.md and docs/onnx-pack.md both close the model-level section with a
promise -- the cause of the 1.13-1.16x loss is a sign without a cause, and the
next step is a profile. That profile has been run, on both arms, in one window:
1. it prices the per-call costs on the array: the one-slot weight cache is
0.24 ms/call (the whole-image BO writeback), the shared epilogue 0.134,
the pack loop 0.06 -- 0.43 of the ~1.3 ms/call the candidate needs;
2. it measures the other candidate without the array at all: dumped from one
prefill, the graph's real int8 activations are 62.1 % nonzero against the
probe's ~99 %, so its host loop is 1.42x faster per pass (1098 -> 770.6 ms)
and the array's kernel advantage is ~1.12x at model level, not 1.7x;
3. it re-measures both arms on the current tree: 930.5 ms/token on the array
against 972.9 on the host kernel, i.e. 4.4 % in the array's favour, from
the same window with two runs each agreeing on the sign. The split closes
on both sides -- device calls 802.5 + epilogue 19.0 + 109 of graph work
against the host loop's 879.0 + 94 -- and the host loop's 879 ms/pass
validates the CPU-only estimate of 848 to within 4 %.
The 1.13x loss the pages report is kept, dated to the session it came from, with
a pointer to the re-measurement rather than deleted. Attribution is stated the
way it was checked, not the way it was first guessed: exactly two commits touch
this path since those figures (#1032 profiling, #1047 the re-pack at ~5 % of a
token), the private NPU runtime the EP links against is unchanged (2026-10-31),
the five design ELFs are byte-identical, and load cannot explain the direction
(the device arm is slower under load: 5.65 ms/call at load 11-14 against 4.09
here). So ~5 % of the 13 % is code and the rest is unestablished -- most
plausibly CPU-side contention in the earlier session, since half of a device
call is host work -- and the pages say the magnitude is the finding, not the
sign, and that it is small in both directions.
Also recorded because it is a measured negative: keeping every weight resident
removes the writeback (delta 0.054/0.067) for 568 MiB of images on the real
export's 197 projected MatMuls, which is 3 % of a token and was not taken.
08d9533 to
5cb4fb3
Compare
|
Persistent review updated to latest commit 5cb4fb3 |
|
Review this head: Two corrections since the ping above, both mine, both worth knowing before you read it:
What survives untouched, because it is direct measurement rather than inference: the both-arms split (device calls 802.5 + epilogue 19.0 + 109 of graph work = 930.5 ms/token against the host loop's 879.0 + 94 = 972.9, both sides summing exactly), the array being 4.4 % ahead in that window, the per-call costs (writeback 0.24, epilogue 0.134, pack 0.06 ms/call), the real-operand host loop at 1.42× (1098 → 770.6 ms/pass), and the resident-weight negative (568 MiB for 3 % of a token, not taken). One claim I would check hardest if reviewing: the host loop's 879 ms/pass measured in the running graph against the 848 predicted offline from dumped activations — 3.7 % apart from two unrelated methods. If that agreement is a coincidence, the real-operand argument weakens; the both-arms split does not depend on it. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at this head (rebase-only; the review stands).
This PR's own files are byte-identical between the heads — git diff over docs/onnx.md docs/onnx-pack.md is empty — so nothing in my verdict changes.
The promise this closed stays closed: NPU_C_SYNC_PROF=1 on one call is the next step is absent from both pages, the 0.24 ms/call attributes to #1047's rotating-weight probe, and 196 calls x 0.43 ms = 84 ms of the same 1074 ms pass #1047 quotes.
|
Persistent review updated to latest commit 4cd5cd6 |
… of them inside the split's file, and the moved block survives it main went bd7d451 -> cb5866e -> 05da4d8 while the five gate rows were being re-measured, and one of the twelve commits edits the file the split is built around: npu/forward/src/q4nx_forward.cpp +38/-5 (#1087 rope-scaling refusal by name, #1072 the batch phase counters zeroed BEFORE BatchPhases snapshots them) npu/forward/include/q4nx_rope.h +72 (new: q4nx_rope_declared_type / q4nx_rope_type_supported) npu/pack_import.cpp/.h +97 (#844 final-norm import from the .fconst tail envelope) app/, config/, docs/, tests/, tools/, os/ (laya-first #1069, determinism tool #1068, serve logging #1074, onnx pricing #1076, puppy vm-check #1080, release-check #1079/#1071) third_party/llama.cpp pin bump 6e42864a6 (#1075) Textually clean (`git merge-tree --write-tree` produced no CONFLICT), which is exactly the case that was NOT clean the last time main landed in that file — so the merge was checked by what the compiler and the tests say, not by "auto-merging" messages: - #1072 moved `BatchPhases bph` / `BatchPhaseScope _batch_scope` / `tp_bs` so the counters are zeroed before the snapshot is taken. Those symbols live in npu/forward/include/q4nx_lh.h in this branch (the move this PR exists for); the statement order is main's and is carried over verbatim, so the reordering lands on the header's definitions unchanged. - The moved set is still defined exactly once: `inline StepPhases g_ph` (q4nx_lh.h:116), `phprof_on`/`ph_ms` (q4nx_lh.h, one each), `inline bool g_in_batch` (:131), `struct BatchPhaseScope` (:132), `lh_dump_layer_host` — and q4nx_forward.cpp no longer defines any of them. - `BatchPhases` remains a local struct in q4nx_forward.cpp (one definition), which is where main keeps it. Build: `cmake --build build-p4 --target onebit -j4` under ~/lb/thermal-run.sh -> rc 0, 16 objects rebuilt (warm tree; the pin bump and the two touched TUs). Tests: `ctest` **87/87, 0 failed** (85 passed, 2 skipped: mtp_batch_all, gdn_batch_order — the same two that skip on main). Consequence, stated rather than implied: the five gate rows measured on A=bd7d451a are VOID and are being re-run against A built at this main; this commit carries no numeric claim of its own.
What this changes, and why now
docs/onnx.mdanddocs/onnx-pack.mdboth close the model-level device-leg section with the same promise: the causeof the 1.13–1.16× loss is measured as a sign but not a cause, and "
NPU_C_SYNC_PROF=1on one call is the next step".That profile has now been run (it is
ONEBIT_EP_QDQ_PROF=1, merged in #1032), and two of the three candidate per-callcosts are measured, so the pages carry the numbers instead of the open question.
Docs only. No code, no behaviour, no CLI change.
The three measurements now in the docs
1. The one-slot weight cache costs 0.24 ms/call — the writeback of the whole packed image (2 MB at 1024×2048).
Priced on the array by the probe mode merged in #1047, which times the same call twice: one weight (cache hits) and a
different weight every call (cache misses, which is what a model pass does — 28 weights of one shape through one
kernel in a row). M=32 designs, 40 repeats, load 9.2, both legs exact against the host accumulators over 32768 values:
2. Keeping every weight resident removes it and is not worth it. Delta falls to 0.054/0.067 ms/call (the pack loop
itself), so residency returns ~35 ms of a 1074 ms token — 3 % — for 568 MiB of int8 images (588 MiB with the
per-context A/C BOs; 140 MiB for the two shapes that have an ELF here), counted off the real export's 197 projected
MatMuls. It was built and verified on the array (the QDQ proof is bit-identical with the knob off and on), and it is
not proposed: this is recorded so the next person does not spend a night re-deriving that the weight path cannot
turn the sign.
3. The shared epilogue is 0.134 ms/call, not the gap. From
ONEBIT_EP_QDQ_PROF=1on the device arm of the sameexport, two runs, identical ids, trace
qdq rows=1764 fallbacks=0 device-busy=0:dev_call_ms5.65 ms/call (1107 ms/pass),dev_epi_ms0.134 ms/call (26.4 ms/pass); the run was at load 11–14 so theabsolutes are not quotable as throughput while the split is, since both terms come from one run. With the graph's other
nodes (~194 ms/pass at that load) the three terms add to the measured 1327 ms/token exactly — a split, not a guess.
4. The host loop is 1.42× faster on the graph's real int8 than on the probe's synthetic draw — measured with no
device at all. Every projected MatMul's int8 activation was exposed as a graph output, one 32-token prefill run at
ORT_ENABLE_BASIC(which keeps the Q/DQ nodes), and the EP's own host loop timed on those bytes against the probe's draw:The real tensors are 62.1 % nonzero against ~99 % synthetic; layer 0 alone is 93.7 %, which is why one tensor is not
a sample of the pass (112 distinct tensors sit behind the 196 MatMuls — q/k/v share one, gate/up share one), and
down_projis the extreme at 55.8 % nonzero (SwiGLU product), 1.76× faster.Against the arms' own numbers — their synthetic column is 1209 ms/pass at m=36 while this dump is at its 32 rows, and on
the synthetic draw the two agree at 1.10× — the graph's operands put the host loop at ~848 ms/pass against a device
multiply of 712 + ~47 ms of one-slot writeback = 759. So the array's kernel advantage at model level is ~1.12×, not
1.7×, and that is why the device arm does not win: it saves ~90 ms/pass on the multiply and still ends 127 ms/token
behind. The multiply is no longer where the loss lives.
5. Re-measured on this tree, the two EP arms have changed sign. Same window, two runs each, back-to-back in one
npu-holdticket, load 4.9–6.9, identical ids,fallbacks=0,device-busy=0,ONEBIT_EP_QDQ_PROF=1on both:Both sides are fully accounted for (802.5 + 19.0 + 109 = 930.5; 879.0 + 93.9 = 972.9), so the array is 4.4 % ahead
here, not 1.13× behind. It is not load — this run was at load 4.9–6.9 against the earlier 1.2–1.35, and the device arm
is slower under load (5.65 ms/call at load 11–14 against 4.09 here). It is the tree: the earlier figures predate
#1047, whose re-pack is 0.24 ms/call = 47 ms/pass (~5 %); the rest came with merges in between and is not bisected,
because
/tmpwas wiped at the 21:24 reboot and the old binary could not be re-run beside the new one. The kernel isunchanged — the probe in this same window still gives q at m=36 at 3.460 device against 5.836 host, the 1.69× the rows
table reports. The host loop's 879 ms/pass also validates the CPU-only estimate of 848 (item 4) to within 4 %.
So the pages keep the old figures, dated and pointed at this re-measurement, and now say the magnitude is the finding
rather than the sign — and that it is small in both directions, so the number is only quotable with its build.
What the pages now say instead of "cause open, next step is a profile"
Candidate "non-kernel work per call" has 0.43 ms/call measured (writeback 0.24, epilogue 0.134, pack 0.06) out of
the ~1.3 ms it would need, and the remainder inside
dev_call_msis the kernel plus the A writeback, launch, two syncs,wait and C readback, which that knob times as one number. Candidate "the host loop is faster on real operands" is now
measured, not assumed: 1.42× per pass, which cuts the array's kernel advantage to ~1.12× at model level. So the
pages name the multiply as not the gap, and the per-call work inside
dev_call_msthat the kernel probe does not seewas then profiled with both arms: 4.09 ms/call at model level against the probe's 3.460 for one shape in the same
window, i.e. the EP adds no structural per-call cost — and the arms finished 930.5 against 972.9 in the array's favour.
Verification
python3 tools/copyright.py --checkrc 0 (no new files);git diff --statis the two markdown files (+56/−14).Both edits are prose and tables; the numbers are the ones in
~/briefs/TQDQ-STATE.mdfor 2026-11-05 00:57 and 01:33,and every one of them was measured on the array in an
npu-holdticket (ticket ids, loads and holds are in that file).