[bong-water-water-bong]

Refs #1788 (F6, MiniCPM-V 4.7). The issue stays open.

Why

vision::tower_forward() (#2056) is the host reference for the vision tower and the only tower the image probe (#2132) has. It ran every loop on one thread and walked each weight matrix once per token, so one 448 × 448 view (1,024 patches) of the 1B pack took 23 to 56 s depending on what else the box was doing. That made every image run mostly a wait for the CPU.

What

• Threads. The token loop of every matrix product and the (window, head) loop of attention are spread over threads: ONEBIT_VISION_THREADS, default the machine's count capped at 16, or vision::tower_threading(threads, min_work) in code. Loops under 4e6 multiply-adds stay on the calling thread.
• A blocked product. linear() now applies each weight row to 16 tokens before moving to the next row. The MLP weight of a block is about 20 MB and a token's input 4.6 kB, so the old loop streamed the whole matrix once per token and was bound by memory.

Neither changes the output. Every token and every attention head is still computed by exactly one thread, with the same sequence of floating-point operations as before.

Measured on this box (1B pack, one 448 × 448 view, box load about 13)

before (main):                 image 448 x 448: 1 piece(s), 64 rows; ... tower (host) 37673 ms      ONEBIT_VISION_THREADS=1, old loop
blocked product, 1 thread:     image 448 x 448: 1 piece(s), 64 rows; ... tower (host) 18148 ms
blocked product, 16 threads:   image 448 x 448: 1 piece(s), 64 rows; ... tower (host)  3187 ms

The comparison with the reference implementation is unchanged at every thread count (16 × 20 grid, 1B pack): output 20480 values, max |d| 2.38e-06, corr 1.000000000.

The box is shared and loaded, so these are single readings, not a benchmark: the same 16 × 20 comparison on one thread read 9.7 s and 20.3 s in two runs minutes apart. The order of magnitude is the claim (tens of seconds to a few seconds), not the third digit.

Test

npu_vision_tower (existing ctest row):

• the fixture's goldens from the reference implementation still hold (max |out - reference| = 7.15e-07, as before);
• new arm: the fixture run on 1 thread, then forced onto 2, 5 and 16 threads with no size threshold, must give byte-identical output rows and tower rows (memcmp). The fixture is small, so without forcing it would never leave the calling thread and the arm would prove nothing; that is what the min_work = 0 argument is for.

$ ./npu_vision_tower_test
vision tower: 15 checks, 0 failed
$ ctest -R npu_vision        100% tests passed, 0 tests failed out of 5

Limits

• Still a host reference: 3 s per view is fine for a probe and not a serving speed, and a 70 × 70 slice has nearly five times the patches with attention growing as the square.
• I did not rerun the image probe on the device with this; the tower's output is bit-identical, so the answers cannot change, but that is an argument, not a run.
• Plain std::thread per loop, no pool. At these sizes thread start-up is noise; it would matter for a tower run many times a second.

Docs: docs/npu.md, the tower paragraph.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 11b91ce

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 73ca0f2)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1788 - Partially compliant Compliant requirements: imports with zero gaps and passes its self-check against a named reference serves on --device npu answers a known-answer prompt through the model's own chat template, 3 identical requests returning identical text a measured decode tok/s and time to first token under npu-hold the row names the kernel set and the engine commit that produced it images: one known image, one known question, an answer that is only obtainable from the picture — not "it accepted the request" tools: a tool-call round trip parsed and answered prefix cache: a repeated prefix (text and a picture) hits the cache and the second TTFT is measurably lower Non-compliant requirements: (No requirements not fulfilled) Requires further human verification: (No requirements requiring further human verification)

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Thread Safety of Global State The g_threads and g_min_work variables are global std::atomic variables that are modified by tower_threading(). While std::atomic provides atomicity for individual operations, there is no synchronization mechanism to prevent race conditions when these values are read and written concurrently. If tower_forward() is called from multiple threads simultaneously, and tower_threading() is called concurrently, it could lead to inconsistent behavior or undefined behavior. The documentation states that tower_threading() should not be used while tower_forward() is running, but this is not enforced at the code level. std::atomic<int> g_threads{0}; // 0: not set, use the environment / the machine std::atomic<double> g_min_work{4e6}; // multiply-adds below which a loop stays on the calling thread Potential Performance Regression The introduction of thread pooling in linear() and attention() functions may introduce overhead for small workloads. The parallel_for function checks if the workload is below a certain threshold (g_min_work) to avoid threading for small tasks, but this threshold is a global default that may not be optimal for all use cases. If the workload is consistently below this threshold, the overhead of creating and managing threads could outweigh the benefits of parallelization. template <typename Fn> void parallel_for(int count, double work_per_item, Fn fn) { // fn(first, last) over [0, count) const int nt = tower_threads(); if (nt <= 1 || count < 2 || work_per_item * count < g_min_work.load()) { fn(0, count); return; } const int parts = count < nt ? count : nt; std::vector<std::thread> pool; pool.reserve(size_t(parts - 1)); for (int p = 1; p < parts; ++p) pool.emplace_back(fn, int(int64_t(count) * p / parts), int(int64_t(count) * (p + 1) / parts)); fn(0, int(int64_t(count) / parts)); for (std::thread& t : pool) t.join(); }

[bong-water-water-bong]

VERDICT at 11b91ce: READY

