feat(rocsolver): Implement ORGHR/UNGHR - #12197
Open
AGonzales-amd wants to merge 4 commits into
Open
AGonzales-amd wants to merge 4 commits into
AGonzales-amd wants to merge 4 commits into
Conversation
Adds rocsolver_[sd]orghr and rocsolver_[cz]unghr, which generate the orthogonal/unitary matrix Q defined as the product of Householder reflectors returned by GEHRD (Hessenberg reduction). Functions are exported from the binary (ROCSOLVER_EXPORT) but not added to the public API header, following the LAHR2 pattern. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Adds a CMake option (default ON) to optionally disable ORGHR and UNGHR. When OFF, all entry points return rocblas_status_not_implemented and tests verify that behaviour instead of running the full computation. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The changes span GPU implementation, build integration, dispatching, references, and tests, warranting final human review.
Pull request overview
Adds internal ORGHR/UNGHR GPU routines to generate orthogonal/unitary matrices from Hessenberg reflectors.
Changes:
- Implements routines, workspace handling, and binary exports.
- Integrates build flags, dispatching, and LAPACK references.
- Adds tests, benchmarks, and changelog documentation.
File summaries
| File | Description |
|---|---|
projects/rocsolver/library/src/CMakeLists.txt |
Builds the implementation. |
projects/rocsolver/library/src/auxiliary/rocauxiliary_orghr_unghr.hpp |
Defines core logic and workspace handling. |
projects/rocsolver/library/src/auxiliary/rocauxiliary_orghr_unghr.cpp |
Provides exported wrappers. |
projects/rocsolver/CMakeLists.txt |
Adds feature configuration. |
projects/rocsolver/clients/gtest/CMakeLists.txt |
Registers tests and feature definitions. |
projects/rocsolver/clients/gtest/auxiliary/orghr_unghr_gtest.cpp |
Adds parameterized tests. |
projects/rocsolver/clients/common/misc/rocsolver.hpp |
Declares client interfaces. |
projects/rocsolver/clients/common/misc/rocsolver_dispatcher.hpp |
Adds dispatcher entries. |
projects/rocsolver/clients/common/misc/lapack_host_reference.hpp |
Declares reference routines. |
projects/rocsolver/clients/common/misc/lapack_host_reference.cpp |
Implements LAPACK references. |
projects/rocsolver/clients/common/auxiliary/testing_orghr_unghr.hpp |
Provides testing and validation helpers. |
projects/rocsolver/clients/common/auxiliary/testing_orghr_unghr.cpp |
Instantiates test templates. |
projects/rocsolver/clients/CMakeLists.txt |
Includes shared test sources. |
projects/rocsolver/clients/benchmarks/CMakeLists.txt |
Propagates feature configuration. |
projects/rocsolver/CHANGELOG.md |
Documents the new routines. |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The documented tau allocation is undersized for ilo > 1 and could lead callers to trigger out-of-bounds reads.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
projects/rocsolver/library/src/auxiliary/rocauxiliary_orghr_unghr.cpp:216
- This buffer contract is too small when
ilo > 1: the implementation indexestau + (ilo - 1), and GEHRD returns the active scalars in ann - 1array. A caller following the statedihi - iloallocation can therefore trigger an out-of-bounds device read. Document the full GEHRD layout instead.
tau pointer to type. Array on the GPU of at least ihi - ilo scalars.
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Balanced
| lda rocblas_int. lda >= n. | ||
| The leading dimension of A. | ||
| @param[in] | ||
| tau pointer to type. Array on the GPU of at least ihi - ilo scalars. |
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
Implement ORGHR/UNGHR for general Eigensolver (GEEV) effort.
Technical Details
Adds rocsolver_orghr and rocsolver_unghr, which generate the orthogonal/unitary matrix Q defined as the product of Householder reflectors returned by Hessenberg reduction (GEHRD).
Functions are exported from the binary but not added to the public API header.
Test Plan
Add tests for the new routines.
Submission Checklist