Skip to content

sycl : fix get_rows on reordered quantized weights - #30430

Merged
arthw merged 1 commit into
ggml-org:masterfrom
skipzx-lab:sycl-get-rows-reorder
Oct 19, 2026
Merged

arthw merged 1 commit into
ggml-org:masterfrom
skipzx-lab:sycl-get-rows-reorder

Conversation

@skipzx-lab

Copy link
Copy Markdown
Contributor

Fixes #30428

Overview

With GGML_SYCL_ENABLE_OPT=1 (the default), ggml-sycl reorders Q4_0, Q8_0, Q2_K, Q3_K, Q4_K, Q5_K and Q6_K weights in place on their first small-batch mul_mat (8 tokens or fewer), and sets extra->optimized_feature.reorder. That's every type ggml-sycl reorders for 2D weights. get_rows ignored the flag, so a tensor read by both ops returned wrong rows or NaN after the first mul_mat.

It's triggered by tied embeddings kept on the device. llama.cpp itself mostly avoids it because input embeddings stay on the CPU and a tied output gets its own copy, but other ggml programs hit it.

Changes (4 files, +262 / -21):

  • getrows.cpp
    • Q4_0 / Q8_0: new k_get_rows_reorder and get_rows_sycl_reorder. Same indexing as k_get_rows, but reading from the reordered layout via the existing dequantize_q4_0_reorder / dequantize_q8_0_reorder.
    • K-quants: new get_rows_sycl_reorder_k, one work-group per 256-value block of a requested row, calling the existing dequantize_block_qX_K_reorder functions with each type's existing thread count and local scratch.
    • Dispatch: each of the 7 cases checks the reorder flag first. The normal path is unchanged.
  • dequantize.hpp: the 5 K-quant reorder dequantisers now take the block index and output pointer as parameters instead of reading get_group(), so get_rows can call them for an arbitrary row.
  • convert.cpp: the 5 existing callers pass y + group * QK_K and group, the same values as before, so behaviour there is unchanged.
  • tests/test-backend-ops.cpp: new test_get_rows_after_mul_mat. It runs a one-column mul_mat on a weight, then get_rows on the same weight. The mul_mat runs first because it's the first input of the final ADD. Covers all 7 types with a 5e-4 tolerance, same as test_mul_mat.

Additional information

Tested on an Arc Pro B60 24 GB, oneAPI 2026.1, Level Zero 1.14.37020, Ubuntu 26.04, master cbe28bb.

Check Result
New test on master (SYCL) Fails for all 7 types; Q8_0 and Q6_K give NaN
New test with the fix SYCL 7/7, Vulkan 7/7
Existing SYCL0 suites ADD 161/161, GET_ROWS 216/216, MUL_MAT 2272/2272, MUL_MAT_ID 953/953
Refactored convert path Mul_mat at batch 32 and 512 on an already-reordered weight passes for all 7 types. Deliberately breaking the Q6_K call made both fail, so the check does exercise that path. (Local check, not part of the PR.)
acestep.cpp before 16 / 299 / 388 / 521 / 1,300 codes for a target of ~1,200
acestep.cpp after 1,196 to 1,199 codes; Vulkan gives 1,197

How I found it. acestep.cpp's 4B LM (Q8_0, tied embeddings) produced random output lengths on SYCL. Setting GGML_SYCL_ENABLE_OPT=0 fixed it at roughly 4x slower decoding, which pointed at the reorder. Logs and timings are in ServeurpersoCom/acestep.cpp#54, where the acestep.cpp maintainer agreed the fix belongs upstream.

Why fix get_rows instead of skipping the reorder? The reorder happens on the first mul_mat, and the backend can't easily see every other op that'll use the tensor later. Teaching get_rows the layout keeps the fix local, keeps the speedup, and matches what the dequantize path already does for reordered weights.

Related PRs. #30374 (reordered Q4_0 dequantize kernel) and #29107 (IQ3 reorder) are open SYCL reorder work, but neither touches get_rows, so this doesn't overlap.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES. The debugging was done by Claude running in an automated loop, including finding the root cause and writing the fix and the regression test. I've reviewed the changes, confirmed the test fails before the fix and passes after for all 7 types, and verified the acestep.cpp issue is resolved on my Arc Pro B60.

@skipzx-lab
skipzx-lab requested review from a team and ggerganov as code owners October 12, 2026 23:25
@github-actions github-actions Bot added testing Everything test related ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language labels Oct 12, 2026

@arthw arthw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's good job!

The reordered weight only happen in mul_mat of SYCL backend.
We didn't meet the issue #30428 before.

