fix(rocalution): fix CSR diagonal operations and local matrix tests for non-square matrices by ntrost57 · Pull Request #14103 · ROCm/rocm-libraries

ntrost57 · 2026-10-21T11:55:55Z

Motivation

The local matrix tests fail or abort on the "Random" test matrix, the only non-square one: 36 failures in local_matrix_add, and aborts in local_add_scalar_off_diagonal, local_diagonal_matrix_mult, local_diagonal_matrix_mult_l and local_triple_matrix_product. Some of these are library bugs that also abort user code on tall matrices; the rest are test bugs. CI doesn't catch them because these tests skip under every ROCALUTION_EMULATION_* variable, so only the full suite runs them.

Technical Details

Library:

ExtractDiagonal() and ExtractInverseDiagonal(): the host and HIP CSR backends now accept min(nrow, ncol) entries, which is what LocalMatrix allocates. The backends only write entries with row == col, so the writes were always in bounds.
DiagonalMatrixMultL(): the host and HIP CSR backends now assert nrow entries instead of ncol, since they index diag by row. LocalMatrix now requires M entries for DiagonalMatrixMultL() and N entries for DiagonalMatrixMultR(), instead of accepting either for both. All callers in the library already pass N entries to MultR, and none call MultL.
rocalution_stop_hip() now resets the accelerator flag. Before, restarting with disable_accelerator_rocalution(true) still reported an accelerator whose handles had been destroyed. The test binary does exactly this for its host tests.

Behavior change: passing N entries to DiagonalMatrixMultL() on a wide matrix (M < N) used to run and scale by the first M entries. It now fails the assert. On a tall matrix the same call read out of bounds.

Tests:

local_matrix_add checks structure == false against the sparsity pattern of the first matrix, and also checks structure == true, which had no test.
local_add_scalar_off_diagonal builds the diagonal with min(M, N) entries.
local_diagonal_matrix_mult passes N entries and checks column scaling.
local_triple_matrix_product uses the Galerkin shapes C^T · B · C with an M × M matrix B.
The diagonal tests use non-constant values, so row and column scaling differ on square matrices.
The device variants of these tests now move their objects to the accelerator. Before, they only tested the host backend.
backend_disable_accelerator_after_stop inits with the accelerator, stops, inits again with disable_accelerator_rocalution(true), and checks that no accelerator is reported. It fails without the rocalution_stop_hip() fix.

Test Result

The six affected test groups: 648/648 pass on host and device. Before: 36 failures and 4 aborts.
local_matrix*:local_vector*:backend*:preconditioner*:*amg*: 7276/7276 pass.
The device variants launch kernel_csr_extract_diag and kernel_csr_diagmatmult_l (checked with AMD_LOG_LEVEL=4).
*backend*:*local_matrix*: 5561/5561 pass.

Dependency

Merge this first. #13995 and #14104 rename the preconditioner test helper, which removes the getTestMatrix clash, so the local_matrix tests get real Random matrices. Both fail Math CI until this PR lands. Merge order: this PR, #13995, then #14190 and #14104 in either order.

JIRA ID : AISPARSE-888

therock-pr-bot · 2026-10-21T12:00:07Z

✅ All Checks Passed — Ready for Review

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🔎 pre-commit	✅ Pass	—
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—
🤖 therock-pr-bot	✅ Pass	—

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

therock-pr-bot · 2026-10-21T12:00:11Z

🎉 All checks passed! This PR is ready for review.

…n and DiagonalMatrixMultL LocalMatrix::ExtractDiagonal() and ExtractInverseDiagonal() allocate min(M, N) entries, but the host and HIP CSR backends asserted at least (or exactly) nrow entries, so both aborted on any tall matrix. The backends only write entries with row == col, which are bounded by min(nrow, ncol), so relax the asserts to that. DiagonalMatrixMultL() computes diag * this and indexes diag by row, but both CSR backends asserted diag.GetSize() == ncol. A correct call with M entries aborted on non-square matrices, and a call with N entries on a tall matrix read past the end of diag. Assert nrow instead, and tighten the LocalMatrix asserts to M entries for MultL and N entries for MultR. Co-authored-by: Cursor <cursoragent@cursor.com>

…kend rocalution_stop_hip() destroyed the rocBLAS and rocSPARSE handles but left the accelerator flag set, and init_rocalution() only updates the flag when the accelerator is enabled. After init/stop with the accelerator, a later init with disable_accelerator_rocalution(true) still reported an available accelerator, so MoveToAccelerator() moved objects to the HIP backend and crashed on the destroyed handles. Co-authored-by: Cursor <cursoragent@cursor.com>

