fix(cuda): widen byte-size products on the weight-upload path (#432) - #433
Open
jamesburton wants to merge 1 commit into
Open
fix(cuda): widen byte-size products on the weight-upload path (#432)#433jamesburton wants to merge 1 commit into
jamesburton wants to merge 1 commit into
Conversation
Found while auditing the other backends for the #429 bug class. The native kernels are already correct — every weight-row base in native/kernels/ uses `(size_t)row * blocks_per_row * BYTES`, and the widening was confirmed to survive into the checked-in PTX. These three are managed-side. CudaKernels.cs LaunchDequantToF16: `(nuint)(totalElements * 2)` is int*int and wraps negative above 1,073,741,823 elements (a 2 GiB F16 tensor), after which `(nuint)` sign-extends it to a ~16 EiB byte count for cuMemcpyDtoD_v2. The Llama 3.1 405B LM head (128256 x 16384 = 2,101,346,304) wraps; the 70B head (1,050,673,152) clears it by only 2.2%. CudaWeights.cs UploadAndDequant: `outputDim * inputDim` wraps at 2^31 elements. Made `checked` rather than widened, deliberately — every byte size derived from it already casts to long, but the count is passed as `int` to LaunchConvertF32ToF16 / LaunchDequantToF16, so a long value could not be honoured without changing those signatures. Supporting >2^31-element tensors is a larger piece of work; this turns silent corruption into a clear failure. CudaTransformerModel.cs: `(seqLen - 1) * hiddenSize * h` is an activation offset and self-limiting, so consistency only — sibling lines in the same file already cast (long), and the inconsistency invites the wrong conclusion about which form is correct. Not verified on hardware: no NVIDIA GPU available here, and the overflowing scale is out of reach of the 12/16 GB cards I can borrow. The changes are type-level and the arithmetic is in the issue.
There was a problem hiding this comment.
Pull request overview
Fixes managed-side integer-overflow hazards on the CUDA weight upload/dequantization path by widening byte-size computations and ensuring element-count products fail loudly instead of wrapping, aligning CUDA behavior with the previously addressed #429 bug class.
Changes:
- Widened
totalElements * 2tolongbefore converting tonuintfor device-to-device FP16 copies. - Added
checkedto theoutputDim * inputDimelement-count computation during upload/dequant to prevent silent wrap. - Widened an activation-buffer byte-offset computation for consistency with neighboring code.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/DotLLM.Cuda/CudaKernels.cs | Widened byte-count arithmetic for FP16 passthrough copies in LaunchDequantToF16. |
| src/DotLLM.Cuda/CudaWeights.cs | Made element-count computation overflow-checked during upload/dequant to avoid silent wrap. |
| src/DotLLM.Cuda/CudaTransformerModel.cs | Widened a hidden-state pointer offset computation to avoid int overflow and match sibling patterns. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+245
to
+249
| // `checked` rather than widened: every byte size below already casts to long, so this | ||
| // element count is the only narrow product — but it is also passed as `int` to | ||
| // LaunchConvertF32ToF16 / LaunchDequantToF16, so a long count could not be honoured | ||
| // without changing those signatures. Fail loudly instead of wrapping negative (#429). | ||
| int totalElements = checked(outputDim * inputDim); |
| case QuantizationType.F16: | ||
| // Already FP16, just copy | ||
| CudaDriverApi.cuMemcpyDtoD_v2(dst, src, (nuint)(totalElements * 2)).ThrowOnError(); | ||
| CudaDriverApi.cuMemcpyDtoD_v2(dst, src, (nuint)((long)totalElements * 2)).ThrowOnError(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #432.
Three managed-side sites on the CUDA weight-upload/dequant path, found while auditing the other backends for the #429 bug class.
CudaKernels.cs:607(nuint)(totalElements * 2)(long)totalElements * 2CudaWeights.cs:245outputDim * inputDimchecked(...)CudaTransformerModel.cs:290(seqLen - 1) * hiddenSize * h(long)Why site 2 is
checkedrather than widenedThis is the one judgement call in the PR, so I want it visible rather than buried. Every byte size derived from
totalElementsalready casts tolong((long)totalElements * sizeof(ushort)etc.), leaving the element count as the sole narrow product — so widening it looks like the obvious fix. But it is also passed asinttoLaunchConvertF32ToF16andLaunchDequantToF16, so alongcount cannot actually be honoured without changing those signatures, and genuinely supporting >2^31-element tensors is a larger piece of work than this PR should take on.checkedconverts silent negative wrap into a clear failure at the real boundary. Happy to do the signature change instead if you would rather have it.The native kernels needed nothing
Worth stating explicitly since it is the bulk of the surface: all 63 weight-row bases across 57
.cufiles are already(size_t)row * blocks_per_row * BYTES. I also checked that the widening survives into the checked-innative/ptx/*.ptx(mul.wide.s32present at those sites) — a widened.cupaired with stale committed PTX would still ship the bug, so the.cualone is not sufficient evidence.Also ruled out, with arithmetic in #432: MoE expert offsets (already
long— DeepSeek-V3's 256 × 8.26 MB = 2.1 GB would otherwise have wrapped), KV-cache offsets, dequant block indices, attention strides, and theCudaDriverApiP/Invoke boundary (allnuint/nint, noint-vs-size_ttruncation).Not verified on hardware
I have no NVIDIA GPU — my dev box is AMD. These are unexercised at the overflowing scale, and site 1 needs a >2 GiB F16 tensor, out of reach of the 12 GB and 16 GB cards I can borrow. The changes are type-level and the arithmetic is shown in the issue, but no CUDA kernel was executed.
dotnet build src/DotLLM.Cuda/DotLLM.Cuda.csprojand the full solution build clean.A regression test would have to fake
totalElementsto reach the boundary, which tests the cast rather than the behaviour; I left it out rather than add a test that only appears to cover the case. Say if you would prefer one anyway.Related
#432 also records a finding I am not proposing to change:
longis 32-bit in the device code this repo ships, becausenative/build.ps1drives nvcc on Windows (MSVC LLP64). ~44 sites innative/kernels/use barelongas though it widened. I bounded all of them and none is reachable, so no change is warranted — but the same source on Linux would widen, so a kernel can be correct upstream and silently 32-bit in the shipped PTX. Worth knowing before the next(long)goes into a.cu.