feat(rocSolver): Improve performance of SYGST/HEGST#9986
Draft
EdDAzevedo wants to merge 16 commits into
Draft
Conversation
✅ 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. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets rocSOLVER’s SYGST/HEGST performance by avoiding the unblocked SYGS2/HEGS2 path and instead using rocBLAS TRSM/TRMM-based updates, with expanded test coverage for larger/edge matrix sizes.
Changes:
- Added an alternative SYGS2/HEGS2 implementation that symmetrizes
A, performs TRSM/TRMM updates, then restores the saved triangular part. - Updated SYGST/HEGST workspace sizing and internal call sites to use the new alternative path for small blocks.
- Expanded gtest matrix-size coverage around the 1024 boundary; minor client parsing cleanup and changelog entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| projects/rocsolver/library/src/lapack/roclapack_sygst_hegst.hpp | Introduces TRSM/TRMM-based alternative for SYGS2/HEGS2 and integrates it into the blocked SYGST/HEGST flow, including a new symmetrize/restore kernel and workspace sizing. |
| projects/rocsolver/clients/gtest/lapack/sygsx_hegsx_gtest.cpp | Adjusts large-matrix test sizes to include 1023/1024/1025 edge cases. |
| projects/rocsolver/clients/common/misc/program_options.hpp | Initializes temporary parse variables to avoid uninitialized warnings. |
| projects/rocsolver/CHANGELOG.md | Notes SYGST/HEGST performance improvement. |
Comments suppressed due to low confidence (1)
projects/rocsolver/library/src/lapack/roclapack_sygst_hegst.hpp:592
len_Asaveis computed usinglbatch_count, butsygs2_hegs2_altcallscopy_symm_tri(..., batch_count, ...)and the kernel indexesAsaveby the fullbidrange[0, batch_count). Ifbatch_count > lbatch_count,Asave + bid*strideAsavewill go out of bounds.Asaveneeds to be sized for the fullbatch_count(or the algorithm needs to explicitly chunk batches across multiple kernel launches with synchronization).
I const max_blocks = get_max_blocks();
I const lbatch_count = std::min(max_blocks, batch_count);
size_t const len_Asave = size_t(nb * (nb - 1) / 2) * lbatch_count;
T* const Asave = static_cast<T*>(work_x_temp);
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (76.07%) 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 #9986 +/- ##
===========================================
- Coverage 69.59% 69.53% -0.06%
===========================================
Files 2771 2771
Lines 452699 452943 +244
Branches 66658 66679 +21
===========================================
- Hits 315022 314915 -107
- Misses 117337 117692 +355
+ Partials 20340 20336 -4
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
projects/rocsolver/library/src/lapack/roclapack_sygst_hegst.hpp:100
- In the per-batch loop, the scratch buffer pointer
Bis indexed withbid_startrather than the currentbid. Whenbatch_count > gridDim.z(i.e., >max_blocks), each z-block iterates over multiple batches and will incorrectly read/write the sameBBslice for all iterations, corrupting saved/restored triangular data for batches beyond the first.
T* const B = load_ptr_batch<T>(BB, bid_start, shiftB, strideB);
EdDAzevedo
marked this pull request as draft
July 26, 2026 19:31
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.
Motivation
This PR improves the performance of SYGST/HEGST by replacing the call to the unblocked routine SYGS2/HEGS2. The computations are replaced by calls to rocBLAS TRSM (triangular solve) or rocBLAS TRMM (triangular multiplication).
Technical Details
The PR is motivated by JIRA ID: AISOLVE-376 and github issue 936. Issue 936 shows for n = 1024, MI250 attained about 29 Gflops/sec when cusolver on Nvidia GH200 achieved 1270 Gflops/sec. For n = 10240, the gap is even greater where MI250 achieved 1750 Gflops/sec, while GH200 achieved 16000 Gflops/sec.
Profiling by rocprof indicates the unblocked SYGST/HEGST are expensive routines for performing
A <- inv(R) * A * inv(R), or A <- R * A * inv(R), where R = L or U' from Cholesky factorization of matrix B in solving the generalized eigenvalue problem.
The PR modifies SYGST/HEGST to replace the calls to SYGST/HEGST by calling rocBLAS TRSM or rocBLAS TRMM directly.
This approach cannot easily take advantage of symmetry and performs twice as many floating point operations in the operations for SYGS2/HEGS2. Moreover, extra O( nb^2 ) ( where nb = 256 ) temporary work space will be needed. A larger block size nb may yield higher performance but more temporary work space will be required.
https://amd-hub.atlassian.net/browse/AISOLVE-376
ROCm/rocSOLVER#936
Test Plan
The google test cases have been expanded to include larger matrix sizes. All google tests should pass.
Test Result
On MI210 for n = 1024, using the command
./clients/staging/rocsolver-bench -f sygst -n 1024 --itype 1 --precision d --uplo L --perf 1
it took 2853 micro seconds.
Assuming SYGST took n^3 floating point operations, this is about 376 Gflops/sec.
Similarly, for --itype = 2, it took about 2736 micro seconds or achieved about 392 Gflops/sec.
For n=8192, for --itype = 1, it took about 0.25 sec or achieved about 2199 Gflops/sec and for --itype = 2, it took about 0.20 sec or achieved about 2749 Gflops/sec.
For n=10240, for --itype= 1, it took about 0.33 sec or achieved about 3254 Gflops/sec, and for --itype = 2, it took about 0.27 sec and achieved about 3977 Gflops/sec.
Submission Checklist