…run them on the device The "Random" test matrix is the only non-square matrix and the only one whose sparsity pattern changes between calls. Several tests assumed square matrices or equal patterns: - local_matrix_add expected the full sum, but MatrixAdd keeps the pattern of this by default. Check structure == false against the pattern of the first matrix, and also check structure == true. - local_add_scalar_off_diagonal built a diagonal with M entries for an M x N matrix. Use min(M, N) entries. - local_diagonal_matrix_mult passed M entries and expected row scaling, but DiagonalMatrixMult() is DiagonalMatrixMultR(). Pass N entries and check column scaling. - local_triple_matrix_product multiplied four M x N matrices. Use the Galerkin shapes C^T * B * C with an M x M matrix B. The diagonal tests now use non-constant values so that row and column scaling differ on square matrices. The device variants never moved any object to the accelerator and so only tested the host backend. They now do. Co-authored-by: Cursor <cursoragent@cursor.com>

After an init with the accelerator and a stop, an init with disable_accelerator_rocalution(true) has to report no accelerator. Co-authored-by: Cursor <cursoragent@cursor.com>

…d test _rocalution_available_accelerator() is not exported from rocalution.dll, so the client test failed to link on Windows. Co-authored-by: Cursor <cursoragent@cursor.com>

…4103 Co-authored-by: Cursor <cursoragent@cursor.com>

…and #14103 Co-authored-by: Cursor <cursoragent@cursor.com>

assistant-librarian · 2026-10-26T19:32:29Z

TheRock Submodule Bump Activity

Newest first

2026-10-26 — Included in TheRock via ROCm/TheRock#9489.

…14185) ## Motivation `HIPAcceleratorVector::Amax` used the 1-based index returned by rocBLAS `iamax` as a 0-based index. On the accelerator, it returned the index and the value of the entry after the largest one, and when the largest entry was the last one, it read past the end of the vector. This also affected the Linf residual norm (`SetResidualNorm(3)`) and `GetAmaxResidualIndex()` of the iterative solvers on the accelerator. The amax test did not catch this, because it never moved its vector to the accelerator. Depends on #14103: the device amax test now uses the accelerator, and without the accelerator flag reset from #14103, the host tests that run after it crash. This PR targets the #14103 branch, so it only shows its own commit. GitHub switches it to `develop` when #14103 merges. ## Technical Details - In `HIPAcceleratorVector::Amax`, convert the index returned by rocBLAS to 0-based before reading the value. - In `testing_amax`, move the vector to the accelerator so that the HIP implementation runs, check that the last entry is found as the largest, and also check a negative largest entry in the middle of the vector. ## Test Result - Without the fix, the device amax test fails with `HIP error: invalid argument` (out-of-bounds copy). - Full `rocalution-test` suite on gfx1100 (Release): 9318 passed, no failures. JIRA ID : AISPARSE-915 Co-authored-by: Cursor <cursoragent@cursor.com>

…14104) ## Motivation `testing_local_gershgorin` never checks anything. Its early return uses `||` instead of `&&`, so it returns for every matrix type, and its expected bounds are wrong too, so fixing only the condition makes it fail. ## Technical Details - Fix the early return so the test runs for Laplacian2D and PermutedIdentity. - Laplacian2D: expect [0, 8]. Interior rows have 4 on the diagonal and four -1 neighbors. - PermutedIdentity: expect [-1, 1]. `gen_permuted_identity()` builds a cyclic column shift, which has no diagonal entries for the tested sizes, so every Gershgorin disc is centered at 0 with radius 1. Test-only change. ## Test Result Release build: all 108 `local_gershgorin` instances pass. A `ROCALUTION_LAYER=1` trace confirms `Gershgorin()` now runs for Laplacian2D and PermutedIdentity. Before, the 54 host instances took 1 ms in total and never called it. ## Dependency Needs to be rebased once #14103 and #13995 are merged. Renaming the preconditioner helper removes the `getTestMatrix` clash, so the `local_matrix` tests now get real `Random` matrices. `local_matrix_add` then fails until the fix in #14103 lands. #13995 carries the same rename commit, plus the call in its ItILU0 history test, so merge it first. Rebasing onto `develop` then drops this PR's copy of the rename ("patch contents already upstream") and leaves only the Gershgorin commit. JIRA ID : AISPARSE-889 --------- Co-authored-by: Cursor <cursoragent@cursor.com>