Reviewed head 11b91ce (refs #1788). This is the first verdict. The claim that matters is "same bits", and I checked it against main directly rather than against the goldens' tolerance.

Same bits as main. I added one temporary line after the fixture's tower_forward(…, GH, GW, &tower), at both main and this head, printing an FNV-1a hash of the raw bytes of out and tower, and built each from an ONEBIT_NPU=ON configure:

[main] CL-HASH out=3323f3d55094d39c tower=c789b25fa29074a8    vision tower: 14 checks, 0 failed
[pr]   CL-HASH out=3323f3d55094d39c tower=c789b25fa29074a8    vision tower: 15 checks, 0 failed

So the blocked linear() reproduces main's single-token loop byte for byte. That run used the default threading, and the fixture sits under min_work, so it is one thread. The new arm then forces 1/2/5/16 threads with min_work = 0 and memcmps them against each other, which chains the threaded result back to main. I removed the probe line (git status clean).

The new arm discriminates. A mutant where the calling thread's slice in parallel_for stops one unit short (fn(0, count/parts - 1)) fails it: rc 1, "fixture: 1, 2, 5 and 16 threads give bit-identical rows". I restored the file and the test is 15/15 again.

Read and checked

• parallel_for's partition is exact: worker p ≥ 1 takes [count·p/parts, count·(p+1)/parts) and the calling thread takes [0, count/parts). Each attention unit (one head of one group) writes its own ctx slice and has its own prob buffer.
• groups = n / group drops no tail. main's inner loops already ran i < group from each g0, so a partial group was never valid. The two callers pass group = n, or 4 on a grid whose sides tower_forward requires to be multiples of 4 (vision_tower.cpp:149).
• In a fresh ONEBIT_NPU=ON configure of this head, ctest npu_vision_pack, npu_vision_tower, npu_vision_plan, npu_vision_image and npu_vision_prompt pass, 5/5. git merge-tree onto main: no conflicts. CI build and CodeQL green.

Not re-run by me: the 37.7 → 3.2 s timings. They are on a loaded box, as the body says, and correctness does not depend on them.

[github-actions]

Persistent review updated to latest commit 73ca0f2

[bong-water-water-bong]

Review at 73ca0f2 — no verdict token (author's lane). The claim that matters is "same bits", and it is proven the only way that makes it believable: the parallelism is over outputs (a token, a head), so each unit is computed by exactly one thread with the same arithmetic as the serial loop, and the arm then runs the fixture at 1, 2, 5 and 16 threads and compares with memcmp. Including 5 is the nice part — it does not divide the work evenly, so it exercises unequal chunking rather than the two friendly counts.

That is the difference between this and a threaded reduction: an inner-product split across threads would give a plausible answer that changes in the last bits with the thread count, and no tolerance-based arm would catch it.

Also right, and each for a specific reason:

• A min_work floor (4e6 multiply-adds) keeps small jobs on the calling thread, so a future small tensor cannot pay 16 thread spawns to do nothing — the usual way a threading change becomes a slowdown.
• Capped at 16 with ONEBIT_VISION_THREADS to override and 1 to turn it off; documented at docs/npu.md:1636 rather than only in the code.
• No perf threshold in tests/ — correctly omitted. A wall-clock assertion in a unit test is a flake generator, so the 37.7 → 3.2 s number belongs in the body, not in ctest.

One thing the test cannot protect, worth a line of visibility: the 12× rests on the min_work threshold and on the loops being large enough to cross it. If someone later changes g_min_work, or a shape change makes the loops smaller, the code silently takes the inline path — every arm above still passes, because bit-identity holds trivially when nothing is threaded, and the only symptom is "image runs are slow again" some days later. Not a reason to add a timing arm; a reason to make the fallback observable, the same way the sealed-set paths announce themselves:

[vision] tower: N thread(s), M of K loops threaded (min_work 4.0e6)

one line under ONEBIT_VISION_DEBUG or into the image probe's output would turn "why is this slow" into a reading. That is the only gap I found, and it is about seeing the fast path rather than believing it.

[bong-water-water-bong]

VERDICT: READY at 73ca0f2 — fresh configure rc 0, built and ran npu_vision_tower_test: vision tower: 15 checks, 0 failed, with the numeric parity unchanged (fixture: max |out - reference| = 7.15e-07; tower sum -0.859799 vs -0.859797 — the same 7e-7 as before the change).

The 37.7 s → 3.2 s is the headline, and the assertion that makes it safe is the one stated as a property rather than measured as an average: "every token and every attention head is still computed by exactly one thread, with the same sequence of floating-point operations". That is the only kind of threading change I will approve on a reference implementation without a bit-exactness arm: if each work item is owned by one thread and the per-item order is unchanged, the output is identical by construction, which is what the unchanged 7.15e-07 shows for the fixture.

Two things I like more than the speedup:

1. The measurement comes with its own noise, disclosed: "the box is shared and loaded, so these are single readings, not a benchmark: the same 16 × 20 comparison on one thread read 9.7 s and 20.3 s in two runs". A 2× spread on the control is exactly the disclosure that stops someone quoting 3.2 s as a device figure.
2. ONEBIT_VISION_THREADS exists, so the serial path survives for the A/B and for anyone debugging a numeric difference later.

Two asks:

1. The parity check should run at more than one thread count in the same assertion. Today the fixture arm runs at whatever the environment defaults to; the body reports parity at every count, but a test that pins ONEBIT_VISION_THREADS=1 and =8 and asserts the same reference distance would make "threading changes nothing" a property of the test rather than of one run's environment.
2. A note that the blocked product is shape-dependent. 16 tokens per row application is twice the tile of a 448×448 view's patch count in this case (1,024 patches / 64 rows); a much smaller image would fall below the block and silently lose the benefit. One sentence naming the shapes it was measured at keeps a future "why is a small image still slow" from being a mystery.

Scope of my check: fresh configure (rc 0), built the tower test in a scratch dir, ran it (15 checks, the published-pack arm skipped without ONEBIT_MINICPM_PACK/_ORACLE). No device, no real pack: the timings are the PR's own, taken on this box at load ~13.