Repository navigation
fix(rocsparse): write the base and use n for csxsldu_preprocess column walks (AISPARSE-906) - #14480
Conversation
…n walks (AISPARSE-906) rocsparse::csxsldu_preprocess_template writes the index base into element 0 of the L and U pointer arrays before the count kernel, which only writes ptr[seq + 1], and the inclusive scan. That write sat in the row-direction case only. In the column direction element 0 kept whatever the caller had left in the arrays, so the scan offset every pointer and the returned lnnz / unnz by that value. The counts were also read back at ptr[m] in both directions. The column walk has n + 1 pointers, so for m < n the counts covered only the first m columns, and for m > n the read went past the last pointer. The base write and the stream synchronization now happen before the direction switch, so both directions start the scan from the base, and the counts are read at ptr[m] for the row walk and ptr[n] for the column walk. The status of the count kernel dispatch, which was dropped, is now returned, so two non-unit diagonals give invalid_value instead of a scan of stale counts. Test: unit_test_host_internal_branches.cpp extends HostInternalBranches.csxsldu_directions, which runs the buffer-size direction combinations and the preprocess walks (row direction with row and column factors, column direction with column factors and a unit or non-unit lower diagonal) on a 2x2 diagonal matrix; the two column walks were missing from develop's version. It adds HostInternalBranches.csxsldu_preprocess_reference, which compares the pointers and counts with a host reference on 2x3, 3x2 and empty 3x0 matrices with strictly lower and upper entries, one-based input, distinct L and U bases, and factors in the other direction (pointers in the scratch buffer), and checks that two non-unit diagonals are rejected. The output arrays start at a -7 sentinel.
✅ 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. |
jsandham
left a comment
jsandham
left a comment
There was a problem hiding this comment.
Review
The three changes in csxsldu_preprocess_template match the three failure modes, and the new tests lock each one separately.
Base write. csxtril_count_kernel only stores per-sequence counts at ptr[seq + 1]. The inclusive scan therefore needs element 0 to already hold the factor base. Moving the lbase / ubase copies, and the stream sync that keeps those stack sources alive, ahead of the direction switch makes the column walk start from the base the same way the row walk already did. Distinct L and U bases in csxsldu_preprocess_reference separate "write the base" from "write zero".
Count index. The scan covers m + 1 pointers on a row walk and n + 1 on a column walk. Reading lptr[nseq] / uptr[nseq], with nseq = (dir == row) ? m : n, matches that length. A 2×3 column matrix would report a short unnz if this stayed at m, and a 3×2 column matrix would read past the last pointer. The row reference case is 3×2, so using n on the row path would fail there too.
Dispatch status. Both diagonals non-unit is rocsparse_status_invalid_value from csxtril_count_kernel_dispatch. That status was discarded, so the routine scanned whatever was already in the pointer arrays and returned success. It is now returned, in both directions. Dropping the LCOV_EXCL around that return is consistent with the new test actually executing it.
The column call still passes (udiag, ldiag, uptr, lptr). That swap is right: in CSC, seq > ind is the upper triangle, so the first count belongs to U. I walked the host reference against that kernel mapping for every case in the new test (2×3 and 3×2, one-based indices, distinct bases, factors in the other direction, the empty 3×0 column walk, and the 2×2 diagonal walks). Pointers and both counts match, including which array receives the scratch output (buffer when only one factor is in the other direction, buffer + nseq + 1 for U when both are).
The itilu0 sync-split caller still uses the row direction with m == n and unit diagonals, so its preprocess results are unchanged. csxsldu_compute_template already sizes its walk with (dir == row) ? m : n, so preprocess now agrees with it.
The unit-test bot warning is a filename heuristic. The new coverage lives in the existing unit_test_host_internal_branches.cpp, which is where the other host-internal branches are tested.
No functional issues found.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #14480 +/- ##
========================================
Coverage 76.89% 76.89%
========================================
Files 2886 2886
Lines 481037 481041 +4
Branches 72377 72381 +4
========================================
+ Hits 369847 369864 +17
+ Misses 86832 86821 -11
+ Partials 24358 24356 -2
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
TheRock Submodule Bump ActivityNewest first
|
JIRA ID : AISPARSE-906.
Summary
rocsparse::csxsldu_preprocess_template(the internal CSR/CSC triangular split behind the itilu0 sync-split path) computes the L and U pointer arrays by writing the index base into element 0, counting the entries of each row or column into elements 1..n, and running an inclusive scan.lnnz/unnzwere off by that value.ptr[m]in both directions. A column walk hasn + 1pointers, so form < nthe counts covered only the firstmcolumns, and form > nthe read went past the last pointer.ptr[m]for the row walk and atptr[n]for the column walk. The row path does the same work as before.clients/unittests/unit_test_host_internal_branches.cpp:HostInternalBranches.csxsldu_directionsruns the buffer-size direction combinations and the preprocess walks on a 2x2 diagonal matrix: row direction with row and with column factors, and column direction with column factors and a unit or non-unit lower diagonal (the column case also runs with the handle buffer dropped, so the scan allocates its own scratch). This restores the two column walks that develop's version of the test dropped.HostInternalBranches.csxsldu_preprocess_reference, with thesldu_case/sldu_referencehost reference helpers, compares every pointer and both counts with a host reference on non-square matrices (2x3 and 3x2, by column, and 3x2 by row) with strictly lower and upper entries, one-based input, distinct L and U bases, factors in the other direction, whose pointers go to the scratch buffer, and an empty (3x0) column walk. It also checks that two non-unit diagonals are rejected withinvalid_valuein both directions. Non-zero bases tell "write the base" apart from "write 0".Test plan
rocsparse-unit-test(23 tests) androcsparse-unit-test-device(1270 passed, plus the 27 existing skips) pass in full;HostInternalBranches.*13 passed.rocsparse-test --gtest_filter='*quick*csritilu0*:*pre_checkin*csritilu0*'(the sync-split caller) passes (668 tests).csxsldu_directionsfail: every pointer and both counts are offset by the -7 left in element 0 (the unit/unit walk giveslptr/uptr{-7, -7, -7} andlnnz-7 instead of {0, 0, 0} and 0; the non-unit-lower walk giveslptr{-7, -6, -5} andlnnz-5 instead of {0, 1, 2} and 2), andcsxsldu_preprocess_referencefails on every column case, including the empty one, and on the non-unit/non-unit check, which returns success. The row-direction reference case passes with and without the change.csxsldu_preprocess_referencefails on the 2x3 column cases (unnzcounts only the first two columns), and the 3x2 column case returns an error from the read past the last pointer.