Repository navigation
AISPARSE-687: clamp grid.x in the dense conversion paths and add grid-stride loops - #11830
Merged
kliegeois merged 16 commits intoOct 8, 2026
Merged
Conversation
The dense conversion launches narrow a 64-bit dimension into dim3 without clamping against handle->properties.maxGridSize[0], and none of the kernels loops, so an undersized grid silently leaves trailing rows or columns unconverted. dense2bell is the reachable case: it launches one block per block-row, so the divisor is one and the grid overflows for mb above 2147483647. Clamp every grid against maxGridSize[0], add a grid-stride loop over block rows, rows or columns to dense2bell_nnz_kernel, dense2bell_fill_kernel, dense2csr_kernel, dense2csc_kernel, csr2dense_kernel and csc2dense_kernel, and drop the intermediate rocsparse_int in rocsparse_csx2dense.hpp so the width is preserved end to end. Bring rocsparse_coo2dense.cpp in line with its already clamped AoS sibling, and compute the grid-stride in int64_t in both coo2dense kernels. JIRA ID: AISPARSE-687 Co-authored-by: Cursor <cursoragent@cursor.com>
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
kliegeois
changed the title
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (74.63%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #11830 +/- ##
===========================================
- Coverage 71.94% 69.69% -2.25%
===========================================
Files 2866 2934 +68
Lines 473102 482508 +9406
Branches 69892 69491 -401
===========================================
- Hits 340347 336239 -4108
- Misses 109153 122038 +12885
- Partials 23602 24231 +629
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
jsandham
reviewed
Sep 15, 2026
AISPARSE-687: address review feedback by turning testing_dense_to_sparse_bell_extra_687 into a template parameterised on the index type I and the value type T, and having the non-template testing_dense_to_sparse_bell_extra wrapper instantiate it at <int32_t, float>. The hardcoded "using I = int32_t; using T = float;" aliases in the function body are dropped. No behavioural change. Co-authored-by: Cursor <cursoragent@cursor.com>
jsandham
approved these changes
Sep 18, 2026
jsandham
reviewed
Sep 25, 2026
jsandham
approved these changes
Sep 25, 2026
kliegeois
commented
Sep 29, 2026
Size m from min(maxGridSize[0], (2^32 - 1) / 256) + 1 so the test runs in about 320 MB, and pass get_grid_size_x straight to dim3 instead of through num_blocks_x.
ntrost57
reviewed
Oct 5, 2026
ntrost57
reviewed
Oct 5, 2026
ntrost57
reviewed
Oct 5, 2026
ntrost57
reviewed
Oct 5, 2026
Contributor
|
changelog entry missing |
ntrost57
reviewed
Oct 5, 2026
…-687) rocsparse_dense_to_sparse to CSR, CSC and COO counts the non-zeros with nnz_kernel_row or nnz_kernel_col before converting. nnz_kernel_col launched one 256-thread block per column and nnz_kernel_row one block per 256 rows, both unclamped and without a grid-stride loop, so a large enough matrix went past the 32-bit dispatch limit. Clamp both launches with rocsparse::get_grid_size_x and select the kernel variant through rocsparse::dispatch_grid_stride_x (identical to the AISPARSE-677 helper): the grid-stride variant runs only when the clamp binds, and the unclamped launch keeps the straight-line body. Add forced-clamp unit tests that drive rocsparse_dense_to_sparse (CSR and CSC, both dense orders) and rocsparse_scoo2dense with maxGridSize[0] shrunk to 1, 3 and 7 blocks.
…2dense counts (AISPARSE-687) coo2dense_aos clamped grid.x to maxGridSize[0] only, which still lets grid.x * 512 pass 2^32 work-items. Use rocsparse::get_grid_size_x like the SoA variant. coo2dense formed m * n in the index type for the null check on A and called valset_2d with the index type, so with 32-bit indices the zero fill bound overflowed once m * n passed 2^31. Form the count in 64 bits and call valset_2d with 64-bit extents, as coo2dense_aos already does.
…(AISPARSE-687) Give block-row 0 an occupied slot so that block 0 enters the trailing block-row with s_slot == 1 unless the fill kernel resets it, and make the trailing block-row the only one with two slots so ell_cols depends on the nnz kernel reaching it. Zero the analysis buffer so an unvisited block-row counts as empty, return early if ell_cols is wrong since every buffer is sized from it, and allocate one spare block-row so a stale slot cannot write past the arrays.
…AISPARSE-687) The non-strided nnz_kernel_row formed the row tile index in the index type J, so bid * DIM_X * 4 could wrap negative for int32 m. Pass the block index as int64_t, as the grid-stride branch already does. Size the row grid as (m - 1) / (NNZ_DIM_X * 4) + 1 for every value type. Each block covers NNZ_DIM_X * 4 rows regardless of T, so the double complex case launched 4x the blocks it needed; the surplus blocks did no work but were the ones that reached the wrap.
…E-687) csx2dense formed m * n in the index type for the null check on A and called valset_2d with offset-typed extents, so with 32-bit offsets the zero fill bound overflowed once m * n passed 2^31. Form the count in 64 bits and call valset_2d with 64-bit extents, as coo2dense does.
…-687) Attach the Blocked ELL fix to rocsparse_dense_to_sparse, which is where dense2bell is reached, and limit the zero fill claim to what this change covers: dense matrices with 2^31 to 2^32 - 1 elements and 32-bit indices.
ntrost57
reviewed
Oct 7, 2026
ntrost57
approved these changes
Oct 7, 2026
…SE-687) get_grid_size_x returns uint32_t and dim3 takes 32-bit dimensions, so holding the clamped block count in int64_t only widened it to narrow it again.
…lamp # Conflicts: # projects/rocsparse/CHANGELOG.md
Contributor
TheRock Submodule Bump ActivityNewest first
|
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.
JIRA ID: AISPARSE-687
What
Clamps
grid.xagainsthandle->properties.maxGridSize[0]in the dense conversion launches and adds a grid-stride loop to every kernel behind them, so a clamped grid still covers the whole matrix.Library changes, all under
library/src/conversion/:rocsparse_dense2bell.cpp:66dense2bell_nnz_kernelrocsparse_dense2bell.cpp:169dense2bell_fill_kernelrocsparse_dense2csx.hpp:62,:83dense2csr_kernelrocsparse_dense2csx.hpp:110,:131dense2csc_kernelrocsparse_csx2dense.hpp:60,:84csr2dense_kernelrocsparse_csx2dense.hpp:114,:138csc2dense_kernelBoth
dense2bellkernels now takembso they know the block-row bound;dense2bell_fill_kernelresets its shareds_slotaccumulator per block-row and barriers before the reset so the stride loop is reentrant.rocsparse_csx2dense.hppalso computed the block count into arocsparse_intlocal before handing it todim3, truncating one step earlier than the other files on a 64-bit index instantiation. That intermediate is gone, so the width isint64_tend to end.All new ceiling divisions use the overflow-safe
(count - 1) / block_size + 1form.dense2bell.h:121-128(dense2bell_max_nnzb_part1_kernel) already had a fixed grid with a correct stride and is untouched.dense2bell_nnz_kernelalready looped over columns internally; only its block-row dimension needed work.Why
Only
dim3-narrowing plus a missing loop, so an undersized grid does not fault — it silently leaves trailing rows or columns unconverted.The two halves of this are very far apart in reachability, and the P2 rests entirely on the BELL half:
dense2bellis reachable on current hardware. It launches one thread block per block-row, so the divisor is 1 and the grid overflows formbabove 2,147,483,647. With an ELL block size of 1 that ismabove 2,147,483,648, roughly 17 GB of row pointers, which fits on a single MI300X several times over. After this change the block-row count is bounded only byint64_t.dense2csxandcsx2denseare not reachable on current hardware. They tile 16 rows or columns per block, so they only overflow abovem(orn) of 34,359,738,368 — about 275 GB of row pointers. Fixed here for consistency and to keep the whole family auditable, not because it is currently exploitable.The reachable entry points are
rocsparse_dense_to_sparseandrocsparse_sparse_to_densewithrocsparse_indextype_i64descriptors, and for BELLrocsparse_dense_to_sparseregardless of the build, sincemat_A->rowsisint64_tthere. The legacyrocsparse_Xdense2csr/rocsparse_Xcsr2denseentry points takerocsparse_intand cannot reach either threshold.rocsparse_coo2dense.cppJIRA flagged bringing the non-AoS
coo2densein line with its already-clamped AoS sibling as a reasonable addition if cheap. It was, so it is included: the launch now clamps andcoo2dense_kernelgrid-strides, mirroringcoo2dense_aos_kernel.While doing that, both
coo2densekernels now compute the stride inint64_t. The AoS kernel usedconst auto NUM_THREADS = hipGridDim_x * BLOCKSIZE, which isuint32_t * uint32_t: atnnz >= 2^32the product wraps to 0 and the grid-stride loop never advances. That is a hang, not a wrong answer, and it is reachable on a 192 GB part, so it is fixed here rather than left in the file being used as the reference pattern.Testing
New
dense_to_sparse_bell_extranightly test (testing_dense_to_sparse_bell_extra_687), registered intest_dense_to_sparse_bell.yamlwithdevice_memory_gb: 44. It drivesrocsparse_dense_to_sparsewithell_block_size = 1andm = maxGridSize[0] + 1, so the block-row count exceeds the grid limit by exactly one block-row.The test asserts values, not just launch success. The dense matrix is zero apart from its very last row, so the converted matrix has one occupied ELL slot, and that slot lives in the trailing block-row that an unclamped or non-striding grid does not reach. It checks
ell_cols, and the column index and value of both the leading (padded) and the trailing (occupied) block-row. The column-index array is pre-filled with a poison pattern so an unvisited block-row cannot be mistaken for either an occupied or a padded slot.On a 15 GB gfx1201 the test auto-skips with
SKIPPING TEST: Insufficient memory / Required device: 44 GB (available: 15 GB), which is the suite's memory guard behaving correctly.Regression coverage for the ordinary-sized path, on gfx1201:
7239 testspass across*dense_to_sparse*,*sparse_to_dense*,*dense2csr*,*dense2csc*,*csr2dense*,*csc2dense*,*coo2dense*,*dense2coo*,*ell2dense*,*prune_dense2csr*,*sparse_to_sparse*,*spmm_bell*.3, forcing all six kernels to cover their matrices with a 3-block grid.5696 testsfrom the same filter pass in that configuration, including value comparisons against the host references. Reverted before commit.