Skip to content

Fix grid_dim_x calculations - #2980

Merged
awni merged 1 commit into
ml-explore:mainfrom
CC-Yeh:fix_affine_cuda_kernel
Jan 12, 2026
Merged

Fix grid_dim_x calculations#2980
awni merged 1 commit into
ml-explore:mainfrom
CC-Yeh:fix_affine_cuda_kernel

Conversation

@CC-Yeh

@CC-Yeh CC-Yeh commented Jan 9, 2026

Copy link
Copy Markdown
Contributor

Proposed changes

Fix grid_dim_x calculations which only shows error when launching 2D grids.

Checklist

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

@awni

awni commented Jan 10, 2026

Copy link
Copy Markdown
Member

Looks good but is there a test case for this? Or do we only launch 2D grids for very large inputs (which we can't really test in CI)?

@CC-Yeh

CC-Yeh commented Jan 10, 2026

Copy link
Copy Markdown
Contributor Author

Looks good but is there a test case for this? Or do we only launch 2D grids for very large inputs (which we can't really test in CI)?

Yeah, I think it would be too big.

bool large = w.size() > UINT_MAX;
------
std::tuple<dim3, uint> get_launch_args(
    size_t size,
    const Shape& shape,
    const Strides& strides,
    bool large,
    int work_per_thread /* = 1 */,
    uint max_block_dim /* = 1024 */) {
  size_t nthreads = cuda::ceil_div(size, work_per_thread);
  uint block_dim = max_block_dim < nthreads ? max_block_dim : nthreads;
  dim3 num_blocks;
  if (large) {
    num_blocks = get_2d_grid_dims(shape, strides, work_per_thread);
    num_blocks.x = cuda::ceil_div(num_blocks.x, block_dim);
  } else {
    num_blocks.x = cuda::ceil_div(nthreads, block_dim);
  }
  return std::make_tuple(num_blocks, block_dim);
}

@awni

awni commented Jan 10, 2026

Copy link
Copy Markdown
Member

@nastya236 do you mind double checking this? I thought you changed it recently to fix a bug I had written but maybe the change was also not exact?

@nastya236

nastya236 commented Jan 10, 2026

Copy link
Copy Markdown
Collaborator

Thanks @CC-Yeh! @awni you remember correctly, but I fixed only in fp_quanitze / fp_dequantize. Also @CC-Yeh qqmm_utils.cu would be fixed by this PR.

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

Ty!

@awni

awni commented Jan 11, 2026

Copy link
Copy Markdown
Member

Do you mind rebasing to resolve the conflict, then we can run tests + merge?

@CC-Yeh
CC-Yeh force-pushed the fix_affine_cuda_kernel branch from bfb7afe to 9ecb97a Compare January 11, 2026 20:09
@CC-Yeh

CC-Yeh commented Jan 11, 2026

Copy link
Copy Markdown
Contributor Author

Do you mind rebasing to resolve the conflict, then we can run tests + merge?

Done!

@CC-Yeh
CC-Yeh requested a review from awni January 11, 2026 20:10
@awni
awni merged commit 46ee0e9 into ml-explore:main Jan 12, 2026
15 checks passed
jzdziarski pushed a commit to jzdziarski/mlx that referenced this pull request Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants