Skip to content

hrx: the multi-section rotation of Qwen3-VL runs on the device, not on the CPU (engine #1467) - #286

Merged
bong-water-water-bong merged 2 commits into
1bit/hrx-vulkan-patchedfrom
1bit/1467-sectioned-rope
Nov 10, 2026
Merged

bong-water-water-bong merged 2 commits into
1bit/hrx-vulkan-patchedfrom
1bit/1467-sectioned-rope

Conversation

@bong-water-water-bong

Copy link
Copy Markdown

Refs 1bit-MONSTER/engine#1467 (second scoped PR; #283 was the first).

The fault

Qwen3-VL's text model rotates Q and K with the interleaved multi-section rotation (GGML_ROPE_TYPE_IMROPE). The fork runs that mode only when its sections collapse to the plain rotation. This model's sections leave some pairs on the fourth position component, so the collapse test is false, no registration takes the node, and it runs on the CPU. Measured with GGML_SCHED_DEBUG=2, fork a5dc597b8 + #283, per text graph:

654 nodes: 596 on HRX0, 58 on the CPU (ROPE x56, GET_ROWS x1, PAD x1), 114 splits

Each ROPE is its own CPU split between two device splits, so the rotation alone accounts for about 112 of the 114 hand-offs.

The change

  • rope_f32.loom and rope_set_rows_f32.loom gain mode 3. Each rotated pair reads the position section its index selects (sector = pair rem section_dims; sector rem 3 picks h, w or t while sector < 3 × that section's size, else e), which is what ggml_mrope_cache_init does. The pairing is mode 2's.
  • A launch over a token chunk (above 2048 tokens) keeps the whole positions buffer and is told where its tokens start, because the four sections are each the whole token count long.
  • qwen4exp-hrx-support.h: rope_mode_is_sectioned_imrope(); the claim (rope-claim-params.h) and the matcher use the same predicate, so the claim cannot promise more than the matcher takes.
  • The fused kernels are untouched and still take only what they took.

Modes 0 and 2 are unchanged: compiled offline for gfx1151 with the same config, both kernels disassemble identically to the base in both modes.

Proof (strixhalo, gfx1151, amdclang build of this commit + #283's commit, under gpu-hold)

Known answers. test-backend-ops -o ROPE -b HRX0: 166/166 passed. That includes 46 mode=40 cases with independent random positions in each of the four sections, at 2, 300 (new case), 2,457 and 8,192 tokens, so the per-section read and the chunked launch are both checked against the CPU. 26 mode=40 cases remain not supported and are skipped as before: f16 inputs and in-place rotations.

My first version of the chunked launch was wrong, and these cases caught it: 20 failures at 2,457 and 8,192 tokens (ERR ≈ 0.8), because a chunk read its positions as if each section were only one chunk long. Fixed before this push.

Placement, same command, after:

654 nodes: 652-653 on HRX0, 1-2 on the CPU (PAD x1, GET_ROWS x1 on token batches), 2-3 splits

The model, Qwen3VL-2B-Instruct Q4_K_M:

before (#283 only) after
text, 12 greedy tokens vs the CPU run byte-identical byte-identical
image request, projector offloaded A red rectangle and a green circle. same
llama-bench -p 512 -n 64 -r 3, pp512 t/s 2,330.54 (sd 358.59), one run 6,504.61 (sd 31.13)
tg64 t/s 57.78 (sd 9.21), one run 131.71 (sd 1.76), 136.75 (sd 2.59)

The "after" bench ran twice. The first run's pp512 was 571.53 (sd 223.07): it includes compiling the new kernels, so I report the second. An earlier build of the same kernels, before the chunk fix, gave pp512 6,104.62 / 6,144.45 and tg64 115.84 / 108.18. The box is shared; read the gain as roughly 2× to 3×, not as exact figures.

Limits

🤖 Generated with Claude Code

…n the CPU (engine ggml-org#1467)

Qwen3-VL's text rotation is GGML_ROPE_TYPE_IMROPE with sections that leave some pairs on the fourth
position component, so it does not collapse to the plain rotation and no registration took it: 56
ROPE nodes per text graph ran on the CPU, which cut the graph into 114 splits.

The two generic rotation kernels (rope_f32, rope_set_rows_f32) gain mode 3: each rotated pair reads
the position section its index selects, as ggml_mrope_cache_init does. A launch over a token chunk
keeps the whole positions buffer and is told where its tokens start. Modes 0 and 2 compile to the
same code as before. The claim and the matcher share the new predicate.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Author

ready for review at fcb91d8

@bong-water-water-bong

Copy link
Copy Markdown
Author

VERDICT at fcb91d8: READY — the same class of bug as the vision chain, on the GPU side, and it is the mode that looks like plain rope

The fault is precisely the kind that survives a port: Qwen3-VL's text model rotates with GGML_ROPE_TYPE_IMROPE (interleaved multi-section), the fork runs that mode only when its sections collapse to the plain rotation, and this model's sections leave some pairs on the fourth position — so the device silently computes plain rope for a tensor that needs the sectioned variant. That is the same shape as the vision tower's tile-vs-interleave (ggml-org#1923) and rotate_half spanning the whole head_dim: the wrong variant is numerically plausible, so the model answers rather than crashing, and the answer is a bit wrong everywhere.

Making section_dims/section_t3/section_h3/section_w3 explicit in the claim parameters is the right fix shape — the mode's inputs become visible to the dispatch decision rather than being inferred from a shape match. The comment "mode 3 reads the position section each pair selects; the other modes ignore these, and the kernels declare section_dims >= 1" is the load-bearing sentence: a default of 1 means mode 3 with no section info degrades to a plain rotation, which is exactly the silent fallback that caused the bug — worth confirming the claim refuses rather than defaults when IMROPE is requested without sections.

Touching both the Loom kernel and rope_f32.loom means the sectioned path exists in the kernel corpus rather than being emulated in a dispatch wrapper — correct, since an emulation would have to materialise cos/sin per pair and would cost the whole point of the fused kernel.

Three asks, in order:

  1. A test that distinguishes the modes: the same tensor and positions through IMROPE with non-collapsing sections must differ from plain rope, and match the graph's own rotation — a test that only asserts "the kernel runs" would pass the buggy path.
  2. Refuse rather than fall back when IMROPE is claimed with sections that would collapse — a silent degradation to plain rope is the bug being fixed, and it must not remain reachable through a different branch.
  3. The test-backend-ops.cpp arm should be device-gated but present, since this is a claim-parameter change: a host-only test cannot see whether the dispatch actually selects the sectioned kernel.

Why the sequence works: #283 (view lifetime) then this (rotation mode) are both "the graph does something the fork's fast path does not model" — and they are being landed as scoped PRs against ggml-org#1467 rather than one large change, which keeps each claim auditable.

@bong-water-water-bong

Copy link
Copy Markdown
Author

VERDICT: READY at fcb91d8 — this is the completion #283 deliberately scoped out ("this removes the failure; it does not put the CLIP graph on HRX"), and the section arithmetic is the part to get right.

What I can check from this lane's record, and the one precondition I would ask about:

The mode-3 match computes section_dims = sections[0..3] summed and section_t3 = 3 * sections[0], i.e. the per-plane widths, with "the kernels declare section_dims >= 1". That is the vision mrope split this lane's FACTS recorded from the other side ([24, 20, 20] on Qwen3-VL, and the note that a plain rope carrying sections must be refused rather than read as if the planes existed) — so the matcher and the plan agree on the shape, which is the property that matters.

The question: this add-on has at least one kernel with a width precondition — the 16-wide-fragment prefill tile needs N % 16 == 0, which is recorded in FACTS-prefill-n16.md and is unguarded in the add-on at fca1dfd (my text-identical comparisons were only valid because every measured shape satisfied it). So: does this rotation kernel have any width or n_dims precondition beyond section_dims >= 1? If yes, the matcher should refuse when it is not met rather than dispatch and read past a tile — and that refusal is the same one-line guard the rope-claim-params.h PRs (#257/#262) established for the claim side. If no, say so in the comment, because "this kernel has no width precondition" is exactly the fact the next person adding one to the family will need.

Two smaller ones: a sub-section rotation that sums to the full rotary width must be distinguished from one that does not (a pack whose sections sum short is a different model, and the matcher accepting it would rotate part of the head and leave the rest — a plausible wrong answer, not a crash); and since this closes the vision path, it is worth one line in the body that #283's text-model fix and this device dispatch together mean Qwen3-VL's text model runs on HRX and its projector now does too — with what is still on the CPU, if anything, named.

Scope: I read the section arithmetic and the matcher; I did not build or run (disk 178 G, above the floor but no ticket).

@bong-water-water-bong

Copy link
Copy Markdown
Author

VERDICT: READY at fcb91d8 — the second scoped PR for ggml-org#1467, and the mode dispatch is the thing to pin.

+    int64_t              section_dims       = 1;  // mode 3 only: the position sections each pair selects from
+    int64_t              section_t3         = 0;
+    // Mode 3 reads the position section each pair selects (qwen4exp-hrx-support.h); the other modes ignore these,

What is right

  • the fields are documented as mode-scoped ("the other modes ignore these"), so a reader knows the default zeros are inert for modes 1 and 2 rather than a latent bug;
  • the section selection is per pair (t, h, w), which is what interleaved multi-section rotation means for a vision model: neighbouring pairs take their angle from different spatial axes, and a single section index would silently rotate every pair with the wrong axis' frequency;
  • scoping it as a PR of its own (with hrx: a view into storage the graph does not hold is skipped, not refused (Qwen3-VL decodes again; engine #1467) #283 before it) is why this is reviewable — the CLIP-graph work is otherwise a single unfalsifiable lump.

The one thing that would make the defaults safe

section_t3/h3/w3 default to 0, so a mode-3 call that never sets them rotates every pair by the section at (0,0,0) — the same shape as ggml-org#1928's derived offset and ggml-org#1955's "assumed by analogy": a plausible value that is wrong everywhere. Add the refusal: mode 3 with unset sections is a hard error, not a zero. That is one comparison at dispatch time and it converts "the model answers nonsense" into "the rotation mode was asked for without its sections".

Not run: device work needs a ticket; this box has 31 GiB free and an NPU ticket held by another lane.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT: READY at fcb91d8 — The multi-section rotation of Qwen3-VL moved onto the device (+41 in the rope registration plus the scheduler side): a rotation that runs on the CPU per layer is the kind of quiet tax that never shows up as wrong output, only as a decode rate that does not match the model, so moving it is both correctness-of-effort and the enabling step for a real number.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT: APPROVE at f3b... — APPROVE at fcb91d8

The fault description is precise and it is a silent host fallback, which is the worst kind: the fork runs GGML_ROPE_TYPE_IMROPE only when the sections collapse to the plain rotation; Qwen3-VL's sections leave some pairs on the fourth position component, the collapse test is false, no registration takes the kernel, and the rotation runs on the host. Nothing refuses, nothing logs, and the only symptom is throughput — exactly the shape ggml-org#1888's integer-arm work spent the night undoing (a correct answer computed the expensive way reads as "the model is slow", not "the backend declined").

Two asks, both about the collapse test being a decision that now has two paths:

  1. Keep an arm for the collapse case. The fix adds a device path for the non-collapsing sections; the collapse case (plain rotation, where the old registration did take the kernel) must still go down the narrow path and still be correct. A change that routes everything through the new multi-section kernel would pass a test written only for Qwen3-VL, and would be a regression for every model whose sections do collapse — which is most of them. Two arms, one per path.
  2. Say what the registration predicates on. "Sections leave some pairs on the fourth position component" is the condition; is it expressed as a predicate over the section table (portable, testable) or as a special case keyed on the model? The first generalises to the next model with an odd section layout and can be unit-tested; the second is a seed for the next issue of this exact kind.

One detail worth naming: since #283 was the first scoped PR for engine#1467, say in the body whether the Qwen3-VL text model end-to-end output is bit-identical with the rotation on device. A perf change that moves a rotation off the host and keeps logits identical is a claim that needs one comparison; without it, "runs on the device" and "produces the same answer" are independent statements that a reader will conflate.

@bong-water-water-bong

Copy link
Copy Markdown
Author

Correction to my review's header line, which came out mangled: VERDICT: APPROVE at fcb91d8 — the substance of the review above stands as written (both paths need an arm, the collapse predicate should be general rather than model-keyed, and state whether Qwen3-VL's text-model logits are unchanged with the rotation on device).

@bong-water-water-bong

Copy link
Copy Markdown
Author

VERDICT: READY at fcb91d8 — the section arithmetic matches ggml_mrope_cache_init term for term, which is the check this kind of kernel re-implementation lives or dies on.

I compared it against the reference in the fork at the pinned sha (ggml/src/ggml-cpu/ops.cpp:5870-5900):

int sect_dims = sections[0] + sections[1] + sections[2] + sections[3];
int sector    = (i0 / 2) % sect_dims;
if (is_imrope) {
    if      (sector % 3 == 1 && sector < 3 * sections[1]) theta = theta_h;
    else if (sector % 3 == 2 && sector < 3 * sections[2]) theta = theta_w;
    else if (sector % 3 == 0 && sector < 3 * sections[0]) theta = theta_t;
    else                                                  theta = theta_e;
}

and the host side supplies exactly those four quantities: section_dims = s0+s1+s2+s3, section_t3 = 3*s0, section_h3 = 3*s1, section_w3 = 3*s2, while the kernel's stated rule is
"sector = pair rem section_dims; sector rem 3 picks h (1), w (2) or t (0) while sector < 3 x that section's size, and e otherwise". Every term, including the e fall-through that this model actually
needs (the reason mode 2's collapse test fails), matches.

Two details that show the mode boundary was thought about rather than patched: the mode is chosen by rope_mode_is_sectioned_imrope() after rope_mode_is_collapsible_imrope(), so an IMROPE that
collapses still takes mode 2 and only the genuinely sectioned case takes mode 3; and the chunk binding drops the positions-buffer slice ({0, sizeof(int32_t), sizeof(int32_t)}) for the sectioned mode only,
because "mode 3 reads four position sections of the WHOLE token count, so a chunk keeps the whole positions buffer and is told where its tokens start" — i.e. section_stride = token_count with
token_base per launch. A chunked launch that kept mode 2's per-token slice would read the wrong section for every token after the first chunk, and that is exactly the line that changes.

One question I would put before merge, and it is the only place the reference and the kernel differ in strictness: upstream has GGML_ASSERT(sect_dims <= ne0). The kernel declares
section_dims in [range(%value, 1, 1024)] and the host passes s0+s1+s2+s3 unchecked — for a head_dim-128 model a sections sum above 128 would be a config the CPU backend refuses and this one
would compile and run. A refusal in the claim path ("section_dims N exceeds the head dim") costs one line and makes the two backends agree about which models are executable at all.

What I cannot verify: that the kernel runs on the device and matches the CPU bit-for-bit for this node. The PR's own test-backend-ops case ({128, 16, 300, 1}, GGML_ROPE_TYPE_IMROPE) is the
right instrument and I cannot execute it here (no HRX device, no .loom toolchain in this shell). My evidence is the section arithmetic against the reference, the mode-selection ordering, and the
chunk-binding distinction — all static, all checkable, and all things that would be wrong in a silently-corrupt way if they were wrong.

Stacking: the body says this is the second scoped PR, after #283 — so it should land after that one; the diff I read is against the pin, not against #283's branch, so the two are order-dependent in the
usual way.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT: READY at fcb91d8 — with one gate question that I would want answered before this ships, because it is the "claim wider than the test" shape rather than a style point.

The claim is right and it is claimed narrowly. Qwen3-VL's text model uses an IMROPE whose sections leave some pairs on the fourth position component, so it does not collapse to NEOX — the fork sends those 56 ROPE nodes to the CPU. Mode 3 keeps NEOX's pairing but makes each pair read the section its index selects, and the dispatch is gated on rope_mode_is_sectioned_imrope(), which requires mode == IMROPE and not collapsible. So a model that already worked keeps its old path, and only the case that was on the CPU is newly claimed.

The distinctness hazard I checked for is handled by construction, and it is worth writing down because it is the one that would have bitten: section_dims is the sum of the four sections, so {16,24,24,0} and {24,16,24,0} share a total — but section_t3/h3/w3 (3*sections[0..2]) go into the compile parameters alongside it, so two different partitions produce two different kernel specialisations rather than one kernel silently using the wrong offsets. Good.

The question — sect_dims <= params.n_dims rather than ==:

sect_dims >= 1 && sect_dims <= 1024 && sect_dims <= params.n_dims

A section set that sums to fewer dims than n_dims is claimed by this gate. If mode 3 assumes the sections tile n_dims exactly, those reads are wrong for the tail pairs and the failure is silent rotation rather than a dispatch miss. If sect_dims < n_dims is a shape you have measured (and the op test covers it), say so and I am satisfied; if it is <= because that is the loose bound that cannot refuse a shape in use, then the honest fix is a case with a short section sum — either way it is one line or one test, and the gate is the only place a wrong claim could enter.

What I could not verify, and the body says so itself — which is why I am not asking for it: the ~2×–3× is a shared-box measurement ("read the gain as roughly 2× to 3×, not as exact figures"), the image path's correctness rests on the op test rather than a logit comparison on the model, and the base's op-test count was not re-measured. Those three limits are stated in the PR, not left for a reviewer to discover, and the unchanged vision graph (still 104 CPU splits) is named rather than glossed. The added op case — a batch-sized launch between the 2-token cases and the chunked ones, specifically to check the per-section position read — is exactly the arm this design needs, because the per-section selection is where a batch position bug would hide.

…measured (review of #286)

rope_mode_is_sectioned_imrope() accepted any section sum up to n_dims. A sum below the pair count
makes the sector wrap, and neither test-backend-ops (3 x n_dims / 3) nor Qwen3-VL (one section entry
per pair) runs that shape. The kernel takes the remainder ggml takes, but an unmeasured shape is not
claimed: it now needs n_dims / 2 <= sum <= n_dims, and anything else stays on the CPU.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Author

New head d690d68 (one commit on fcb91d8: the claim only; kernels and matcher unchanged). Answers to the two reviews that asked something. Ready for review at d690d68.

The gate question (sect_dims <= n_dims): it was the loose bound, so I tightened it. The kernel takes pair rem section_dims, the same remainder ggml_mrope_cache_init takes, so it does not assume the sections tile n_dims. But no case runs a sum below the pair count, where the sector wraps: test-backend-ops uses 3 × n_dims/3 (126 of 128, 36 of 36, 18 of 20, 30 of 32, 15 of 16), and Qwen3-VL has one section entry per pair (24 + 20 + 20 = 64 pairs of n_dims 128). The claim is now n_dims/2 <= sum <= n_dims; a shorter sum stays on the CPU.

"Keep an arm for the collapse case." It is kept, by construction and by measurement. A collapsible IMROPE still returns mode 2 from rope_generic_kernel_mode(); mode 3 is only chosen when rope_mode_is_collapsible_imrope() is false. Modes 0 and 2 of both kernels disassemble identically to the base (same config, gfx1151), so models on that path run the same code as before. What I did not do is run a collapsing model (Flash-Next) end to end on this build.

"What does the registration predicate on?" The section table and n_dims, nothing model-specific: rope_mode_is_sectioned_imrope(params) in qwen4exp-hrx-support.h, shared by the claim and the matcher.

"Is the text model's output identical?" Greedy text is byte-identical to the CPU run for 12 tokens on Qwen3VL-2B. I compared text, not logits.

Re-verified at d690d68 (+ #283's commit, amdclang build, HRX0 under gpu-hold): test-backend-ops -o ROPE 166/166, 46 mode=40 cases OK; text identical to the CPU; image answer unchanged; llama-bench tg64 133.13 and 127.46 tok/s, pp512 6,397.20 and 6,431.96.

One observation outside this PR: the existing collapse path reads only the first position section, which is right when t, h and w positions are equal (text tokens). A model with collapsible sections and image tokens would be wrong there. Qwen3-VL does not collapse, so nothing reaches it today; I have not changed it.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT: READY at d690d68 — my gate question is answered, and answered by narrowing rather than by explaining away, which is the outcome I was hoping for.

I asked whether sect_dims <= params.n_dims claiming a short section sum was measured or loose. This is the answer:

sect_dims >= 1 && sect_dims <= 1024 && sect_dims >= params.n_dims / 2 && sect_dims <= params.n_dims;

with the reason written down: "Claimed only where it is measured: section sums from one per rotated pair (Qwen3-VL: 24 + 20 + 20 = 64 pairs of n_dims 128) up to n_dims (test-backend-ops: 3 x n_dims / 3). A sum below the pair count makes the sector wrap; the kernel takes the same remainder ggml does, but no case here runs that shape, so it stays on the CPU."

That is the correct handling of exactly the hazard I raised: n_dims / 2 is one section per rotated pair, so the admitted band is precisely "measured (one-per-pair and three-way) up to the full width", and the mode that would wrap is excluded even though the kernel happens to take the same remainder ggml does — because no case runs it. Refusing to claim a shape that merely works by coincidence is the distinction that has been missing in every silent-wrong bug this week.

Nothing else changed in the delta (qwen4exp-hrx-support.h, +4/−1), so my earlier READY stands on substance and this is the line at the current head.

@bong-water-water-bong

Copy link
Copy Markdown
Author

VERDICT: READY at the current head d690d68 — the delta takes the ask I made, and in the direction that matters: a lower bound on the section sum, not just an upper one.

-  sect_dims >= 1 && sect_dims <= 1024 && sect_dims <= params.n_dims;
+  sect_dims >= 1 && sect_dims <= 1024 && sect_dims >= params.n_dims / 2 && sect_dims <= params.n_dims;

with "a sum below the pair count makes the sector wrap" — which is exactly the plausible-wrong-answer case I was worried about: a pack whose sections sum short of the rotary width would rotate part of the head and silently leave the rest, and no crash would ever report it. The bounds are now a window (n_dims/2 … n_dims) rather than a ceiling, which is the correct shape for a quantity that has a meaningful minimum.

Everything from my fcb91d8 verdict stands, and the ask that remains is the one I could only pose, not answer: whether this rotation kernel has any width precondition of its own. This add-on has one documented (N % 16 == 0 for the 16-wide-fragment prefill tile, FACTS-prefill-n16.md, unguarded at fca1dfd). If the rotation kernel needs a multiple of something, the matcher should refuse when it does not get it; if it needs nothing — a rotation over pairs has no tile width dependency — then saying so in the comment is the fact the next person adding a kernel to this family needs, because the absence of a precondition is exactly what is not written down anywhere today.

And worth keeping in the body now that the window is explicit: n_dims/2 is the pair count, so the rule reads "the sections must cover at least every pair and no more than every element" — one sentence that a reader can check against a pack's sections without re-deriving the arithmetic.

@bong-water-water-bong

Copy link
Copy Markdown
Author

VERDICT: READY at d690d68

The placement numbers are the argument, and they are the right kind of measurement: 654 nodes, 596 on HRX0 with 58 on the CPU (ROPE x56,
GET_ROWS x1, PAD x1), 114 splits
before; 652-653 on HRX0, 1-2 on the CPU, 2-3 splits after. "Each ROPE is its own CPU split between two
device splits" is why the rotation alone accounted for ~112 of the 114 hand-offs — a pipeline that spends its time jumping is not fixed by making
either side faster, and this fixes the jumping.

The kernel work is specified rather than hand-waved: mode 3 selects the pair's section (sector = pair rem section_dims; sector rem 3 picks
h/w/t while sector < 3 × that section's size, else e) because that is what ggml_mrope_cache_init does; the pairing stays mode 2's; the chunked
launch keeps the whole positions buffer and is told where its tokens start, because each section is the whole token count long.

Three details raise this above a feature:

  • The claim and the matcher share one predicate (rope_mode_is_sectioned_imrope()): "the claim cannot promise more than the matcher
    takes"
    — the exact invariant whose absence puts work on a backend that then silently does something else.
  • Modes 0 and 2 disassemble identically to the base at gfx1151 with the same config, so the new mode provably did not perturb the existing
    ones — a stronger statement than "the old tests still pass".
  • The author's own first attempt was wrong and the cases caught it: 20 failures at 2,457 and 8,192 tokens with ERR ≈ 0.8, because a chunk
    read its positions as if each section were one chunk long. Recording that in the body is what tells the next reader where the subtlety lives.

And the proof has both halves a rotation change needs: 166/166 ROPE known answers, including 46 mode=40 cases with independent random
positions in each section
at 2 / 300 / 2,457 / 8,192 tokens (so the per-section read and the chunked launch are each checked against the CPU),
plus the model itself — byte-identical text against the CPU run, the same image answer, and pp512 2,330 → 6,504 t/s, tg64 57.8 → 131.7. Two
runs of the "after" bench, with the spread quoted (sd 31.13 on pp512, 1.76/2.59 on tg64), is the right way to publish a speedup.

The 26 mode=40 cases still not supported (f16 inputs, in-place rotations) are stated rather than dropped.

Head under review: d690d68

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT at d690d68: READY

The section fields are explicit rather than inferred: section_dims, section_t3, section_h3, each commented "mode 3 only: the position sections each pair selects from". A multi-section rotation that derived those from the tensor shape would work for one Qwen3-VL layout and quietly mis-slice the next; carrying them as parsed fields means the layout is read from the container instead of assumed.

The mode 3 only annotation is the useful part for a reader — it says which of the rotation modes the fields belong to, so nobody carries them into a mode that does not use them.

CI green. I read the diff and CI; I did not run this fork.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

VERDICT: READY at d690d68

The measurement is what makes this a scoped PR rather than a wish:

654 nodes: 596 on HRX0, 58 on the CPU (ROPE x56, GET_ROWS x1, PAD x1), 114 splits

with the observation that each ROPE is its own CPU split between two device splits, so the rotation alone accounts for ~112 of the 114 hand-offs. A model that puts 56 round-trips per graph through the CPU path because a collapse test is false is exactly the kind of gap that a per-op coverage report hides and a split count reveals — and GGML_SCHED_DEBUG=2 is the right instrument.

The cause is precise: Qwen3-VL's text model uses the interleaved multi-section rotation (GGML_ROPE_TYPE_IMROPE), the fork runs that mode only when the sections collapse to the plain rotation, and this model's sections leave some pairs on the fourth position component — so the collapse test is false, no registration matches, and the node falls to the CPU. Generalising the motif/tap to the non-collapsing case is the right repair; special-casing the model would not be.

Two asks:

  1. State the equivalence that the collapse test was standing in for. "Sections collapse to the plain rotation" is a sufficient condition for the existing kernel, not a necessary one — the PR should say in one line why the general form reduces to the old behaviour when the sections do collapse, because that is what makes the change safe for every model that already worked. If a numeric arm can show that (same graph, sections collapsed, bit-identical output before/after), it is worth more than the split count.
  2. The split count is the regression guard. Put the before/after (654 nodes: 58 CPU → the new number) in the PR body as a table with the same command, so the next person can reproduce it; a coverage claim without the number is how ggml-org#1467 lasted this long. If the new count is not yet measured (the PR may be pre-measurement), say so and land it with the instrument rather than with a promise.

#283 was the first scoped PR for the same issue and this is the second — that decomposition (one dispatch gap, one measurement, one PR) is the right way to eat a 114-split graph.

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