I think it would happen frequently when mix backends (CPU, CUDA, Vulkan) in the future.
It's good to make the other OP to support reordered weight.
It's the base to apply reordered weight in all OPs of SYCL backend.

Thank you!

@anantshri anantshri mentioned this pull request Oct 15, 2026
@ServeurpersoCom

ServeurpersoCom commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Thanks a lot @skipzx-lab, this is exactly the kind of help I needed since I have no Intel hardware to test on. I cherry-picked your get_rows fix into the ggml module used by my music and vocoder projects, so acestep.cpp and yue2.cpp get the full speed SYCL path right now without waiting for the upstream merge. audio.cpp should benefit too once it resyncs its ggml, since several of its decoders share one tensor between the token embedding and the lm_head.

Next on my side is the Snake fusion for SYCL, and your VAE profile shows IM2COL is worth a look too. I'll ping you when there is something to test on the B60.

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

It's good job!

The reordered weight only happen in mul_mat of SYCL backend. We didn't meet the issue #30428 before.

I think it would happen frequently when mix backends (CPU, CUDA, Vulkan) in the future. It's good to make the other OP to support reordered weight. It's the base to apply reordered weight in all OPs of SYCL backend.

Thank you!

Thanks @arthw, agreed on making every SYCL op reorder aware. I'll leave the merge button to you since you own the backend.

@skipzx-lab

Copy link
Copy Markdown
Contributor Author

Thanks @arthw and @ServeurpersoCom for the reviews, and glad it's already useful in acestep.cpp and yue2.cpp.

Happy to test the Snake fusion and anything on IM2COL on the B60 when they're ready. Ping me and I'll run them and report numbers.

@arthw
arthw merged commit 3bb63cb into ggml-org:master Oct 19, 2026
21 checks passed
iki007 added a commit to iki007/llama.cpp that referenced this pull request Oct 21, 2026
…ghts gets its per-block kernels back

Upstream ggml-org#30430 lets get_rows read weights that a mat-mul has already reordered, through the per-block q4_K /
q5_K / q6_K reorder dequantize kernels with an explicit block index. This branch replaced those kernels with the
wide ones (and replayed with its own side winning), so they come back next to the wide kernels, used by get_rows
only. The reorder layouts of q4_0, q8_0 and q2_K .. q6_K are identical here and upstream. The q6_K pair shares a
name: ours takes the store width as a template argument and 4 arguments, upstream's a block index and 5.

Assisted-by: Claude Opus 5.5
iki007 added a commit to iki007/llama.cpp that referenced this pull request Oct 22, 2026
…ghts gets its per-block kernels back

Upstream ggml-org#30430 lets get_rows read weights that a mat-mul has already reordered, through the per-block q4_K /
q5_K / q6_K reorder dequantize kernels with an explicit block index. This branch replaced those kernels with the
wide ones (and replayed with its own side winning), so they come back next to the wide kernels, used by get_rows
only. The reorder layouts of q4_0, q8_0 and q2_K .. q6_K are identical here and upstream. The q6_K pair shares a
name: ours takes the store width as a template argument and 4 arguments, upstream's a block index and 5.

Assisted-by: Claude Opus 5.5
edwardyoon pushed a commit to edwardyoon/focus-llama that referenced this pull request Oct 24, 2026
Co-authored-by: Sam <sam@garagellm.tail7dd858.ts.net>
Gizmo-Ger pushed a commit to Gizmo-Ger/llama.cpp that referenced this pull request Oct 27, 2026
Cherry-pick from upstream sycl-fa-q4-xmx-up. Fixes incorrect get_rows
results when weight tensors use the reorder layout (q4_0/q8_0).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
auRose94 pushed a commit to auRose94/llama.cpp that referenced this pull request Oct 28, 2026
Co-authored-by: Sam <sam@garagellm.tail7dd858.ts.net>
@BrewTestBot BrewTestBot mentioned this pull request Oct 28, 2026
1 task done
iki007 added a commit to iki007/llama.cpp that referenced this pull request Oct 29, 2026
…ghts gets its per-block kernels back

Upstream ggml-org#30430 lets get_rows read weights that a mat-mul has already reordered, through the per-block q4_K /
q5_K / q6_K reorder dequantize kernels with an explicit block index. This branch replaced those kernels with the
wide ones (and replayed with its own side winning), so they come back next to the wide kernels, used by get_rows
only. The reorder layouts of q4_0, q8_0 and q2_K .. q6_K are identical here and upstream. The q6_K pair shares a
name: ours takes the store width as a template argument and 4 arguments, upstream's a block index and 5.

Assisted-by: Claude Opus 5.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Misc. bug: [SYCL] GET_ROWS returns wrong values or NaN on weights reordered by MUL_MAT

3 participants