Skip to content

CUDA: Various fixes to cpy.cu - #25000

Merged
ORippler merged 6 commits into
ggml-org:masterfrom
ORippler:osimons/port_cpy_bugfix_sdiffcpp
Jun 25, 2026
Merged

CUDA: Various fixes to cpy.cu#25000
ORippler merged 6 commits into
ggml-org:masterfrom
ORippler:osimons/port_cpy_bugfix_sdiffcpp

Conversation

@ORippler

Copy link
Copy Markdown
Collaborator

Overview

Closes #24072 and does some more cleanup to the copy functions while going at it

Additional information

Concrete changes

  1. Fixes Misc. bug: cpy.cu:229: GGML_ASSERT(grid_z < USHRT_MAX) failed #24072 by falling back to default kernel if grid-dim constraints are not met
  2. Adds test-cases for y/z > UHSRT_MAX to test fall-through of transposed kernel to default kernel
  3. Adjusts constraints for x/y/z gridDim to <=INT_MAX/<=USHRT_MAX/<=USHRT_MAX, so they actually match cuda constraints, see https://docs.nvidia.com/cuda/cuda-programming-guide/05-appendices/compute-capabilities.html#compute-capabilities-table-device-and-streaming-multiprocessor-sm-information-per-compute-capability
  4. Fix potential int overflow in transposed copy kernel

Requirements

@ORippler
ORippler requested review from a team and ggerganov as code owners June 25, 2026 10:11
@github-actions github-actions Bot added testing Everything test related ggml changes relating to the ggml tensor library for machine learning CUDA Related to the CUDA backend labels Jun 25, 2026

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

Congrats on 25000!

@askmyteapot

Copy link
Copy Markdown

Just built and merged it into koboldcpp to test.
This updated cpy.cu prevents the assert. Its slightly slower than vulkan for the same settings (32s/it vs 29s/it), but the fact that its not crashing is a win.

@ORippler

Copy link
Copy Markdown
Collaborator Author

Its slightly slower than vulkan for the same settings (32s/it vs 29s/it), but the fact that its not crashing is a win.

Is this E2E comparison between CUDA/VULKAN? Or isolated to the changes of this PR

@askmyteapot

askmyteapot commented Jun 25, 2026

Copy link
Copy Markdown

It's cuda with this PR vs Vulkan.

The test settings are
1216 x 832 x 145 frames

Previously, the Cuda path would assert, but the vulkan path was fine. No changes to the speed of vulkan after this PR (which is expected)

The s/IT metrics are taken during the generation. (this gen takes about 10 mins to complete)

@ORippler

ORippler commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

It's cuda with this PR vs Vulkan.

If you have an nsys report I could try to take a look, but current CUDA perf priorities are still for LLM inference. Though we are looking to improve PP perf a bit, and that should align with image/video gen workloads from the characteristics

@ORippler

Copy link
Copy Markdown
Collaborator Author

Happy to hear it's no longer crashing!

@askmyteapot

Copy link
Copy Markdown

I think further speed improvements need to come from stablediffusion.cpp.

But for the time being, this is functional :)

@ggerganov ggerganov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving in case you want to merge @ORippler, but I haven't tested or reviewed the code.

@ORippler
ORippler merged commit 1ec44d1 into ggml-org:master Jun 25, 2026
27 of 29 checks passed
@ORippler
ORippler deleted the osimons/port_cpy_bugfix_sdiffcpp branch June 25, 2026 15:29
@CMay

CMay commented Jun 25, 2026

Copy link
Copy Markdown

Thanks for looking into this and grats on 25000! Stable-diffusion.cpp maintainers should be aware of this fix now. I understand they run an older version of GGML, but with any luck should be able to incorporate these changes without issue.

😊👍

papamoose pushed a commit to papamoose/llama.cpp that referenced this pull request Jun 27, 2026
* Add failing test-case to test-backend-ops

Extracted from ggml-org#24072

* Minimize repro with help of AI

N = 8 * (65535 - 1) + 1 = 524273

* Port and adjust workaround from LostRuins@0ba7983

Fall-back should share code, also relax y-z constraint to be inclusive

* Add test-case + fallback also for y dim

* Fix x-guards which is 2^{31}-1, so inlusive of INT_MAX

* Fix overflow problems for transposed copy kernel
adrianhoehne pushed a commit to adrianhoehne/llama.cpp that referenced this pull request Jul 5, 2026
* Add failing test-case to test-backend-ops

Extracted from ggml-org#24072

* Minimize repro with help of AI

N = 8 * (65535 - 1) + 1 = 524273

* Port and adjust workaround from LostRuins@0ba7983

Fall-back should share code, also relax y-z constraint to be inclusive

* Add test-case + fallback also for y dim

* Fix x-guards which is 2^{31}-1, so inlusive of INT_MAX

* Fix overflow problems for transposed copy kernel
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.

Misc. bug: cpy.cu:229: GGML_ASSERT(grid_z < USHRT_MAX) failed

5 participants