…eck fixes (#14354) ## Motivation #13989, #14090, #14001, #14089, #13991, #14088, #14091, #14100 and #14103 were merged without a changelog entry. This adds them to the unreleased section above 4.1.1. ## Technical Details - Add one entry under "Resolved issues" for each of the first five PRs. #13989 was merged as part of #14090 (bbba2b3). - Drop the version number from the "(Unreleased) rocALUTION 4.2.0" heading added in #14106, since the next version is not decided yet, and add the entries to that section. - Add the entries for #14088, #14091, #14100 and #14103. The `DiagonalMatrixMultL()` / `DiagonalMatrixMultR()` size requirement from #14103 and the MPI sample runtime output from #14100 go under "Changed". The rest of #14100 only changes tests. - #14184 and #14187 are not included. They were folded into #14093, which is still open, so their entries belong in that PR. ## Test Result Changelog only, no code change. pre-commit passes. JIRA ID : AISPARSE-869, AISPARSE-870, AISPARSE-878, AISPARSE-880, AISPARSE-881, AISPARSE-882, AISPARSE-883, AISPARSE-887, AISPARSE-888 --------- Co-authored-by: Cursor <cursoragent@cursor.com>

## Motivation `HIPAcceleratorMatrixCSR::ItILU0Factorize` ignored the status returned by `rocsparseTcsritilu0_history`. When the query fails, the caller receives a history buffer that was never written and an iteration count read from rocSPARSE's workspace. This is reachable: rocSPARSE rejects the query for the default and AsyncInPlace algorithms whenever `ConvergenceHistory` is set without `ComputeNrmResidual`. The single precision path also leaked its temporary history buffer on every call. ## Technical Details - Store and check the status of both history queries (double and single precision). - On failure, log a warning with the rocSPARSE status and report an empty history (`niter = 0`) instead of exiting, because the factorization itself is valid and only the diagnostic history is missing. - Replace the leaked `new float[]` buffer in the single precision path with a `std::vector<float>`, and only copy it into the caller's buffer when the query succeeds. - Add `preconditioner/parameterized_itilu0_history.convergence_history_device_{float,double}`. It builds ItILU0 on the device with a convergence history for AsyncInPlace, AsyncSplit and SyncSplit and checks the iteration count and the residual history. It also checks that the rejected configuration (Default and AsyncInPlace without residual norms) completes and reports an empty history. Until now no test set `ConvergenceHistory`. The test checks residual norms only. rocSPARSE has separate ItILU0 issues: the asynchronous algorithms do not compute correction norms, and SyncSplit's stopping norms depend on the initial contents of the work buffer. These are tracked separately. **Merge note:** the "without residual" test case expects `niter == 0` only because rocSPARSE currently rejects that query. #14190 replaces that case: rocALUTION now requests the residual norm whenever the stopping criteria or the history is enabled, so the query no longer depends on the rocSPARSE fixes. Merge this PR first, then #14190. **Dependency:** needs to be rebased once #14103 is merged. Renaming the preconditioner helper removes the `getTestMatrix` clash, so the `local_matrix` tests now get real `Random` matrices and fail until the fixes in #14103 land. #14104 carries the same rename commit without the ItILU0 history test call, so merge this PR before #14104; its copy then drops out when it is rebased. JIRA ID : AISPARSE-871 --------- Co-authored-by: Cursor <cursoragent@cursor.com>

The accelerator Amax returns a zero-based index since #14185, stopping rocalution resets the accelerator flag since #14103, ILUT computes the row limit in 64 bit since #13943, and the iterative U solve of a HIP matrix has its own analysis since #14186. Run these checks on both backends again, without KNOWN_BUG and expect_in_child. The complex accelerator Amax stays a known bug, since rocblasTamax is a stub there. Co-authored-by: Cursor <cursoragent@cursor.com>

…iteria and the history (#14190) ## Motivation rocSPARSE evaluates the ItILU0 stopping criteria and records the convergence history from norms that it only computes when they are requested. With `ItILU0Option::StoppingCriteria` or `ConvergenceHistory` but neither `ComputeNrmResidual` nor `ComputeNrmCorrection`, the accelerator path breaks: SyncSplit stops after the first sweep and returns zero factors, so the preconditioner is the identity; AsyncSplit fails with an illegal memory access; and the AsyncInPlace history query fails. rocALUTION does not document that these options need a norm option. In debug builds, requesting a history without `StoppingCriteria` also aborts on an assert. Depends on #13995, which adds the ItILU0 history test and checks the history status. This PR targets the #13995 branch, so it only shows its own commit. GitHub switches it to `develop` when #13995 merges. Merge order: #14103, #13995, then this PR. #13995 fails Math CI until #14103 lands. Because #13995 is squash-merged, this branch then has to be rebased onto `develop` with `--onto`, dropping the #13995 commits; a plain rebase conflicts on them. ## Technical Details - In `HIPAcceleratorMatrixCSR::ItILU0Factorize`, request `rocsparse_itilu0_option_compute_nrm_residual` whenever `StoppingCriteria` or `ConvergenceHistory` is set. rocSPARSE documents its stopping test in terms of the residual, and the asynchronous algorithms only use the residual. SyncSplit stops on the larger of the two norms when both are computed, so with `ComputeNrmCorrection` alone, its stopping test now also takes the residual into account. - Remove the assert that a history requires `StoppingCriteria`. With the residual norm always computed, rocSPARSE records a history for every algorithm. - In `testing_itilu0_convergence_history`, replace the case that expected an empty history without a norm option. For `StoppingCriteria`, `ConvergenceHistory` and both without a norm option, and for the Default, AsyncInPlace, AsyncSplit and SyncSplit algorithms, the factorization has to run more than one sweep, record a valid residual history when requested, and give a preconditioner that is not the identity. This works with the current rocSPARSE and with either fix for AISPARSE-873. It also removes the reason #13995 waits for the rocSPARSE fixes, since its "without residual" case no longer exists. ## Test Result - Without the fix, the new cases fail: SyncSplit reports `niter` = 1, and AsyncSplit fails with an illegal memory access. - `preconditioner/*:itsolver/*` (Release, `ROCALUTION_CODE_COVERAGE=1`): 68/68 pass. - `*itilu0_history*:itsolver/*` in a Debug build: 32/32 pass. JIRA ID : AISPARSE-918 --------- Co-authored-by: Cursor <cursoragent@cursor.com>

ntrost57 requested a review from a team as a code owner October 21, 2026 11:55

ntrost57 added bug project: rocalution labels Oct 21, 2026

assistant-librarian Bot added the organization: ROCm label Oct 21, 2026

ntrost57 mentioned this pull request Oct 22, 2026

fix(rocalution): use a 0-based index in HIPAcceleratorVector::Amax #14185

Merged

ntrost57 added this pull request to stack #14192 October 22, 2026 10:41

ntrost57 and others added 5 commits October 24, 2026 13:48

test(rocalution): check that the accelerator flag is reset on stop

ee23f94

After an init with the accelerator and a stop, an init with disable_accelerator_rocalution(true) has to report no accelerator. Co-authored-by: Cursor <cursoragent@cursor.com>

test(rocalution): drop the internal accelerator check from the backen…

4c93be0

…d test _rocalution_available_accelerator() is not exported from rocalution.dll, so the client test failed to link on Windows. Co-authored-by: Cursor <cursoragent@cursor.com>

ntrost57 force-pushed the users/ntrost/rocalution/fix/local-matrix-nonsquare branch from 9173f94 to 4c93be0 Compare October 24, 2026 11:48

YvanMokwinski approved these changes Oct 24, 2026

View reviewed changes

ntrost57 merged commit bb3d694 into develop Oct 25, 2026
87 checks passed

ntrost57 deleted the users/ntrost/rocalution/fix/local-matrix-nonsquare branch October 25, 2026 11:52

ntrost57 added a commit that referenced this pull request Oct 25, 2026

docs(rocalution): add the changelog entries for #14091, #14100 and #1…

1193bd7

…4103 Co-authored-by: Cursor <cursoragent@cursor.com>

ntrost57 added a commit that referenced this pull request Oct 25, 2026

docs(rocalution): add the changelog entries for #14088, #14091, #14100 …

e962d71

…and #14103 Co-authored-by: Cursor <cursoragent@cursor.com>

ntrost57 mentioned this pull request Oct 25, 2026

docs(rocalution): add the missing changelog entries for the status check fixes #14354

Merged

Navigation Menu

Repository navigation

fix(rocalution): fix CSR diagonal operations and local matrix tests for non-square matrices - #14103

fix(rocalution): fix CSR diagonal operations and local matrix tests for non-square matrices#14103
ntrost57 merged 5 commits into
developfrom
users/ntrost/rocalution/fix/local-matrix-nonsquare

ntrost57 commented Oct 21, 2026 •

edited

Loading

Uh oh!

therock-pr-bot Bot commented Oct 21, 2026 •

edited

Loading

Uh oh!

therock-pr-bot Bot commented Oct 21, 2026

Uh oh!

Uh oh!

assistant-librarian Bot commented Oct 26, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ntrost57 commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Test Result

Dependency

Uh oh!

therock-pr-bot Bot commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All Checks Passed — Ready for Review

Uh oh!

therock-pr-bot Bot commented Oct 21, 2026

Uh oh!

Uh oh!

assistant-librarian Bot commented Oct 26, 2026

TheRock Submodule Bump Activity

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ntrost57 commented Oct 21, 2026 •

edited

Loading

therock-pr-bot Bot commented Oct 21, 2026 •

edited

Loading