ggml : address integer overflows in binary ops CUDA implementation - #24706
Conversation
ORippler
left a comment
There was a problem hiding this comment.
Could you share the measured performance numbers?
|
@ORippler performance: Without PR: With PR: Looks like I missed 10% perf degradation for very small tensors caused by size_t() casts, investigating. |
|
@ORippler Added missing size_t casts, now performance is even higher than original: Register usage: unchanged in 74 kernels, lower by 1-2 registers in 15 kernels, higher by 1-4 registers in 64 kernels. Edit: I did a clean recompile of the project and now perf is again a little lower (43.21 GB/s for small matrix, so ~1.2% slower, 2631.88 GB/s for large, so 0.3% slower). Honestly I have no idea what causes these fluctuations. |
The perf benchmarks do not factor out:
which may affect performance and introduce r2r variation. At 1 and 8 us/run this is pretty fast already (typically one cannot go faster than 2-5 us as at these are the minimums pro/epilogue overheads of launching a cuda kernel (constructing & tearing down CTAs etc.) |
|
@ORippler Yeah, I checked clock values of my Max-Q and it was doing pretty wild swings. Locked it to 1410 MHz, increased test time from 1s to 10s and now got this (also added some more tests):
|
ggerganov
left a comment
There was a problem hiding this comment.
Apart from the performance concerns/analysis, I am in favor of pretty much casting all (or most) i32*i32 to 64-bits. It's a rule I follow in the Metal backend as well.
Generally agree. However, doing ALU in 64-bit requires different HW than 32-bit, and thus perf implications depend on HW (for NVGPUs one wants to use 32-bit ALU where possible/reasonable). Anyways for this instance it's a non-issue. E2E perf numbers``` ggml_cuda_init: found 1 CUDA devices (Total VRAM: 97250 MiB): Device 0: NVIDIA RTX PRO 6000 Blackwell Max-Q Workstation Edition, compute capability 12.0, VMM: yes, VRAM: 97250 MiB | model | size | params | backend | ngl | fa | dio | test | t/s | | ------------------------------ | ---------: | ---------: | ---------- | --: | --: | --: | --------------: | -------------------: | | qwen35moe 35B.A3B Q4_K - Medium | 20.60 GiB | 34.66 B | CUDA | -1 | 1 | 1 | pp512 | 7567.30 ± 123.94 | | qwen35moe 35B.A3B Q4_K - Medium | 20.60 GiB | 34.66 B | CUDA | -1 | 1 | 1 | tg128 | 246.50 ± 2.59 | | qwen35moe 35B.A3B NVFP4 | 19.51 GiB | 34.66 B | CUDA | -1 | 1 | 1 | pp512 | 8792.63 ± 24.38 | | qwen35moe 35B.A3B NVFP4 | 19.51 GiB | 34.66 B | CUDA | -1 | 1 | 1 | tg128 | 215.35 ± 2.16 | | gemma4 26B.A4B NVFP4 | 16.45 GiB | 25.23 B | CUDA | -1 | 1 | 1 | pp512 | 10716.34 ± 188.49 | | gemma4 26B.A4B NVFP4 | 16.45 GiB | 25.23 B | CUDA | -1 | 1 | 1 | tg128 | 168.72 ± 0.52 | | gpt-oss 20B MXFP4 MoE | 11.27 GiB | 20.91 B | CUDA | -1 | 1 | 1 | pp512 | 17384.51 ± 124.35 | | gpt-oss 20B MXFP4 MoE | 11.27 GiB | 20.91 B | CUDA | -1 | 1 | 1 | tg128 | 373.90 ± 1.48 | | gemma4 26B.A4B Q4_K - Medium | 15.90 GiB | 25.23 B | CUDA | -1 | 1 | 1 | pp512 | 9645.72 ± 223.84 | | gemma4 26B.A4B Q4_K - Medium | 15.90 GiB | 25.23 B | CUDA | -1 | 1 | 1 | tg128 | 214.47 ± 0.89 |build: bf95565 (9674)
|
ORippler
left a comment
There was a problem hiding this comment.
Thanks for bearing with the multi-review-rounds
…gml-org#24706) * ggml : address integer overflows in binary ops CUDA implementation * ggml : add size_t casts to avoid integer overflows * ggml : add more asserts checking integer overflows in binary ops CUDA implementation --------- Co-authored-by: Stanisław Szymczyk <sszymczy@gmail.com>
…gml-org#24706) * ggml : address integer overflows in binary ops CUDA implementation * ggml : add size_t casts to avoid integer overflows * ggml : add more asserts checking integer overflows in binary ops CUDA implementation --------- Co-authored-by: Stanisław Szymczyk <sszymczy@gmail.com>
…gml-org#24706) * ggml : address integer overflows in binary ops CUDA implementation * ggml : add size_t casts to avoid integer overflows * ggml : add more asserts checking integer overflows in binary ops CUDA implementation --------- Co-authored-by: Stanisław Szymczyk <sszymczy@gmail.com> (cherry picked from commit f728ada)
Overview
This PR addresses the problem of integer overflows when processing large tensors with GGML binary OPs in CUDA backend. It's not a major overhaul, but rather a least-intrusive way of improving the current implementation.
Fixes #24643
Additional information
The problem was addressed by:
inttype usage touint32_tto double usable integer range,uint32_twon't overflow,uint32_tparameters (and their products inside the kernels) will stay insideuint32_ttype range,i0variable doesn't overflow when incremented in loop iterations,I ran
./bin/test-backend-ops(all tests passed), ran failing tests from #24643 (passed) and compared performance of example small and large tensor f32 addition in old and new implementations (was the samenew impl after adding size_t casts is slightly slower, ~1.2% for small tensors, ~0.3% for large ones).Additionally I compared kernel register usage in old and new implementation.
In most kernels (126) register usage was the same, in 17 kernels it was lower by 1-2 registers and in 10 kernels it was higher by 1-2 registers. With addedsize_tcasts: unchanged in 74 kernels, lower by 1-2 registers in 15 kernels, higher by 1-4 registers in 64 kernels.Requirements