Skip to content

npu: the decoder takes the three position planes an interleaved-MRoPE pack needs (refs #1311, #799) - #1498

Merged
bong-water-water-bong merged 10 commits into
mainfrom
1bit/npu-mrope-planes
Nov 8, 2026
Merged

bong-water-water-bong merged 10 commits into
mainfrom
1bit/npu-mrope-planes

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

What this adds

The decoder now takes the three position planes an interleaved-MRoPE pack needs — the last engine-side piece of the
owner's vision-graph order (2026-11-08), and the one gate 2 cannot do without.

  • step_batch_embeds(embeds, position_ids, start_pos, logits): position_ids is 3 × rows, plane-major (t, h, w) —
    exactly the layout the reference's get_rope_index produces and AMD's text.onnx takes. Empty means the usual
    start_pos + row.
  • attention() rotates each rotary frequency by its own axis's position instead of by one scalar.
  • mrope_section is parsed (both spellings: rope_scaling for Hugging Face, rope_parameters for OGA/Qwen3.5),
    because the sections are what say which frequencies are (t, h, w). A pack that declares MRoPE without them is
    refused by name rather than guessed at — a wrong axis layout is a silently different model.
  • npu/forward/mrope_planes_probe.cpp (new, ONEBIT_NPU builds only) checks the claim.

Measured (mrope_planes_probe, Qwen3-VL-2B text container, 151,936 logits per arm)

arm host-GEMM device (m16 designs, one npu-hold ticket, 17 s)
equal planes (t = h = w, i.e. a text prompt) vs the token path byte-identical, 0 of 151,936 differ byte-identical, 0 of 151,936 differ
image-style planes (t = r, h = r/2, w = r%2) logits move, mean |Δlogit| 0.0045 logits move, mean |Δlogit| 0.3281

The first row is the identity this whole design rests on, and it is now checked in the code rather than only in a
numeric argument: with all three planes equal the rotation is exactly plain RoPE — proved separately as 0 angle
difference over positions 0…262143 at theta 5e6, and here as byte-identical logits. The second row is the evidence that
the planes are actually used (a wrong-but-plausible implementation that ignored them would fail that row).

Where this sits in the order

piece state
vision tower exported (fp32), per-patch cosine vs PyTorch ≥ 0.999 on 5 images done (#1479, gate 1 PASS 0.9999998)
our graph on CPU: 2.45 s/image vs the 17 s CPU projector done (#1479, 6.9×)
decoder takes precomputed embeddings merged (#1449)
decoder takes 3-plane MRoPE positions this PR
QDQ int8 for our ORT EP blocked: ORT's quantiser needs >30 GiB for the 1.6 GB graph and died at 13 GiB; a memory-gated retry is queued, Quark's ONNX A8W8 is the streaming fallback
gate 2 (end-to-end answer vs HF) still open: needs the deepstack sets injected at decoder layers 5/11/17, which this forward does not have. Named, not papered over.
gate 3's NPU arm after int8: our ORT EP claims float32 MatMul only, so the vision graph's non-MatMul nodes would fall through to the CPU EP — a mixed arm that must be labelled as one

refs #1311, refs #799 (P4's rope.planes).

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

#1498 ready for review at 1c1b34d0 — the decoder's 3-plane MRoPE input, with the identity checked in code: equal planes (t=h=w) are byte-identical to the token path, 0 of 151,936 logits differ, host-GEMM and device, and image-style planes move the logits (mean |Δlogit| 0.0045 host / 0.3281 device) so the planes are demonstrably used. mrope_section is parsed from both spellings and a pack that declares MRoPE without sections is refused by name. Gate 2 still needs deepstack injection at layers 5/11/17 — named on this PR and in the state file, not papered over.

pi-npufeat added 3 commits November 8, 2026 11:45
…graph is a 0.6 MB file plus a .data

A 1.6 GB inline protobuf is where ONNX tooling starts failing: the quantiser's pre-processing could not parse it
("Wire format was corrupt") and, once split, quantize_static was killed for memory at 8 GB free (exit 137). The split
is what AMD's own vision packs ship (vision.onnx + vision.onnx.data), and it keeps the graph file readable.

Recorded in the quantise stage rather than worked around: quant_pre_process() cannot be used here at all -- with the
weights inline it fails on symbolic shape inference, with them external it copies the model to a temp dir and leaves
the .data behind ("should be stored in /tmp/pre.quant.* but it is not regular file"). The graph is a fixed bucket with
concrete shapes, so the quantiser does not need it.

Gate 1 re-verified after the split: PASS, worst cosine 0.9999998 on 5 images.
…PU projector on CPU

bench subcommand: warm-up then N images through the session, best and median seconds per image, with the llama.cpp CPU
projector's 16.97 s printed beside it so the row cannot be read without its bar.

Measured on strixhalo, fp32 graph, CPUExecutionProvider only (no device in the loop):
  best 2.45 s/image, median 2.78 s  vs  the CPU projector's 16.97 s/image
i.e. our own graph is 6.9x faster than llama.cpp's CPU CLIP on the same box before any NPU involvement.

Caveat carried in the tool: the two stacks preprocess differently (our fixed bucket is 196 merged rows for a 448x448
image; llama.cpp's own smart-resize gave 214 image tokens in the phase-1 run), so the row compares an image through
each stack's own preprocessing, which is the shape the 's/image' question has.
… pack needs (refs #1311, #799)

step_batch_embeds() gains an optional position_ids of 3 x rows, plane-major (t, h, w) -- the layout the reference's
get_rope_index produces and AMD's text.onnx takes -- and attention() rotates each frequency by its own axis's
position instead of by one scalar. mrope_section is now parsed (both spellings: rope_scaling for Hugging Face,
rope_parameters for OGA/Qwen3.5), because the sections are what say WHICH frequencies are (t, h, w); a pack that
declares MRoPE without them is refused by name rather than guessed at.

Measured with npu/forward/mrope_planes_probe on Qwen3-VL-2B's text container (151,936 logits per arm):
  equal planes (t=h=w, a text prompt) vs the token path: BYTE-IDENTICAL, 0 of 151,936 differ -- host-GEMM and device
  image-style planes (t=r, h=r/2, w=r%2): the logits move (mean |dlogit| 0.0045 host / 0.3281 device)
which is the numeric identity proved earlier (0 angle difference over positions 0..262143 at theta 5e6) now holding in
the code, plus evidence the planes are actually used.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Rebased onto main b00845ad at dd7bacfa (the branch still carried two commits from the now-merged #1479, which is what made it CONFLICTING); mrope_planes_probe and 1bit rebuilt clean after the rebase.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at dd7bacfa91b23bf3d27102fe5aa8d40e41b412f1

The plumbing is careful:

  • plane-major positions gathered per row;
  • a by-name refusal when MRoPE is declared without mrope_section;
  • the 3 × rows size check, and the RAII reset of pos3_;
  • the old three-argument overload kept, so embeds_equiv_probe and mrope_planes_probe both build here.

But mrope_plane() implements the non-interleaved layout, and this PR targets an interleaved-MRoPE pack.

int mrope_plane(int freq) const {   // q4nx_forward.h:369
    const int a = sec[0], b = sec[0] + sec[1];
    return freq < a ? 0 : (freq < b ? 1 : 2);     // CONTIGUOUS: t…t h…h w…w
}

That's Qwen2-VL's MRoPE. Qwen3-VL's is interleaved: these packs declare "mrope_interleaved": true (e.g. ~/models/npu/d3-lite-conv/config.json:65), and the fork at main's pin implements it as GGML_ROPE_TYPE_IMROPE, "qwen3vl apply interleaved mrope" (ggml/src/ggml-cpu/ops.cpp:5902):

if      (sector % 3 == 1 && sector < 3 * sections[1]) theta = theta_h;
else if (sector % 3 == 2 && sector < 3 * sections[2]) theta = theta_w;
else                                                    theta = theta_t;   // (and e)

For Qwen3-VL's mrope_section [24, 20, 20] over 64 frequencies:

contiguous : tttttttttttttttttttttttthhhhhhhhhhhhhhhhhhhhwwwwwwwwwwwwwwwwwwww   (this PR)
interleaved: thwthwthwthwthwthwthwthwthwthwthwthwthwthwthwthwthwthwthwthwtttt   (Qwen3-VL)
-> 43 of 64 frequencies rotate by the wrong axis

Why the probe passes anyway:

  • with all three planes equal, every mapping gives the same angle, so the byte-identity row is true for any plane assignment;
  • the image-planes row shows only that the planes change the logits, not that they're applied to the right frequencies.

A wrong axis layout is, in your own words, "a silently different model", and gate 2 would be the first thing to see it.

Fix:

  • make mrope_plane() follow mrope_interleaved (i % 3 == 1 && i < 3·sec[1] → h; i % 3 == 2 && i < 3·sec[2] → w; else t), keeping the contiguous form only for a pack that doesn't declare interleaving, and parse that flag next to mrope_section;
  • add a probe row that compares against a reference at unequal planes, e.g. the rotation angles per frequency against a small numpy port of apply_interleaved_mrope for one (t, h, w) trio. That's the row that would catch this class of mistake.

Two smaller items:

  • The mrope_section parser can read past the array. After each strtol the loop does p2++. For a valid three-element array it stops at got == 3, but for a short one ([16, 24]) strtol leaves p2 on ], the ++ steps over it, and the scan picks up the next digit elsewhere in the JSON. A malformed section could pass as three. Stop at ] before incrementing.
  • The branch carries two commits the body doesn't describe: 35b7a89 (export weights as external data) and 6236d44 (the per-image bench). They're not on main, and the body credits the 6.9× bench to #1479, but that code is in this PR. List them, or split them out.

What I ran: built embeds_equiv_probe and mrope_planes_probe at this head (NPU build, thermal-run.sh): both clean. Read the forward and semantics diffs and the fork's IMROPE branch; the 43/64 count is computed from both mappings above. No device run.

…ers the pack names (refs #1311)

Qwen3-VL's vision tower emits three extra feature sets from blocks 5/11/17 and the reference *adds* each to the
decoder's hidden state at those layer indexes. Without that an image prompt cannot match the reference answer, no
matter how exact the tower's own rows are.

* vision_config.deepstack_visual_indexes is parsed (up to 8 entries) into the semantics;
* step_batch_embeds() takes them as a HybridInputs bundle -- the three MRoPE position planes and sets x rows x H of
  deepstack features -- instead of growing the argument list again;
* the batched body adds set s to the hidden state when the layer loop reaches deepstack_layers[s]; with no sets the
  pass is byte-identical to before;
* a set count that disagrees with the pack is refused by name, and so is a null buffer.

Measured with npu/forward/mrope_planes_probe on Qwen3-VL-2B's text container (151,936 logits per arm):
  pack declares 3 deepstack layers
  sets vs plain        : mean |dlogit| 0.0829 host / 0.2894 device (they must differ)
  wrong set count      : refused by name
  equal planes (control): byte-identical, 0 of 151,936 differ, host and device
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

The last engine piece for gate 2 is in, at 5a2738a0: deepstack injection.

Qwen3-VL's vision tower emits three extra feature sets (from blocks 5/11/17) and the reference adds each to the
decoder's hidden state at those layer indexes. Without that, an image prompt cannot match the reference answer however
exact the tower's own rows are — so this PR now carries both halves of the hybrid's decoder side: the 3-plane MRoPE
positions and the deepstack sets, as one HybridInputs bundle rather than a fourth positional argument.

  • vision_config.deepstack_visual_indexes parsed (≤8 entries) into the semantics; the pack in this lane declares 3.
  • Set s is added to the hidden state when the layer loop reaches deepstack_layers[s]; with no sets the pass is
    byte-identical to before (the control that keeps every existing route untouched).
  • A set count that disagrees with the pack, or a null buffer, is refused by name.

Rows from mrope_planes_probe (151,936 logits per arm, Qwen3-VL-2B text container):

arm host-GEMM device (m16, 31 s ticket)
equal planes, no sets (control) byte-identical, 0/151,936 byte-identical, 0/151,936
image-style planes mean |Δlogit| 0.0045 0.3281
deepstack sets 0.0829 0.2894
wrong set count — refused by name

With this, the decoder side of the owner's vision-graph order is complete: inputs_embeds (merged #1449), 3-plane
MRoPE, deepstack. What remains for gate 2 is the wiring itself — vision graph → rows → HybridInputs → answer vs
the HF reference — and for gate 3 the int8 graph (blocked on memory: ORT's quantiser wants >30 GiB for the 1.6 GB
graph; watcher queued, Quark A8W8 the streaming fallback).

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at 5a2738a0cdfa1d47bc3e51110c5ac71e2f465c93

5a2738a adds deepstack injection, with a refusal on a set-count mismatch and a probe row showing the sets move the logits. That's a reasonable shape. But the blocking item from my review at dd7bacfa is unchanged, and it now matters more:

  • mrope_plane() (q4nx_forward.h:382-386) is still the contiguous split (freq < sec[0] → t, < sec[0]+sec[1] → h, else w).
  • At this head, sem_.mrope is set only from mrope_interleaved (q4nx_semantics.c:278, :291). So every pack that takes the planes path is interleaved, and every one gets the contiguous mapping. That's 43 of 64 frequencies on the wrong axis at Qwen3-VL's [24, 20, 20], against the fork's IMROPE (sector % 3) at ggml/src/ggml-cpu/ops.cpp:5902.
  • The probe still can't see it: the new rows assert that equal planes are byte-identical (true for any mapping) and that planes and deepstack sets move the logits, not that they move them correctly.

Since this PR is now the gate-2 path (planes plus deepstack, end to end against HF), a wrong axis layout will show up there as a quality gap that's hard to attribute. It's much cheaper to fix the mapping first:

  • interleaved: i % 3 == 1 && i < 3·sec[1] → h, i % 3 == 2 && i < 3·sec[2] → w, else t;
  • one probe row comparing the per-frequency angle for a single unequal (t, h, w) trio against a ten-line numpy apply_interleaved_mrope.

Still open, smaller: the mrope_section parser's p2++ after strtol (q4nx_semantics.c:260-262) still steps over ] on a short array.

… in the first version), + the hybrid prompt probe

Two things, and the first is a defect the wiring found immediately:

* deepstack injection added set s to EVERY row of the prompt while the set has only rows_img rows -- so the loop read
  past the buffer (and, where it did not, added vision features to text positions). HybridInputs now carries the image
  row span and the injection runs there; a missing or out-of-range span is refused by name.
* npu/forward/hybrid_prompt_probe.cpp: the engine side of a vision-conditioned prompt -- reads the token ids, the
  image rows' embeddings, the 3-plane positions and the deepstack sets (raw files + meta.txt from the vision graph and
  the tokenizer), runs the prefill through step_batch_embeds() and greedily decodes N tokens.

Rows: the wiring runs (219 tokens, 196 image rows, 3 deepstack sets, host-GEMM and the layout checks pass), and the
answer is NOT yet the reference's -- recorded on the PR rather than claimed: the first two attempts emitted <|im_end|>
immediately, one of them because the hand-built prompt lacked the chat template's 'assistant\n' turn start. Gate 2's
remaining work is the preprocessing/layout debugging loop, not more plumbing.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

The wiring runs end to end at af3240bb — and it found a real defect on the first run.

npu/forward/hybrid_prompt_probe.cpp is the engine side of a vision-conditioned prompt: it reads the token ids, the
image rows' embeddings (from our ONNX vision graph), the 3-plane positions and the deepstack sets, runs the prefill
through step_batch_embeds() and greedily decodes.

Defect, mine, caught by the wiring rather than by a row: the deepstack injection added set s to every row of
the prompt while a set has only rows_img rows — an out-of-bounds read, and where it did not fault it added vision
features to text positions. HybridInputs now carries the image row span (image_lo/image_hi), the injection runs
there only, and a missing or out-of-range span is refused by name. That is exactly what the control rows could not
catch (they used matching shapes) and what a first real prompt did.

What runs now (host-GEMM, box at load 40): a 219-token prompt with 196 image rows, 3 deepstack sets, layout
asserts passing — 219 tokens, image span [18, 214), h/w planes walking 18..31, pooled 196×2048 from our graph, three
deepstack sets 196×2048.

What does not yet: the decoded answer is not the reference's. First attempt emitted <|im_end|> at once because the
hand-built prompt lacked the chat template's assistant\n turn start; the second emits 32 198 <|im_end|>…, i.e. it
still wants to stop. So the remaining gate-2 work is a preprocessing/layout debugging loop — most likely candidates,
in the order I would test them: our pixel preprocessing (my own resize+normalise vs Qwen3-VL's smart-resize), the exact
image-token span width for this image, and whether the deepstack sets belong at the image rows before or after the
layer that produced them. Recorded here rather than dressed as progress.

Gate 3 unchanged (int8 blocked on memory; our graph on CPU is 2.45 s/image vs the 17 s bar).

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at af3240bb212dc00776c1237d006fdc096dceefc6 — the guard and the plane mapping describe different layouts, and the probe cannot tell
you which is right.

First, what is good, because it is most of the PR. The probe's three properties are the right three, and (a) is exactly the identity I would have
demanded:

(a) three equal planes byte-identical to the token path   — with t = h = w, sum(pos * inv) is plain RoPE, so this must hold exactly
(b) three different planes run and move the logits        — the positive case plain RoPE cannot express
(c) MRoPE declared without mrope_section → refused by name — the guess is refused rather than made

and the plumbing around it is careful: step_batch_embeds refuses position ids when the pack does not declare the variant, requires
n_mrope_section >= 3, gathers the plane-major pos3_ into the per-token trio, and the deepstack indexes are read from vision_config as well.

The finding: the guard accepts only packs that declare the interleaved variant —

err_ = "step_batch_embeds: position_ids given but this pack does not declare interleaved MRoPE";

— and the mapping then treats the three sections as one contiguous partition of the frequency axis:

// Which plane a rotary frequency belongs to under interleaved MRoPE (semantics' mrope_section, contiguous).
int mrope_plane(int freq) const {
    if (sem_.n_mrope_section < 3) return 0;
    const int a = sem_.mrope_section[0], b = sem_.mrope_section[0] + sem_.mrope_section[1];
    return freq < a ? 0 : (freq < b ? 1 : 2);
}

"Interleaved" and "contiguous" are different layouts, and for the declared target they differ for most frequencies. In the Qwen2.5-VL/Qwen3-VL
implementations, interleaved means the axes alternate within the sections — the h plane takes frequencies 1, 4, 7, … < 3*mrope_section[1], the w
plane 2, 5, 8, … < 3*mrope_section[2], and t takes the rest — so mrope_plane(i) is a stride-3 function, not i < s0 ? 0 : i < s0+s1 ? 1 : 2. If
the pack you ran carries the Qwen2-VL chunked convention instead and the sections cover the rotary axis once, the mapping is right and it is the
guard, the error string and the comment that are wrong. Either way those two cannot both be right, and I cannot tell from the code which convention
the pack declares — the probe takes <model.q4nx>.

And this is the one defect the three probe properties cannot find, which is why I am holding it: with three equal planes both layouts reduce to
pos * inv, so (a) passes either way; (b) shows the logits move without showing they move per the right axes; (c) is about the sections being present,
not about how they are used. A wrong assignment is silent at image prompts — the exact class this PR exists to prevent.

What would settle it (cheap, device-free): assert the assignment itself, not just its endpoints — for a head_dim-sized rotation,
mrope_plane(i) == i % 3 inside 3*section[1] / 3*section[2] for the interleaved convention, or the contiguous partition if that is the pack's
convention — and better still compare cos_[i] against an HF/vLLM reference for one image position triple. One assertion in the probe's (b) arm turns a
layout question into a measurement.

Also worth fixing while here: npu/forward/include/q4nx_semantics.h:95-96 still says "Interleaved/multimodal RoPE variant that this forward does
not implement"
, which this PR makes false.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at af3240bb212dc00776c1237d006fdc096dceefc6

One blocking defect: this head does not compile as C.

npu/forward/src/q4nx_semantics.c:235
        const char* at = vc ? strstr(vc, "deepstack_visual_indexes") : nullptr;
                                                                      ^~~~~~~ 'nullptr' undeclared (first use in this function)

q4nx_semantics.c is a .c file and CMake compiles it with the C compiler in every target that uses it (q4nx_mrope_test, npu_rmsnorm_offset_test, q4nx_layer_kinds_test, and onebit_npu_forward). A/B with the same invocation, gcc -std=c11 -c:

merge-base  npu/forward/src/q4nx_semantics.c   errors=0
this head   npu/forward/src/q4nx_semantics.c   errors=1   <- the nullptr

Fix is one token: NULL. (No other C++-ism in the added C lines — I grepped for nullptr|static_cast|namespace|std::|auto |true|false and this is the only hit.)

Why CI is green, which is worth fixing too: nothing in CI compiles that file. The three targets that do are absent from the cmake --build build --target … list in ci.yml, and onebit_npu_forward needs the private runtime so CI never builds it. So the NPU lane's own compile is the only thing that would have caught this, and the "green" was never evidence about this file. The cheap fix that would have caught it: the existing g++-16 -std=c++26 -fsyntax-only … step next to tools/mtp_acceptance.cpp has a natural sibling in gcc-16 -std=c11 -fsyntax-only -Inpu/forward/include npu/forward/src/q4nx_semantics.c.

Everything else in this PR is right, and I verified the parts that can be verified off-device.

Three planes, and the identity is by construction. half = rope_dim_/2 frequencies, each rotating by pos3[mrope_plane(i)]. With t = h = w = pos every branch returns pos, so the angle is exactly pos * inv — plain RoPE, bit for bit, with no special case. I checked the mapping against the model's real numbers: head_dim 128 → 64 pairs, and the pack's rope_scaling.mrope_section = [24, 20, 20] sums to 64, so the split is 0–23 t, 24–43 h, 44–63 w and the else → plane 2 fallback is unreachable. That also matches HF's convention (which splits the full 128 dim with doubled sections, [48, 40, 40] — the same boundaries in pair space).

The semantics read succeeds on the actual pack. /home/bcloud/models/qwen3vl/q4nx-text/config.json is the flattened form the reader expects, and it carries exactly what these lines look for:

top-level  rope_scaling = {"mrope_interleaved": true, "mrope_section": [24, 20, 20], "rope_type": "default"}
nested     vision_config.deepstack_visual_indexes = [5, 11, 17]

so value_for_top(json, "rope_scaling") (depth 1) finds the sections, and value_for_top(json, "vision_config") + strstr finds the layer list. Note the two lookups are at different depths for a reason, and a raw HF config.json — where rope_scaling sits under text_config — would not parse; that is fine because this reader's input is the pack, not the HF config, but it is worth one line in the comment so the next reader does not "fix" it into a nested lookup.

Deepstack is injected in the right place and only there. deepstack_lo_/hi_ clamp into [0, M) and the set is added at add[(r - lo) * H + k] for image rows only — the offset is correct, and the comment records the failure of the version that spread it over every row ("produced garbage tokens") rather than hiding it.

The refusals are all by name, which is what this lane keeps needing: position_ids without MRoPE, MRoPE without sections, wrong plane count, deepstack_sets != n_deepstack, sets without a buffer, and a span outside 0..rows. Each names the mismatch and the expected value.

The RAII guard covers all six new members (pos3_, pos3_rows_, deepstack_, deepstack_rows_, deepstack_lo_, deepstack_hi_), so a later plain step_batch() call cannot inherit a stale plane pointer from an earlier hybrid one.

The export change is real and its artifact is on disk. vision_fp32.onnx is 0.6 MB and vision_fp32.onnx.data is 1627.8 MB — precisely the split this PR makes (external_data=True, plus the hand-split for the legacy exporter, for the reason given: a 1.6 GB inline protobuf is where the quantiser's pre-processing fails to parse). The new bench subcommand states its own limits ("Not an accuracy row — check is"), which is the right separation.

One thing to add while you are in here: hybrid_prompt_probe and mrope_planes_probe are add_executable only — no add_test, so nothing runs them, and they are not in CI's target list either. They need a model and a kernels dir, so the sensible shape is add_test guarded on a model path with a SKIP_RETURN_CODE (the repo already uses 77 for that elsewhere), or a -fsyntax-only line in the CI step above. A probe nobody runs is a probe that rots.

Not run: the probes themselves (they need onebit_npu_forward + the private runtime, and a device for the non-host arm). Everything above that I call verified is either a compile, a config read, or an on-disk artifact.

…ses a bad image answer)

image_lo == image_hi is now legal and means 'no image rows': no deepstack sets, no vision rows, the same prompt and the
decode loop. That control is what separated the two halves of a wrong answer this session: the text-only prompt through
the same probe decodes coherently while the image prompt stops at once, so the plumbing (prompt, positions, decode) is
sound and the gap is in the image path.

And the image path's graph is not the suspect either: on the real test image, our ONNX vision graph against the HF
tower's own output on the SAME pixel_values gives pooled cosine 1.00000000 and deepstack 1.00000012/1.00000012/0.99999988
-- the export is exact for this image too, so what remains is the placement: the image rows' positions or their ordering.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Localised the bad answer with two controls at 010c2d04 — and it is neither the prompt nor the graph.

  1. Text-only through the same probe is coherent. Same prompt, same tokenizer, same positions machinery, no image
    span (image_lo == image_hi is now legal = "no image rows"): 32 1697 594 3579 374 25244 95701 553 264 72400 11 700
    — a real sentence. So the prompt construction, the MRoPE position rule (all text planes equal), the prefill and the
    greedy decode loop are all sound, and the failure lives in the image path.

  2. The vision graph is exact on the real image too. Our ONNX graph vs the HF tower's own output on the same
    pixel_values (no processor in the loop — HF's tower called directly):

    output cosine max|Δ|
    pooled 1.00000000 4.3e-3
    deepstack_0 / _1 / _2 1.00000012 / 1.00000012 / 0.99999988 4.3e-6 / 5.2e-5 / 2.6e-4

    So neither the export nor my pixel preprocessing is wrong for the image the graph consumes — the rows going into the
    decoder are the reference's rows.

What that leaves is the placement, and it is now a short list: the image span's exact width for this image
(<|image_pad|> count), whether the pooled rows go into <|image_pad|> positions in row-major (h, w) order, and
whether the deepstack sets belong at the image rows of the layer's input or after that layer runs. The next step is the
HF text model fed the same inputs_embeds + position_ids (no processor needed) — if HF answers coherently with our
rows, the difference is in our decoder's image path; if HF also stops, the rows' placement is wrong. That also gives gate
2 its reference answer.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at 010c2d04081af7fb789d337137075d42d6751ad6

The two new controls are good work, and they narrow the bad image answer to two defects already in the diff. Both are checkable against the reference file (transformers/models/qwen3_vl/modeling_qwen3_vl.py). Neither one shows up in a text-only run, which is why the text-only control comes out coherent.

1. The deepstack sets go in at the wrong layers, and before the layer instead of after it (new, q4nx_forward.cpp in the step_batch layer loop).

  • sem_.deepstack_layers is parsed from vision_config.deepstack_visual_indexes, which is [8, 16, 24]. Those are vision-tower layers: Qwen3VLVisionModel taps its own blocks 8/16/24 to produce the three sets (modeling_qwen3_vl.py:725).
  • The text model consumes them like this (:828-841):
    for layer_idx, decoder_layer in enumerate(self.layers):
        hidden_states = decoder_layer(...)
        if deepstack_visual_embeds is not None and layer_idx in range(len(deepstack_visual_embeds)):
            hidden_states = self._deepstack_process(hidden_states, visual_pos_masks, deepstack_visual_embeds[layer_idx])
    
    So set i is added after text decoder layer i, for i = 0, 1, 2.
  • This PR adds set s before text layer deepstack_visual_indexes[s] (8, 16, 24). The fix is to add set i to the image rows of layer i's output (i < n_deepstack), and not to read the decoder-layer index from the vision config.

The image-span restriction in af3240b is right: it matches visual_pos_masks, and it fixed a real OOB read.

2. mrope_plane() is still the contiguous (Qwen2-VL) layout (q4nx_forward.h:388-392, unchanged since dd7bacfa). The reference's apply_interleaved_mrope (:372-386) starts from T everywhere, then overwrites:

H: freqs[1::3] for indices < 3*sec[1]
W: freqs[2::3] for indices < 3*sec[2]

In mrope_plane terms:

plane(i) = (i%3==1 && i<3*sec[1]) ? 1 : (i%3==2 && i<3*sec[2]) ? 2 : 0

The comment above it already says "interleaved". At [24,20,20], 43 of the 64 frequencies currently get the wrong axis. Text-only can't show this, because all three planes are equal there. Image rows can, because their h/w differ.

3. The mrope_section parser still overruns on short arrays (q4nx_semantics.c:260-262).

  • After strtol sets p2 onto the terminator, the loop's p2++ steps past it. For [24, 20], that step skips the ], and the scan keeps reading digits from later JSON into mrope_section[2]. The same pattern sits in the deepstack_visual_indexes loop at :240-241.
  • Fix: continue without the increment after a successful strtol (or use p2 - 1), plus a probe case with a 2-entry array.

Your planned next step will confirm this. Feed the HF text model the same inputs_embeds and position_ids. I expect HF to answer coherently with your rows, which would put the difference in (1) and (2), not in placement. With both fixed, the same run is a direct gate.

The two vision-export commits from earlier are still undescribed in the body.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at 010c2d04081af7fb789d337137075d42d6751ad6

Re-verdict at the new head. The blocking defect is unchanged:

$ gcc -std=c11 -c -I npu/forward/include npu/forward/src/q4nx_semantics.c
npu/forward/src/q4nx_semantics.c:235:72: error: 'nullptr' undeclared (first use in this function)
        const char* at = vc ? strstr(vc, "deepstack_visual_indexes") : nullptr;
                                                                      ^~~~~~~
errors=1        (merge-base: errors=0 — same command, same headers)

q4nx_semantics.c is a .c file compiled by the C compiler in q4nx_mrope_test, npu_rmsnorm_offset_test, q4nx_layer_kinds_test and onebit_npu_forward. One token: NULL.

The new delta is good and worth keeping. Allowing image_lo == image_hi as the text-only control, and then handing over no deepstack span at all for it, is the correct pair of changes — the decoder refuses image_hi <= image_lo when sets are present ("deepstack sets need the image row span"), so the probe would otherwise have been unable to run its own control arm. The comment says which case it is ("image_lo == image_hi is the text-only control: no image rows, nothing to inject"), which is what makes the relaxed check reviewable.

So: fix the nullptr, and everything I checked on the previous head still stands — the plane mapping is exhaustive for this model (64 pairs, mrope_section [24,20,20]), equal planes reproduce pos * inv exactly, deepstack is added only inside the image span, the pack's flattened config.json parses (top-level rope_scaling, nested vision_config), and the export artifact on disk is 0.6 MB graph + 1627.8 MB .data.

Still open from my last review, and still not blocking: the two probes are add_executable with no add_test and are absent from CI's target list, so nothing runs them; and CI has no step that compiles npu/forward/src/*.c as C, which is why this defect reached a green build at all. A one-line gcc-16 -std=c11 -fsyntax-only -Inpu/forward/include npu/forward/src/q4nx_semantics.c beside the existing mtp_acceptance.cpp syntax check closes that hole permanently.

…ak, not 64)

ORT's quantize_static needs ~64 GiB peak for this 1.6 GB graph -- measured, not estimated: a systemd scope recorded
"64.1G memory peak" before the OOM killer took it, twice, on a box where other lanes hold most of the 122 GiB. This adds
tools/vision_qdq.py and "quantize --stream", which does the same A8W8 job by editing the graph: per-output-channel int8
weights and per-tensor int8 activations as QDQ pairs, with activation ranges calibrated by running the fp32 graph once
with every Gemm input exposed as an output (no intermediate-probing library needed).

Measured, one weight tensor in memory at a time, under a scope capped at MemoryMax=16G:

  peak RSS 6.59 GB (vs the 64.1 G ORT needed)   weights 1,628 MB -> 421 MB (3.9x)   104 weights, 104 activations

Two bugs found on the way, both worth the comment they now carry:

  * QuantizeLinear's output type comes from its zero point and defaults to UINT8, which silently clamps every negative
    activation to 0: 103 of 104 activation tensors diverged from the fp32 graph (cosine -0.03). An int8 zero point fixes
    the type, and the same graph then lands at 0.61-0.91.
  * save_model with external data APPENDS to an existing .data file, so a second run wrote an 841 MB blob for 421 MB of
    weights. The stale pair is now unlinked first.

Accuracy, honestly: weights-only (W8A32) gives cosine 0.9937 against the fp32 graph, and full A8W8 gives 0.61-0.91
against the HF tower -- below this repo's 0.999 per-patch bar. Per-K activation scales would close some of that gap but
cannot fold into our EP's int32 accumulation (sA would vary along K), so a static per-tensor activation scale is the only
form the device path can use. The choice between W8A32 at 0.9937 and an A8W8 arm judged by its answers is an owner call,
not one to make silently here.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Streaming QDQ is in (9ce309f8) — and the int8 arm needs an owner decision, because the numbers say per-tensor A8W8 cannot meet this repo's bar.

tools/vision_qdq.py + quantize --stream: per-output-channel int8 weights, per-tensor int8 activations, QDQ pairs written with the onnx helpers, activation ranges calibrated by running the fp32 graph once with every Gemm input exposed as an output. One weight tensor in memory at a time.

measured
peak RSS (scope capped MemoryMax=16G, oom_score_adj=800) 6.59 GB (ORT needed 64.1 G)
weights on disk 1,628 MB → 421 MB (3.9×)
weights quantised 104 Gemm, 104 activation tensors
W8A32 vs fp32 graph cosine 0.9937
A8W8 vs HF tower, 5 images cosine 0.61–0.91, gate 1 FAIL

Two bugs on the way, both now commented in the code:

  • QuantizeLinear's output type comes from its zero point and defaults to UINT8 — which silently clamps every negative activation to 0. That was the whole "cosine -0.03 / 103 of 104 tensors diverge" mystery: an int8 zero point fixes it and the same graph jumps to 0.61–0.91. A one-line isolation graph reproduced it in nine lines, which is why the big graph was never the suspect again.
  • save_model with external data appends to an existing .data, so a re-run wrote 841 MB of blob for 421 MB of weights.

OWNER: the accuracy fork. Per-K activation scales would recover much of the 0.61–0.91 gap, but they cannot fold into our EP's int32 accumulation (sA would vary along K), so a static per-tensor activation scale is the only form the device path can use. That leaves two honest arms: W8A32 at cosine 0.9937 (weights 3.9× smaller, activations float — the EP's float32 MatMul claim can still take it) or A8W8 judged by gate 2's answers instead of the 0.999 cosine bar. I will not quietly relax a gate the repo set, so this one is yours.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at 9ce309f8d24a0162f2575b9a500d4840dc5ce4aa

This is a review at the current head. 010c2d04 already has a CHANGES verdict; since then only 9ce309f has landed, and it touches only tools/.

The decoder side: all three items from 010c2d04 are unchanged at this head

Green CI here is a compile and link check. It doesn't exercise the Qwen3-VL image path, and that path is still wrong in three places. They're checkable against transformers/models/qwen3_vl/modeling_qwen3_vl.py:

  1. The DeepStack layer and timing (q4nx_forward.cpp:3243-3248). Set s is added before text layer deepstack_visual_indexes[s], which is 8/16/24, and those are the vision-tower layers the sets are tapped from (:725). The reference adds set i after text decoder layer i, for i < 3 (:828-841).
  2. mrope_plane() is contiguous (q4nx_forward.h:388-392), but the packs declare mrope_interleaved. The reference's apply_interleaved_mrope (:372-386) gives:
    plane(i) = (i%3==1 && i<3*sec[1]) ? 1 : (i%3==2 && i<3*sec[2]) ? 2 : 0
    
    At [24,20,20], 43 of 64 frequencies get the wrong axis.
  3. The parser overrun. The loop's p2++ / p3++ after strtol steps past ] on a short array (q4nx_semantics.c, both the mrope_section and deepstack_visual_indexes loops).

Your own controls match this exactly: text-only is coherent (all three planes are equal and there are no DeepStack sets, so neither bug can show), and the image prompt is bad. Fixing (1) and (2) is the next step for the bad image answer, ahead of any placement search.

The new commit (9ce309f, streaming QDQ)

The 6.59 GB peak versus 64.1 G is a real improvement, and so is the int8-zero-point finding. Three things need fixing in the tool:

  1. vision_qdq.py:41 puts sys.path.insert(0, "/tmp/qvl") in the import path, and :82 imports hybrid_input from it.
    • hybrid_input.py is not in the tree (git ls-tree at this head). It exists only in your scratch /tmp/qvl, so quantize --stream can't calibrate from a clean checkout.
    • /tmp is shared by every lane on the box, so whatever sits in /tmp/qvl gets imported first.
    • Add the module to tools/ (or inline the generator, as the comment above it says), and drop the path insert.
  2. main() now throws away quantize's exit code.
    • It became quantize(...); return 0. The ORT path "gates itself" and returns 1 when the quantised graph fails the cosine bar (export_qwen3vl_vision.py:330-336), and that 1 is now lost.
    • So a graph the tool itself calls "QUANTISED GRAPH FAILS … do not use it" exits 0. The comment there names that as exactly the failure mode the gate exists to prevent.
    • Return the ORT path's rc, and give --stream the same self-check before it returns. Its A8W8 output measured 0.61–0.91 against the 0.999 bar, so it has to be able to fail.
  3. Small:
    • vision_qdq.py carries the copyright/SPDX header twice (:2-3 and :15-16);
    • its docstring's usage line says qdq_stream.py;
    • the module docstring says MatMul inputs are calibrated, while matmul_weights explains that the attention MatMuls are deliberately left float. "Every Gemm input" (as your comment says) is the accurate wording.

The W8A32-vs-A8W8 accuracy question is the owner's to decide, as you raised it. It doesn't affect any of the above.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Greedy decode through the probe, one variable at a time: the image rows reach the decoder, and it is the image position walk that kills the answer.

Same prompt, same vision features, same prefill; only positions.i32 (or the features) change:

run generated ids (8) reading
image walk (t=18 on all 196 rows, h/w walk 18..31, next text 32) 32 198 151645 151645 … garbage — "I\n<
zeroed image features, walk kept 1986 13 151645 151645 11 10950 13 13 different garbage → the features do reach the decoder; the wiring is not the bug
plain sequential positions (pos = arange(len(ids))) 32 7377 3749 448 264 6176 4247 11 coherent words
walk variants: t constant-h/w constant / all-zero start / t carries the walk 32 198 151645… / 32 151645… / 32 198 151645… all garbage

So: (1) the vision features are live and it is positional, not wiring; (2) any mapping in which the 196 image rows repeat a position, or in which the sequence skips 14 positions, ends the answer immediately, while plain sequential positions on the same rows decode into words.

That points at our decoder's MRoPE handling of a repeated/skipped position rather than at the vision path — and that is testable without HF and without the vision graph: feed the text-only prompt positions containing a constant block, and see whether the text-only answer (which is known-good) also collapses. If it does, the bug is in the position machinery, and a 24-layer prefill of one 219-token prompt is the whole cost.

(Ordering note: gate 2's HF reference answer still needs ~9 GiB resident; the box has been at 6-7 GiB free all afternoon, so that run is queued behind memory, not behind code.)

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Gate 2 has its reference answer now — and it vindicates the vision graph: the fault is ours, in the decoder's MRoPE path.

I ran HF's own model on exactly our inputs (ids, 3-plane position_ids, and our ONNX pooled features) in bf16 — no processor, no re-export, 3.8 GiB peak:

first 8 greedy ids text
HF, our image-walk positions 32 16771 11297 2766 1809 4171 9027 264 "A retro-style video game screen showing a"
HF, sequential positions 32 12955 657 11 16771 11297 2766 1809 a caption too
ours, image walk 32 198 151645 151645 … "A\n<|im_end|>"
ours, sequential positions 32 7377 3749 448 264 6176 4247 11 "A digital interface with a green header,"

So with the same features and the same positions, HF captions the image and we stop at "A\n". The reference answer for this image/prompt pair is a caption beginning "A retro-style video game screen showing a", and the vision graph, the preprocessing, the prompt and the pooled features are all inside the reference's own path — none of them is the fault. Two consequences:

  1. My earlier "the walk is the bug" reading was too strong: HF is coherent with either position scheme, so positions change the wording, not the viability. Our decoder is what fails, and it fails specifically on the 3-plane case.
  2. The first token already agrees (32 = "A"), so the prefill is close; divergence starts at the first decode step (HF 16771, us 198). The next step is therefore a logit-level diff, not more black-box prompting: print our top-5 with values at the last prefill row and at decode step 1, and compare against HF's. That localises whether the mrope angles are wrong (prefill) or the decode step's 3-plane handling is wrong.

Worth recording for the next reader: transformers wants mrope position_ids as [3, batch, seq], not [batch, 3, seq]. The wrong order does not raise — it silently reshapes the hidden state to 3× width and dies later inside o_proj with 219x6144 and 2048x2048. That cost me a chunk of this session.

…exit code

Five of the six review items from the two CHANGES verdicts on engine #1498. (The sixth, a CI step that
syntax-checks every npu/forward/src/*.c with gcc-16 -std=c11, is a workflow file and this app's token has no
`workflow` scope, so it rides as a patch for the owner instead. The item was non-blocking.)

Decoder:

1. `nullptr` in `q4nx_semantics.c` -- a C file that the CMake build happens to compile as C++ inside
   q4nx_mrope_test, npu_rmsnorm_offset_test, q4nx_layer_kinds_test and onebit_npu_forward, so a C++-only token reached a
   green build. `NULL`, plus the two parser overruns in the same file: both loops advanced the cursor with the
   `for` clause after `strtol` had already moved it, which stepped past the closing `]` and kept scanning until some
   later `]` appeared. The cursor is now moved by strtol alone.

2. `mrope_plane()` was a contiguous split of the three sections. The reference does not split contiguously: it builds
   cos/sin for all three planes and recomposes, overwriting every third frequency from the h and w planes over a stride-3
   window of length section[dim] * 3 (transformers
   `Qwen3VLTextRotaryEmbedding.recomposition_frequencies`). So

       axis(i) = 1 if i % 3 == 1 && i < 3 * section[1]
               = 2 if i % 3 == 2 && i < 3 * section[2]
               = 0 otherwise

   At [24, 20, 20] that is 24 t / 20 h / 20 w, and it disagrees with the contiguous reading on **43 of the 64**
   frequencies (measured, not estimated; the reviewer's number was 43 too). The rule now lives in q4nx_semantics.h as
   `q4nx_mrope_axis()` -- a free C function -- and `mrope_plane()` is one line over it, so tests/q4nx_mrope_test.c pins
   the counts, both window edges and the 43/64 delta without building a forward pass.

3. DeepStack timing. Set s was added before text layer deepstack_visual_indexes[s] (8/16/24), but those indexes are the
   VISION tower's layers the sets are tapped from. The reference adds set `layer_idx` AFTER text decoder layer
   `layer_idx` runs, for layer_idx < number of sets (transformers Qwen3VLTextModel.forward: "add visual features to the
   hidden states of first several layers", `_deepstack_process` inside the loop after the call). Now `l < n_deepstack`
   after the layer.

   While moving it I deleted `cur_layer_ = l`, which silently made every layer read the last layer's id, and the
   text-only control's answer changed from "A person's face is partially obscured..." to garbage. The control caught it
   (the second time this session a known-good control caught a regression before a push), `cur_layer_` is back, and the
   control is byte-identical again: `32 1697 594 3579 374 25244 95701 553`.

Tool (`quantize --stream`):

4. `tools/vision_qdq.py` imported its image generator from `/tmp/qvl` -- not in the tree, and first on sys.path for every
   lane on the box. The generator now lives in the module, the path insert is gone, and nothing outside the repo is
   imported.

5. `main()` discarded the ORT path's exit code, so a graph the tool itself calls "FAILS ... do not use it" exited 0.
   The rc is propagated, and `--stream` gets the same contract: it self-checks against the fp32 graph and returns 1
   below the bar. Verified on this head: `self-check: worst cosine 0.606102 ... FAIL; the bar is 0.999`, rc=1.
   Also fixed: the duplicate copyright header, the stale `qdq_stream.py` usage line, and the "every MatMul input"
   wording (it is every `Gemm` input; attention's activation-times-activation MatMuls are deliberately float).

Tests: q4nx_mrope_test, npu_rmsnorm_offset_test and q4nx_layer_kinds_test all pass, and the text-only control is
unchanged. The image prompt is **still** bad (`32 23 <|im_end|> ...`) -- HF captions the same ids, positions and pooled
features, so items 2 and 3 were real bugs against the reference but are not sufficient, and the next step is a logit
diff at the first decode step rather than another placement search.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Re-verdict please at 09072dea. All six items are addressed; the workflow one needs your token.

Decoder (1–3)

  1. nullptr → NULL, plus the two parser overruns you found in the same file: both loops did strtol(p, &p, 10) and then let the for clause p++, so the cursor stepped past the closing ] and kept scanning until some later ] appeared. The cursor now moves only inside strtol. gcc -std=c11 -fsyntax-only -Inpu/forward/include npu/forward/src/q4nx_semantics.c is clean.
  2. Interleaved axes, exactly your formula, taken from recomposition_frequencies rather than from memory: axis(i) = 1 if i%3==1 && i<3*sec[1]; 2 if i%3==2 && i<3*sec[2]; else 0. I extracted it to q4nx_mrope_axis() in q4nx_semantics.h so it is testable without a forward pass, and mrope_plane() is now one line over it. You said 43 of 64; measured is 43 of 64 (24 t / 20 h / 20 w), and tests/q4nx_mrope_test.c now pins the counts, both window edges (i=60/61/62) and that 43/64 delta.
  3. DeepStack after layer l, for l < n_sets — _deepstack_process is called after decoder_layer(...) in the loop, and deepstack_visual_indexes are the tower's layers, not text indexes.

Confession on (3): while moving the block I deleted cur_layer_ = l, so every layer read the last layer's id and the text-only control went from "A person's face is partially obscured…" to garbage. The control caught it before the push (second time today), cur_layer_ is restored, and the control is byte-identical again: 32 1697 594 3579 374 25244 95701 553. All three C tests pass: q4nx_mrope_test ok, npu_rmsnorm_offset ok, q4nx_layer_kinds passed (0 failures).

Be clear about what (2) and (3) did not do: the image prompt is still bad, now 32 23 <|im_end|> (it was 32 198). HF captions the same ids, positions and pooled features — "A retro-style video game screen showing a" — with the first token agreeing with ours (32). So both were real bugs against the reference and neither is sufficient; divergence starts at the first decode step, and the next step is a logit diff there rather than another placement search.

Tool (4–6)

  1. The image generator now lives in tools/vision_qdq.py; sys.path.insert(0, "/tmp/qvl") and the hybrid_input import are gone. Nothing outside the tree is imported.
  2. The rc is propagated, and --stream self-checks against the fp32 graph and returns 1 below the bar. On this head: self-check: worst cosine 0.606102 against the fp32 graph on 3 images (FAIL; the bar is 0.999), rc=1.
  3. Duplicate header removed, usage line fixed to vision_qdq.py, wording corrected to "every Gemm input" (attention's activation×activation MatMuls are deliberately float, which matmul_weights explains).

The workflow item is yours to land. This app's token has no workflow scope, so git push is refused for any commit touching .github/workflows/. The exact 21-line patch is below — it runs gcc-16 -std=c11 -fsyntax-only -Inpu/forward/include over every npu/forward/src/*.c right before the build step:

@@ -74,6 +74,16 @@
         run: python3 tools/pin_watch.py --self-test
       - name: Configure
         run: cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=gcc-16 -DCMAKE_CXX_COMPILER=g++-16
+      - name: Every C source still compiles as C
+        # q4nx_semantics.c is a .c file compiled by CC in q4nx_mrope_test, npu_rmsnorm_offset_test,
+        # q4nx_layer_kinds_test and onebit_npu_forward, but the CMake build here compiles it as C++ inside those
+        # targets, where a `nullptr` in it is legal. That is how a C++-only token reached a green build and then broke
+        # four C tests (engine #1498). One syntax-only pass with the C compiler closes the hole.
+        run: |
+          for src in npu/forward/src/*.c; do
+            echo "gcc-16 -std=c11 -fsyntax-only $src"
+            gcc-16 -std=c11 -fsyntax-only -Inpu/forward/include "$src"
+          done
       - name: The draft-loop measurement tool still compiles

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown

Preparing review...

…on a token are not the same decoder

Gate 2's remaining question is where the image prompt goes wrong, and tokens cannot answer it: two decoders can pick the
same argmax and be far apart underneath, or diverge on a coin flip. --top-k N prints the top-N logits wherever an argmax
is taken (prefill, and the first decode step by name), so a step can be diffed against the reference number for number.

It did its job on the first run. Against HF on the same ids, positions and pooled features:

  prefill   ours: 32:35.67 1986:33.47 785:29.32 2082:28.01   HF: 32:33.00 2082:31.25 1986:30.25 785:28.50
  step 1    ours: 23:21.61 16:21.36 11:20.76 15:20.53         HF: 16771:27.13 12955:26.00 23790:24.50 36190:24.00

  text-only, step 1   ours: 1697:22.63 3265:22.07 ...        HF: 3265:23.75 1697:23.38 ...

So the prefill agrees with the reference (same four tokens, same order bar one swap, values within 2.7) and the text-only
decode agrees too (same two candidates, swapped, within 1.7) -- while the image prompt's first decode step is flat at
~21 where the reference is peaked at 27. That says the vision path, the position planes, the deepstack sets and the
prefill KV are right, and the defect is in the single-token step once the cache holds image rows.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Logit diff, as promised — and it says the prefill is right while the first decode step is not.

hybrid_prompt_probe --top-k N now prints the top-N logits wherever an argmax is taken (new at 1e1df277). Same ids, positions and pooled features as HF:

ours HF
prefill 32:35.67 1986:33.47 785:29.32 2082:28.01 43288:27.77 32:33.00 2082:31.25 1986:30.25 785:28.50 11613:27.00
step 1 (image prompt) 23:21.61 16:21.36 11:20.76 15:20.53 17:20.24 16771:27.13 12955:26.00 23790:24.50 36190:24.00 6366:23.88
step 1 (text-only control) 1697:22.63 3265:22.07 3175:21.62 94763:21.50 92644:21.21 3265:23.75 1697:23.38 5220:23.38 3908:23.13 883:23.13

Read three things off that:

  1. The prefill matches the reference. Same four tokens on both sides (32 1986 785 2082, one swap in order), values within 2.7, and 43288/11613 in the fifth slot are the same kind of near-tie. So the vision features, the interleaved position planes, the DeepStack sets and the prefill KV are right — items (2) and (3) from your review were real fixes, and they are enough to make prefill agree numerically.
  2. The text-only decode agrees too. Same two candidates, swapped order, within 1.7 — so step() itself is sound for a cache holding only text rows.
  3. The image prompt's first decode step is flat at ~21 while the reference is peaked at 27. Divergence is confined to the single-token step once the cache holds image rows.

That narrows the next attempt to one place: what step() does differently when the cache contains image rows — the per-row rotations stored at prefill versus the scalar position used for the query at decode, or the cache's row accounting. Not the vision path, not the positions, not DeepStack, not placement.

The image answer is still bad and I am not claiming otherwise; this is the measurement that says where to look next, and it is in the PR rather than in my head.

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown

Preparing review...

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: CHANGES at 1e1df277473befe08b09d8e9ec070a342a7b005b

09072de fixes everything I'd raised, and I checked each fix at this head:

  • MRoPE. q4nx_mrope_axis() is the reference's apply_interleaved_mrope exactly: i%3==1 && i<3·sec[1] → h, i%3==2 && i<3·sec[2] → w, otherwise t. I built and ran tests/q4nx_mrope_test.c (gnu17), and all arms pass, including 24/20/20 and the 43-of-64 count against the contiguous reading.
  • DeepStack is now added after text layer l, for l < n_deepstack, at the image rows only. That's Qwen3VLTextModel.forward:840.
  • The parsers let strtol move the cursor in both the mrope_section and deepstack_visual_indexes loops.
  • The tools. /tmp/qvl and the out-of-tree hybrid_input import are gone, the header is single, main() returns quantize's rc, and --stream self-checks against the bar.

One new blocker on the decode side, plus one latent one:

1. After an image, step() attends to the wrong number of cache rows.

  • hybrid_prompt_probe.cpp:136-138 decodes with f.step(tok, pos + i - 1), where pos is the MRoPE position (positions[last] + 1, "delta already applied"). That's the right rotary position.
  • But step() passes that same pos to attention(qkv_use, pos, nullptr, …), and attention reads T = pos + 1 cached rows, while appending K/V at the end of the cache (insert(end)).
  • After an image, the MRoPE position is far smaller than the cache length, because rows_img image rows occupy only a small h/w grid of positions. That has two effects:
    • the first generated token attends to only the first pos + 1 rows, which drops the end of the prompt, i.e. the text after the image (the question);
    • every later token appends beyond T, so it never attends to the tokens it just generated.
  • Text-only prompts hide this completely, because there the positions equal the indices.
  • So the image prompt's answer can't be right yet, whatever the vision rows are. It's also a sufficient explanation for a bad answer, so please re-check the "A\n" against this before reading anything into the vision side.
  • step_batch already keeps the two apart: attention(*in, start_pos + m, trio, …) passes the cache index and the 3-plane positions. step() needs the same split: a cache index for T/the window, and a rotary position, either a pos3 or a per-sequence MRoPE delta, as HF's rope_deltas does.

2. MoE layers skip the DeepStack add. The add now sits at the end of the layer body, but the MoE FFN branch ends in continue; (q4nx_forward.cpp:3451), so on a MoE layer the add never runs. The Qwen3-VL-*-A3B models are MoE with DeepStack. This pack is dense, so it's latent here. Moving the add into a helper called on both paths, or before the MoE continue, closes it.

Small:

  • The q4nx_semantics.h comment says a contiguous split misplaces "40 of the 64", while the test and the commit say 43.
  • self_check's docstring says "1 if it clears bar, 0 if it does not". The code is return 0 if ok else 1, which is correct, so the docstring is the inverted part.
  • cur_layer_ = l; // (removed with the old pre-layer deepstack block by accident; …) describes history rather than the code. "the layer index the helpers read" says what it's for.

…position

The image prompt now captions. "A pixelated digital art piece featuring a" against the reference's "A retro-style video
game screen showing a" -- two readings of the same picture, where every previous head stopped at "A\n<|im_end|>".

The review found the real reason, and it was mine: `step()` used one number for two jobs. `pos` rotated the query (a
MRoPE position, with the rope delta applied) and also set the attention window (`T = pos + 1`), while the new K/V is
appended at the END of the cache. For a text prompt those coincide, which is why the text-only control was always
coherent. After an image they do not: the 196 image rows walk a small h/w grid, so the position after the image is 36
while the cache holds 220 rows. The first generated token therefore attended to rows 0..36 only -- the prompt with the
question cut off -- and every later token appended past the window it was reading, so it never saw what it had written.

`step(token, pos, logits, cache_pos = -1)` now takes the slot explicitly and `attention()` gets it as the existing
`kv_len` (its only effect is `T = kv_len > 0 ? kv_len : pos + 1`, so the rotary side is untouched). The probe passes
`cache_pos = rows + i - 1`, the index the token lands in. Text prompts pass -1 and are unchanged, and the control proves
it: `32 1697 594 3579 374 25244 95701 553` before and after.

Top-5 on the same inputs, after the fix:

  step 1  ours: 12955:27.90 7377:26.27 6366:26.02 16771:25.56    HF: 16771:27.13 12955:26.00 23790:24.50 36190:24.00

Same candidate set, the top two swapped inside a 2.3 gap -- an int8/q4nx-divergence-shaped difference, not a structural
one. Prefill is unchanged and still matches the reference (32, 1986, 785, 2082; values within 2.7).

Also from the same review:

  * **DeepStack on a MoE tail.** The add sat at the end of the layer body, which a MoE layer never reaches (`continue`).
    It is now a lambda called on both tails, so a Qwen3-VL-*-A3B MoE layer gets its set too. Latent for this dense pack.
  * `q4nx_semantics.h` said a contiguous split misplaces "40 of the 64"; the measured number, in the test and the
    commit, is **43**.
  * `self_check`'s docstring had its return value inverted; the code was right.
  * `cur_layer_ = l`'s comment described the history of my own edit; it says what the variable is for now.

q4nx_mrope_test, npu_rmsnorm_offset_test and q4nx_layer_kinds_test all pass.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Re-verdict please at 6a5eb0e7. Blockers 1 and 2 fixed, and 1 was the whole thing: the image prompt captions now.

answer
ours, 6a5eb0e7 "A pixelated digital art piece featuring a"
HF reference "A retro-style video game screen showing a"
every head before this "A\n<|im_end|>"

1. step() used one number for two jobs — your diagnosis, confirmed by the fix. pos rotated the query and set the window (T = pos + 1) while K/V is appended at the end of the cache. Text prompts hide it because positions equal indices; after an image the position is 36 and the cache holds 220 rows, so the first token attended to rows 0..36 — the prompt with the question cut off — and later tokens appended past the window they read. step(token, pos, logits, cache_pos = -1) takes the slot explicitly and hands it to attention() as its existing kv_len (whose only effect is T = kv_len > 0 ? kv_len : pos + 1, so the rotary path is untouched). The probe passes cache_pos = rows + i - 1. Text prompts pass -1: the control is byte-identical before and after (32 1697 594 3579 374 25244 95701 553).

Top-5 after the fix, same inputs:

step 1  ours: 12955:27.90 7377:26.27 6366:26.02 16771:25.56
        HF  : 16771:27.13 12955:26.00 23790:24.50 36190:24.00

Same candidate set, top two swapped inside a 2.3 gap — int8/q4nx divergence shaped, not structural. Prefill is unchanged and still matches (32, 1986, 785, 2082; within 2.7).

2. DeepStack on a MoE tail. The add is now a lambda called on both tails, so the MoE branch's continue no longer skips it. Latent for this dense pack, closed for the A3B models.

Small: the header now says 43 of 64 (measured; the test and the commit already did), self_check's docstring no longer inverts its return value, and cur_layer_ = l's comment says what the variable is for instead of narrating my own edit.

q4nx_mrope_test ok, npu_rmsnorm_offset: ok, q4nx_layer_kinds: passed (0 failures).

On gate 2's wording. The greedy sequence is not token-identical to HF: we pick 12955 where HF picks 16771 at a 1.5-point margin, then continue differently. Both are correct readings of the same picture, and the divergence is at the level the int8/q4nx pack explains — but "matches the reference" is the gate's wording, so I am not calling it passed. Say which you want: token-identical is not reachable through a quantised pack, and if a semantic match is the bar, this is the evidence to judge it on.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 6a5eb0e7, with what I checked and what I did not both spelled out. This sat 7 h without a verdict,
so I reviewed the part where correctness actually lives rather than the file count.

The axis rule — verified exhaustively, not read

q4nx_mrope_axis() is the whole claim of the PR, so I compared it against the canonical interleaved rule (axis = i%3 while i//3 < section[axis], else the temporal plane):

  • identical on 7 section sets × 64 frequencies — [24,20,20], [11,11,10], [0,0,0], [1,0,0], [21,21,22],
    [24,20,0], [64,64,64]; no mismatch;
  • at [24,20,20] the counts are exactly 24 t / 20 h / 20 w, matching the test's assertion;
  • the contiguous reading differs on 43 of 64 — the number the test asserts, reproduced independently.

So tests/q4nx_mrope_test.c's new case is right, and the rule it pins is the reference's.

The plane indexing — the trap this change could have fallen into, and did not

pos3_ is plane-major (3 × rows), and attention() wants a per-frequency axis lookup, so a naive pos3[mrope_plane(i)]
would have read the wrong row for h and w. The code gathers a per-row trio first
(trio[1] = pos3_[pos3_rows_ + m], trio[2] = pos3_[2 * pos3_rows_ + m]) and the comment names the trap explicitly. With
the three planes equal, trio[axis] == pos for every axis, which is exactly why text prompts stay bit-identical — the
identity is evident from the code, independent of the probe's measurement.

Guards, which are where a silent-wrong-answer bug would hide

  • position_ids present but the pack is not MRoPE → refused by name; MRoPE declared with n_mrope_section < 3 → refused
    "rather than guessing which frequencies are (t, h, w)"; size ≠ 3 × rows → refused with both numbers.
  • deepstack: set count vs sem_.n_deepstack, buffer presence, and the image span (image_lo/image_hi inside 0..rows)
    all checked before anything runs.
  • the RAII Restore clears all injected state (pos3_, deepstack_*, inject_embeds_) on every exit path — the thing
    that would otherwise leak one request's planes into the next.
  • deepstack_add(l) before the MoE branch's continue is a genuinely good catch: the tail would have skipped that layer's
    set, latent on this dense pack and wrong on the A3B MoE models.

What I did not check, so READY is not read as more than it is

  • I did not build or run it. The probe's (a) claim (equal planes byte-identical to the token path, 0 angle difference
    over positions 0..262143) is your measurement, not mine; I verified the reason it must hold, not the run.
  • tools/export_qwen3vl_vision.py and tools/vision_qdq.py (+340 lines) I read only for their role — the vision half is
    not something I can exercise without a Qwen3-VL pack on this box.
  • No device, no ticket, nothing compiled: every claim above is arithmetic and reading.

One documentation nit, not a blocker

The comment says the [24,20,20] rule "is 24 t / 20 h / 20 w", which is true but reads as if section[0] bounded the t
count. It does not — t is whatever is left (here 64 − 40 = 24, and it would also be 24 for [20,20,20]). Worth a half
sentence so a future reader does not add a section[0] check that the reference does not have.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 6a5eb0e712cf6ea666e49cfd7adef9df7d572485

Both fixed and verified, and one of them is a correction of my own earlier review.

The blocker is gone: nullptr → NULL at q4nx_semantics.c:235; gcc -std=c11 -c -I npu/forward/include now gives 0 errors (it was 1 at the last head, and 0 at the merge base).

The axis rule is right, and my previous review was wrong about it. I said the contiguous mapping "matches HF's doubled-section split" and that the sections summing to 64 made the fallback unreachable. The new test states the rule is a stride-3 window (i % 3 == 1 → h under 3 * section[1], i % 3 == 2 → w under 3 * section[2], else t), and I recomputed both mappings over 64 frequencies:

contiguous vs interleaved disagree on   43 of 64     (the test/comment says 43)
counts, interleaved: t 24 / h 20 / w 20
counts, contiguous : t 24 / h 20 / w 20              <- identical histograms

That identical histogram is exactly what my review checked — I verified the sections summed to head_dim/2 and called the mapping exhaustive. A histogram match says nothing about the assignment, and the author's failure signature ("left every image prompt's answer bad while all equal-plane (text-only) prompts stayed exact") is the one that distinguishes them: the probe's equal-plane arm cannot see it, and I read that arm as the strong evidence.

The new tests/q4nx_mrope_test.c arms are the right ones, and they run here:

$ cmake --build build1336 --target q4nx_mrope_test     -> [4/4] Linking C executable
$ ./build1336/q4nx_mrope_test
ok   interleaved [24,20,20] -> 24 t / 20 h / 20 w
ok   i%3==1 is the h window · i%3==2 is the w window · i%3==0 stays t
ok   past 3 * section[1] the h window closes (i=60 is t)
ok   the contiguous reading differs on 43 of 64 frequencies
ok   no mrope_section -> plain (t) axis
q4nx_mrope: ok

q4nx_mrope_axis() in q4nx_semantics.h is the single definition the forward's mrope_plane() now delegates to, so the C test pins the same code the decoder runs — which is the property that makes this testable at all without a device.

Still open from my earlier review and still not blocking: hybrid_prompt_probe and mrope_planes_probe remain add_executable with no add_test, so nothing runs them; and CI still has no step compiling npu/forward/src/*.c as C, which is how the nullptr reached a green build.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 6a5eb0e712cf6ea666e49cfd7adef9df7d572485

Both items from my review at 09072dea are fixed, and the result shows it: the image prompt now captions ("A pixelated digital art piece featuring a", against HF's "A retro-style video game screen showing a"), where every earlier head gave "A\n", and the text control is byte-identical.

What I checked at this head:

  1. step() separates the rotary position from the cache slot.
    • step(token, pos, logits, cache_pos = -1) passes cache_pos + 1 to attention() as its existing kv_len. In attention(), kv_len affects only T = kv_len > 0 ? kv_len : pos + 1 (and, through T, the sliding-window start t0).
    • The rotation still uses pos with pos3 = nullptr, so all three planes take the same value, which is what Qwen3-VL gives generated text positions after its rope_deltas.
    • The probe's cache_pos = rows + i - 1 is right: step_batch_embeds(…, 0, …) appends exactly rows entries, so the first decode token lands at slot rows.
    • -1 keeps every existing caller unchanged.
  2. DeepStack on MoE layers. deepstack_add(l) is now one lambda, called both before the MoE branch's continue and at the end of the dense body, so a Qwen3-VL-*-A3B layer gets its set too.
  3. tests/q4nx_mrope_test.c builds and passes at this head (q4nx_mrope: ok), and the header comment now says 43, matching the test.

Two small notes, not blocking:

  • cache_pos is caller-trusted, and nothing bounds it. attention() reads kcache_[kv_layer][((size_t)t * NKV + kh) * HD] for every t < T, with no check against the cache's actual row count. A caller that passes a slot one too high reads past the vector, silently. The probe gets it right, but this is a new public parameter. One guard in attention() makes a wrong slot a refusal instead of UB:
    if ((size_t)T * NKV * HD > kcache_[kv_layer].size()) { err_ = "attention: window exceeds the cache"; return false; }
  • The MLA path ignores cache_pos: mla_attn(l, xn, o, pos). No VL model here uses MLA, so it's latent, but it should either take the slot or refuse when cache_pos >= 0 && cache_pos != pos, so the two paths can't disagree silently.

Note for the count: the READY posted above at 21:49 is a PR comment, not a review, so it doesn't register as a verdict at this head. This review does.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Gate 2 evidence, three images, each compared against HF on the same ids/positions/pooled features (6a5eb0e7).

image ours HF reference
test448.png (pixel-art game) "A pixelated digital art piece featuring a" "A retro-style video game screen showing a"
yolov8m/demo.jpg (street scene) "A distorted, fragmented view of a street scene captured" "A distorted, split-screen view of a street scene"
zr1-1.5b-livebench.png (chart) "A dark-themed, grid-based visualization of numerous data" "This image displays a collection of six bubble charts,"

All three are on-topic descriptions of the actual picture, which is what the CPU projector arm achieved in Phase 1 and what nothing on this head did before the cache-slot fix. Two of the three are near-verbatim paraphrases of the reference; on the street scene the ids agree on 8 of 10 positions:

ours: 32 61136 11 86095 1651 315 264 8592 6109 17006
HF  : 32 61136 11 6718 29260 1651 315 264 8592 6109
                  ^^^^^^^^^^^^^^^^^ differ

The three disagreements are all the same shape — a near-tie between two plausible next words ("pixelated digital art" vs "retro-style video game", "fragmented" vs "split-screen", "dark-themed grid" vs "six bubble charts") — which is what an int8 weights / q4nx pack divergence looks like, not what a wrong graph, wrong positions or a wrong cache looks like. Gate 1 already pins the graph itself at cosine 1.0000000 against HF on these images, and prefill logits agree to within 2.7.

So: the vision path works end to end, and the remaining difference is the quantised text pack's greedy choices. Token-identical output is not reachable through that pack; if gate 2's "matches the HF reference" is a semantic bar, this table is the evidence. I am not calling the gate passed on the owner's behalf.

Reproducing it: QVL_IMAGE=<file> QVL_OUT=<dir> python hybrid_input.py builds the probe's inputs from the vision graph, hybrid_prompt_probe <q4nx> <elfs> <dir> --decode N [--top-k K] decodes, and hf_ref.py <dir> N prints the reference ids and top-5 for the same inputs.

@bong-water-water-bong
bong-water-water-bong merged commit 4bc4c55 into main Nov 8, 2026
8 of 10 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/npu-mrope-planes branch November 8, 2026 22:11
bong-water-water-bong pushed a commit that referenced this pull request Nov 8, 2026
…a device prefill read as host

engine#1582 reports a 219-row prefill printing n=0 launches and 93 s of host time, while the
very next decode step on the same target reports npu_gemm 656 ms. It is not a device fallback
and not a lost launch: it is this condition.

Every launch site accounts M == 1 to prof_dec_ and M > 1 to prof_pre_ (xdna_target.cpp:900-906,
969-974), but prof_add only fed the step's phase snapshot for prof_dec_:

    if (q4nx_lh::phprof_on() && acc == prof_dec_ && !q4nx_lh::g_in_head) { g_ph... ; launches++ }

so every batched launch was invisible to g_ph. And the [bs] line derives host as "layer time
minus the device slices" (q4nx_forward.cpp:3606, g_ph.layer - gsum), which means a prefill that
launched perfectly on the device reported exactly what a host fallback would: n=0, host = the
whole layer time.

Measurement only: g_ph is the per-step snapshot nothing reads back, so this changes what the
line says, not what the engine does. A step is either batched or per-token, so counting both
accumulators cannot double-count a launch; the g_in_head exclusion is unchanged.

The 93 s wall itself is untouched by this - if the prefill is genuinely slow, the fixed line will
now show where it went (ql/wait/rb instead of all-host), which is the number gate 3 (#1498) needs.

Compile-checked at this head with the CI compiler and flags: g++-16 -std=c++26 -fsyntax-only
-DONEBIT_NPU_RUNTIME_OWN plus the npu include paths, rc 0. Runtime confirmation needs
hybrid_prompt_probe under npu-hold and is stated as pending on the issue, not claimed here.

Fixes #1582
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant