Skip to content

ggml: support non-contiguous SUM (CPU, CUDA) - #28851

Open
rssndr wants to merge 3 commits into
ggml-org:masterfrom
rssndr:cuda-sum-noncont
Open

rssndr wants to merge 3 commits into
ggml-org:masterfrom
rssndr:cuda-sum-noncont

Conversation

@rssndr

@rssndr rssndr commented Sep 13, 2026 •

Copy link
Copy Markdown

Overview

test-backend-ops includes SUM(type=f32,ne=[33,256,1,1],permute=[1,0,2,3]), which CUDA rejects and which crashes the CPU reference if enabled. Two small changes:

  • CUDA: supports_op required ggml_is_contiguous_rows, but the kernel only requires ggml_is_contiguously_allocated (asserted in ggml_cuda_op_sum since llama/ggml: add LLM training support #10544). The flat CUB reduction is valid for any gap-free view, since summation is order-independent. The contiguous_rows check was added in metal: optimise GGML_OP_SUM #16559 to keep CI green after that PR introduced permuted SUM test cases, it wasn't based on what the CUDA kernel can actually handle. This change makes supports_op match the condition the kernel itself asserts. The kernel is unchanged.
  • CPU: ggml_compute_forward_sum_f32 asserted nb[0] == sizeof(float), so it aborted on non-contiguous rows, reached as soon as the CPU acts as reference (or fallback) for this case. Added a strided path keeping ggml_float accumulation; contiguous fast path unchanged. f16/bf16 variants left as-is.

Why CPU and CUDA together : the CPU fix is required for the CUDA case to be testable, since the reference crashes once the case is enabled.

Additional information

Testing (RTX 3070 Ti Laptop, CUDA 13.3):

  • test-backend-ops -o SUM: 8/8, permuted case now passing
  • Full test-backend-ops: 16094/16094
  • perf -o SUM: unchanged

Ref #14909, #16559.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES. Used Claude for analysis help, code drafting for the CPU change, and the initial description draft. Reviewed, built, and tested locally.

@rssndr
rssndr requested review from a team and ggerganov as code owners September 13, 2026 14:58
@github-actions github-actions Bot added ggml changes relating to the ggml tensor library for machine learning CUDA Related to the CUDA backend labels Sep 13, 2026
@ggml-gh-bot

ggml-gh-bot Bot commented Sep 13, 2026

Copy link
Copy Markdown

Hi @4ndrearossetti, thanks for your contribution!

Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:

  • PR Template not respected: Please respect the template when creating a new pull request. Make sure to fill out all required sections.

  • Multiple backend changes in one PR: When adding support for a new model or feature, focus on CPU support only in the initial PR. Add support for other backends like CUDA in follow-up PRs. If you have a good reason to modify multiple backends in one PR, please explain it.

  • AI-generated content: While code is allowed to be generated by AI, please write the PR description and commit messages on your own without the help of AI.


Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below.

@ggml-gh-bot ggml-gh-bot Bot added the draft PR will be changed to draft by github-actions bot label Sep 13, 2026
@github-actions
github-actions Bot marked this pull request as draft September 13, 2026 15:03
@github-actions github-actions Bot removed the draft PR will be changed to draft by github-actions bot label Sep 13, 2026
@rssndr
rssndr marked this pull request as ready for review September 13, 2026 15:20
@rssndr

rssndr commented Oct 8, 2026

Copy link
Copy Markdown
Author

Hello,
I checked the three failures, here a summary:

  • build-cmake-pkg / linux: it's a runner config error ("jobs without a container are forbidden"), so it failed at setup, the code was never checked.
  • gpu-cuda: self-hosted runner lost communication with GitHub ~1h53m in; all tests up to that point, SUM included, were passing.
  • gpu-openvino-low-perf: OpenVINO crashed with CL_OUT_OF_RESOURCES on the weak Intel-GPU runner. My code doesn't touch the OpenVINO backend.

So, it seems it's three infrastructure-related faillures, rather than anything from my code.
I remain available if there's anything more to change or check.

Thank you very much!

@JohannesGaessler JohannesGaessler 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.

I just realized: please add a corresponding test case to test-backend-ops.cpp as well.

@rssndr
rssndr force-pushed the cuda-sum-noncont branch 2 times, most recently from c18aa07 to 0458c7c Compare October 8, 2026 14:22
@github-actions github-actions Bot added the testing Everything test related label Oct 8, 2026
@rssndr

rssndr commented Oct 8, 2026

Copy link
Copy Markdown
Author

Hello,
just added two tests, as asked. I also rebased on master in the meantime, since quite some time passed since my last commits.

@JohannesGaessler

Copy link
Copy Markdown
Contributor

A permutation is not enough for the tests. Please add the option to calculate a sum over a non-contiguous view.

@rssndr

rssndr commented Oct 8, 2026

Copy link
Copy Markdown
Author

Hello Johannes,

That's right. I added the slice option following the test_sum_rows pattern. The sliced cases run on CPU and report unsupported on CUDA since such views aren't allocated contiguously. And, the slice + row-breaking-permute case skips grad because the backward pass goes through ACC, whose CPU implementation requires contiguous rows.

That should be all. Let me know if I missed anything. Thank you!

This branch has not been deployed

No deployments
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 testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants