Repository navigation
fix(cuda): refuse tensors exceeding 2^31 elements to prevent silent miscomputation - #31
srivilliamsai wants to merge 5 commits into
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6563e0aa70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| fn zeros_impl(&self, shape: &Shape, dtype: DType) -> Result<CudaStorage> { | ||
| let elem_count = shape.elem_count(); | ||
| check_elem_count(elem_count)?; |
There was a problem hiding this comment.
On 64-bit release builds, Shape::elem_count() uses unchecked multiplication, so a CUDA shape such as (1usize << 32, 1usize << 32) wraps to 0 before this guard; Tensor::zeros then passes the limit check and requests a zero-length backing allocation for a logical 2^64-element tensor instead of returning TooManyElements. The rand_normal path likewise checks only after its odd-count + 1 can wrap. Check shape multiplication and cuRAND rounding with checked arithmetic before comparing to the cap.
AGENTS.md reference: AGENTS.md:L74-L77
Useful? React with 👍 / 👎.
|
Addressed in commit
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c7bf58f0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.device().is_cuda() { | ||
| crate::cuda::check_shape_elem_count(&shape)?; |
There was a problem hiding this comment.
If a CPU tensor is broadcast to a shape with more than 2^31 elements and then moved to CUDA, this condition is false, so the check is skipped. Tensor::to_device copies only the backing storage and preserves the broadcast layout, allowing the oversized logical tensor to reach CUDA kernels and miscompute; validate the layout when transferring to CUDA as well.
AGENTS.md reference: AGENTS.md:L74-L77
Useful? React with 👍 / 👎.
|
Addressed in commit eb22324: • Added CUDA shape validation in if device.is_cuda() {
crate::cuda::check_shape_elem_count(self.shape())?;
}This prevents broadcasted CPU tensors with > 2^31 logical elements from transferring their broadcast layout onto CUDA devices. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb22324cd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut count: usize = 1; | ||
| for &d in shape.dims() { | ||
| match count.checked_mul(d) { | ||
| Some(c) if c <= MAX_CUDA_ELEMENTS => count = c, |
There was a problem hiding this comment.
For a shape such as (MAX_CUDA_ELEMENTS + 1, 0), this branch returns TooManyElements before reaching the zero dimension, even though the complete shape has zero elements; the reversed shape (0, MAX_CUDA_ELEMENTS + 1) passes. Thus creating or transferring an empty tensor to CUDA depends on axis order. Check for a zero dimension before enforcing the product cap.
AGENTS.md reference: AGENTS.md:L74-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 75f7661:
Checked for zero dimensions upfront before enforcing the product cap:
if shape.dims().contains(&0) {
return Ok(0);
}Both trailing and leading zero shapes (such as (MAX_CUDA_ELEMENTS + 1, 0) and (0, MAX_CUDA_ELEMENTS + 1)) now consistently return Ok(0) regardless of axis order. Added unit test test_cuda_empty_shapes_with_large_dim covering these cases.
|
Addressed discussion on preserving empty CUDA shapes in commit 75f7661: • if shape.dims().contains(&0) {
return Ok(0);
}This guarantees that empty shapes (such as @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75f76617aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .bt()); | ||
| } | ||
| if self.device().is_cuda() { | ||
| crate::cuda::check_shape_elem_count(&shape)?; |
There was a problem hiding this comment.
With CUDA available in a default debug/test build, reshaping a zero-element CUDA tensor to (1usize << 32, 1usize << 32) panics inside ShapeWithOneHole::into_shape when it calls Shape::elem_count() before this guard runs. That makes the new test_cuda_refuses_reshape_overflow fail on GPU test runners instead of returning TooManyElements; validate the requested dimensions with checked arithmetic before into_shape or elem_count.
AGENTS.md reference: AGENTS.md:L74-L77
Useful? React with 👍 / 👎.
Summary
Per
LLM.md("Past 2^31 elements on CUDA"):CUDA kernel launches and internal indexing variables rely on signed 32-bit integers ($2^{31} - 1 = 2{,}147{,}483{,}647$ ), integer overflow produces negative offsets or incorrect wraparounds, leading to silent calculation corruptions with zero hardware errors.
int/i32). When total element counts exceedi32::MAX(Changes
CudaError::TooManyElements { elem_count: usize, max_elements: usize }.pub const MAX_CUDA_ELEMENTS: usize = i32::MAX as usize;andcheck_elem_count(len: usize) -> Result<()>incuda_backendanddummy_cuda_backend.CudaDevice::alloc<T>CudaDevice::alloc_zeros<T>CudaDevice::clone_htodCudaDevice::zeros_implCudaDevice::rand_uniformCudaDevice::rand_normalCudaDevice::alloc_uninitCudaDevice::storage_from_sliceTensor::broadcast_as(when target device is CUDA)hanzo-ml/tests/cuda_limit_tests.rscovering:MAX_CUDA_ELEMENTS == i32::MAX as usizelimit + 1LLM.mdhanzo-ml:0.11.114->0.11.115hanzo-nn: updatedhanzo-mldependency to0.11.115hanzo-transformers: updatedhanzo-mldependency to0.11.115Validation
cargo fmt --all -- --check(clean)cargo clippy -p hanzo-ml -p hanzo-nn -p hanzo-transformers --tests -- -D warnings(clean, 0 warnings)cargo test -p hanzo-ml --test cuda_limit_tests(2 passed)cargo test -p hanzo-ml --lib(76 passed)cargo test -p hanzo-nn -p hanzo-transformers --lib(48 passed)