Repository navigation
npu: the decoder takes the three position planes an interleaved-MRoPE pack needs (refs #1311, #799) - #1498
Conversation
|
#1498 ready for review at |
…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.
1c1b34d to
dd7bacf
Compare
|
Rebased onto main |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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 × rowssize check, and the RAII reset ofpos3_; - the old three-argument overload kept, so
embeds_equiv_probeandmrope_planes_probeboth 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()followmrope_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 tomrope_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_mropefor one (t, h, w) trio. That's the row that would catch this class of mistake.
Two smaller items:
- The
mrope_sectionparser can read past the array. After eachstrtolthe loop doesp2++. For a valid three-element array it stops atgot == 3, but for a short one ([16, 24])strtolleavesp2on], 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) and6236d44(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
|
The last engine piece for gate 2 is in, at Qwen3-VL's vision tower emits three extra feature sets (from blocks 5/11/17) and the reference adds each to the
Rows from
With this, the decoder side of the owner's vision-graph order is complete: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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_.mropeis set only frommrope_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'sIMROPE(sector % 3) atggml/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.
|
The wiring runs end to end at
Defect, mine, caught by the wiring rather than by a row: the deepstack injection added set s to every row of What runs now (host-GEMM, box at load 40): a 219-token prompt with 196 image rows, 3 deepstack sets, layout What does not yet: the decoded answer is not the reference's. First attempt emitted 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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
|
Localised the bad answer with two controls at
What that leaves is the placement, and it is now a short list: the image span's exact width for this image |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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_layersis parsed fromvision_config.deepstack_visual_indexes, which is[8, 16, 24]. Those are vision-tower layers:Qwen3VLVisionModeltaps 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):So set i is added after text decoder layer i, for i = 0, 1, 2.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]) - 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
strtolsetsp2onto the terminator, the loop'sp2++steps past it. For[24, 20], that step skips the], and the scan keeps reading digits from later JSON intomrope_section[2]. The same pattern sits in thedeepstack_visual_indexesloop at:240-241. - Fix:
continuewithout the increment after a successfulstrtol(or usep2 - 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
left a comment
There was a problem hiding this comment.
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.
|
Streaming QDQ is in (
Two bugs on the way, both now commented in the code:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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:
- The DeepStack layer and timing (
q4nx_forward.cpp:3243-3248). Set s is added before text layerdeepstack_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). mrope_plane()is contiguous (q4nx_forward.h:388-392), but the packs declaremrope_interleaved. The reference'sapply_interleaved_mrope(:372-386) gives:At [24,20,20], 43 of 64 frequencies get the wrong axis.plane(i) = (i%3==1 && i<3*sec[1]) ? 1 : (i%3==2 && i<3*sec[2]) ? 2 : 0- The parser overrun. The loop's
p2++/p3++afterstrtolsteps past]on a short array (q4nx_semantics.c, both themrope_sectionanddeepstack_visual_indexesloops).
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:
vision_qdq.py:41putssys.path.insert(0, "/tmp/qvl")in the import path, and:82importshybrid_inputfrom it.hybrid_input.pyis not in the tree (git ls-treeat this head). It exists only in your scratch/tmp/qvl, soquantize --streamcan't calibrate from a clean checkout./tmpis shared by every lane on the box, so whatever sits in/tmp/qvlgets imported first.- Add the module to
tools/(or inline the generator, as the comment above it says), and drop the path insert.
main()now throws awayquantize'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
--streamthe 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.
- It became
- Small:
vision_qdq.pycarries the copyright/SPDX header twice (:2-3and:15-16);- its docstring's usage line says
qdq_stream.py; - the module docstring says MatMul inputs are calibrated, while
matmul_weightsexplains 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.
|
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
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.) |
|
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 (
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:
Worth recording for the next reader: transformers wants mrope |
…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.
|
Re-verdict please at Decoder (1–3)
Confession on (3): while moving the block I deleted Be clear about what (2) and (3) did not do: the image prompt is still bad, now Tool (4–6)
The workflow item is yours to land. This app's token has no @@ -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 |
|
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.
|
Logit diff, as promised — and it says the prefill is right while the first decode step is not.
Read three things off that:
That narrows the next attempt to one place: what 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. |
|
Preparing review... |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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'sapply_interleaved_mropeexactly:i%3==1 && i<3·sec[1]→ h,i%3==2 && i<3·sec[2]→ w, otherwise t. I built and rantests/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, forl < n_deepstack, at the image rows only. That'sQwen3VLTextModel.forward:840. - The parsers let
strtolmove the cursor in both themrope_sectionanddeepstack_visual_indexesloops. - The tools.
/tmp/qvland the out-of-treehybrid_inputimport are gone, the header is single,main()returnsquantize's rc, and--streamself-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-138decodes withf.step(tok, pos + i - 1), whereposis the MRoPE position (positions[last] + 1, "delta already applied"). That's the right rotary position.- But
step()passes that samepostoattention(qkv_use, pos, nullptr, …), and attention readsT = pos + 1cached 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_imgimage rows occupy only a small h/w grid of positions. That has two effects:- the first generated token attends to only the first
pos + 1rows, 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.
- the first generated token attends to only the first
- 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_batchalready 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 forT/the window, and a rotary position, either apos3or a per-sequence MRoPE delta, as HF'srope_deltasdoes.
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.hcomment 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 clearsbar, 0 if it does not". The code isreturn 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.
|
Re-verdict please at
1. Top-5 after the fix, same inputs: 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 Small: the header now says 43 of 64 (measured; the test and the commit already did),
On gate 2's wording. The greedy sequence is not token-identical to HF: we pick |
|
VERDICT: READY at The axis rule — verified exhaustively, not read
So The plane indexing — the trap this change could have fallen into, and did not
Guards, which are where a silent-wrong-answer bug would hide
What I did not check, so READY is not read as more than it is
One documentation nit, not a blockerThe comment says the |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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:
step()separates the rotary position from the cache slot.step(token, pos, logits, cache_pos = -1)passescache_pos + 1toattention()as its existingkv_len. Inattention(),kv_lenaffects onlyT = kv_len > 0 ? kv_len : pos + 1(and, throughT, the sliding-window startt0).- The rotation still uses
poswithpos3 = nullptr, so all three planes take the same value, which is what Qwen3-VL gives generated text positions after itsrope_deltas. - The probe's
cache_pos = rows + i - 1is right:step_batch_embeds(…, 0, …)appends exactlyrowsentries, so the first decode token lands at slotrows. -1keeps every existing caller unchanged.
- DeepStack on MoE layers.
deepstack_add(l)is now one lambda, called both before the MoE branch'scontinueand at the end of the dense body, so a Qwen3-VL-*-A3B layer gets its set too. tests/q4nx_mrope_test.cbuilds and passes at this head (q4nx_mrope: ok), and the header comment now says 43, matching the test.
Two small notes, not blocking:
cache_posis caller-trusted, and nothing bounds it.attention()readskcache_[kv_layer][((size_t)t * NKV + kh) * HD]for everyt < 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 inattention()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 whencache_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.
|
Gate 2 evidence, three images, each compared against HF on the same ids/positions/pooled features (
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: 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: |
…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
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_idsis 3 × rows, plane-major (t, h, w) —exactly the layout the reference's
get_rope_indexproduces and AMD'stext.onnxtakes. Empty means the usualstart_pos + row.attention()rotates each rotary frequency by its own axis's position instead of by one scalar.mrope_sectionis parsed (both spellings:rope_scalingfor Hugging Face,rope_parametersfor 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)npu-holdticket, 17 s)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
MatMulonly, so the vision graph's non-MatMul nodes would fall through to the CPU EP — a mixed arm that must be labelled as onerefs #1311, refs #799 (P4's
rope.planes).