Skip to content

CUDA: fix MMQ memory fault if n_expert >> n_ubatch - #29941

Merged
JohannesGaessler merged 1 commit into
ggml-org:masterfrom
JohannesGaessler:cuda-mmq-fix-lopsided-tensors
Oct 4, 2026
Merged

JohannesGaessler merged 1 commit into
ggml-org:masterfrom
JohannesGaessler:cuda-mmq-fix-lopsided-tensors

Conversation

@JohannesGaessler

Copy link
Copy Markdown
Contributor

My attention was drawn to these failing test cases by @am17an :

for (bool b : {false, true}) {
    test_cases.emplace_back(new test_mul_mat_id(GGML_TYPE_Q4_0, GGML_TYPE_F32, 512, 10, b, 640, 508, 2560));
}

With the CUDA backend they result in an illegal memory access. The issue seems to be a bug in determining how much padding is needed for the temporary q8_1 buffer in the MMQ kernel. It's supposed to be as many extra columns as is the maximum tile width J. For dense models this is determined bounded by src1->ne[1]. For MoE models however the tensor layout is different and it is instead bounded by src1->ne[2]. So the MMQ code is using the wrong value which can result in OOB memory accesses if the number of experts is much larger than the physical batch size. And the fix is simply to use the correct value instead.

I decided against adding test cases to test-backend-ops.cpp because the tensors needed for a reproduction are relatively large and the conditions for this defect to manifest as a bug were highly specific.

Requirements

@JohannesGaessler
JohannesGaessler requested a review from a team as a code owner October 4, 2026 09:15
@github-actions github-actions Bot added ggml changes relating to the ggml tensor library for machine learning CUDA Related to the CUDA backend labels Oct 4, 2026
@JohannesGaessler

Copy link
Copy Markdown
Contributor Author

Fixes #29847 . (Seems this is where the reproduction is from which with I was pinged.)

Supersedes #27044 . The fix in that PR should work to resolve the illegal memory access but also overallocate memory.

May be the same issue as #28383 .

@JohannesGaessler
JohannesGaessler merged commit dd26678 into ggml-org:master Oct 4, 2026
15 of 17 checks passed
Wizard815 pushed a commit to Wizard815/mx-llama.cpp-Rocm10 that referenced this pull request Oct 6, 2026
Wizard815 added a commit to Wizard815/mx-llama.cpp-Rocm10 that referenced this pull request Oct 6, 2026
Picking dd26678 brought in upstream's launch block as well as its one-line
size fix, but the fork declares ids_src1/ids_dst/expert_bounds further down, so
the block referenced them before their declarations:

  mmq.cu:251: error: use of undeclared identifier 'ids_src1'

The fork already has its own launch inside the chunked workspace path, so the
inserted copy was both a duplicate and out of order. Remove it. Upstream's
actual change (J_max sized by ne12 rather than ne11) stays.

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

Labels

CUDA Related to the CUDA backend ggml changes relating to the ggml tensor library for machine learning

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants