✨ Enhance Matrix functionality with new methods and optimizations#1802
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Had some time, so I thought I'd give this a first review already 😌
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughExpands the QCO MLIR matrix utilities in ChangesQCO Matrix Utility Expansion
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Matrix4x4
participant symmetricTred24
participant symmetricTql24
Caller->>Matrix4x4: symmetricEigen4()
Matrix4x4->>symmetricTred24: tridiagonalize real symmetric 4x4 input
symmetricTred24-->>Matrix4x4: tridiagonal form + orthogonal accumulator Q
Matrix4x4->>symmetricTql24: QL iteration on tridiagonal
symmetricTql24-->>Matrix4x4: converged eigenvalues and eigenvectors
Matrix4x4-->>Caller: SymmetricEigen4 {eigenvalues[4], eigenvectors Matrix4x4}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/include/mlir/Dialect/QCO/Utils/Matrix.h`:
- Around line 791-794: Add member documentation to the SymmetricEigen4 struct
for consistency with other structs in the file. Add a brief /// comment above
the eigenvalues member that describes what it contains (the eigenvalues of the
matrix), and add a /// comment above the eigenvectors member that describes what
it contains (the corresponding eigenvectors as a Matrix4x4). Follow the same
documentation style used in other struct members throughout the file.
In `@mlir/lib/Dialect/QCO/Utils/Matrix.cpp`:
- Around line 458-470: The column() and setColumn() methods in Matrix4x4 lack
bounds validation on the col parameter, unlike the row APIs, allowing invalid
column indices to cause out-of-bounds access. Add assert statements in both
column() and setColumn() to verify that col is less than K_COLS before
performing any array indexing operations.
- Line 326: The bit shift operation `1ULL << numQubits` at line 326 and another
occurrence at line 518 lack validation for the numQubits parameter, which can
cause undefined behavior when numQubits is 64 or greater and signed integer
overflow when numQubits is 63 or greater. Create a checked helper function that
validates numQubits is within a safe range (less than 63 to prevent both shift
overflow and signed integer overflow) before performing the bit shift, then
replace both instances of the unsafe `1ULL << numQubits` expression with calls
to this helper function to guard against overflow.
- Around line 350-351: The functions Matrix2x2::embedInTwoQubit and
Matrix4x4::reorderForQubits return Matrix4x4 but end with assert(false && ...)
statements. In NDEBUG builds, these assertions are removed, causing the
functions to fall through without returning a value, resulting in undefined
behavior. Replace the assert(false && "Invalid qubit index for single-qubit
embed") call with llvm::reportFatalInternalError() passing the error message,
which will properly terminate execution in both debug and release builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7dba2258-18ab-43b4-b8b2-19b7ae967b83
📒 Files selected for processing (4)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Utils/Matrix.hmlir/lib/Dialect/QCO/Utils/Matrix.cppmlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
Thanks a lot for the quick review 🙏 |
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
LGTM. Just two minor comments/suggestions left. Feel free to address and resolve these and merge this afterwards.
One overall comment on the whole PR iteration process: it would be good if you wouldn't resolve review comments that you replied to as part of incorporating feedback. People using the GitHub App/Notifications (like me) do not see these comments without going through all resolved comments, opening them up, and checking whether there have been any replies. Most of the time, it is best for the person who wrote the last comment in a discussion thread to leave it to someone else to actually resolve the conversation. Makes the whole process a little smoother. When I get a request for a re-review, I will typically first go through all open conversations of the previous review before going through the actual code.
I assume you are resolving the comments to indicate that you have addressed the comments and to keep a clean slate. Hopefully the above is only a slight adaptation to your workflow.
Let me know if any of that does not make sense.
Co-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: simon1hofmann <119581649+simon1hofmann@users.noreply.github.com>
I usually resolve comments where I do no think a re-review is necessary, but makes total sense to let the other person resolve that comment. Will keep that in mind in the following PR's! I addressed the remaining comments, should be good to go in after all tests are green. |
2 participants
Description
Extends
mlir::qcomatrix utilities with fixed-size helpers, n-qubit embedding/reordering, and a real symmetric 4×4 eigensolver. These are building blocks for native two-qubit gate synthesis (Weyl decomposition and related passes).New APIs
symmetricEigen4— eigendecomposition of a real symmetric 4×4 matrix via EISPACKtred2/tql2(adapted from John Burkardt’s MIT-licensed C port); includes aMatrix4x4overload that usesrealPart()kron— Kronecker product for single-qubit factors (MSB-first wire order)reorderTwoQubitMatrix— conjugate a 4×4 gate to act on wires{0,1}embedSingleQubitInNqubit/embedTwoQubitInNqubit— embed fixed gates into larger Hilbert spacesMatrix4x4/DynamicMatriximprovementstranspose,diagonal,fromDiagonal,column,setColumn,realPart,imagPart,isIdentityDynamicMatrix::operator*=and scalaroperator*overloads for 2×2 / 4×4 / dynamic matricesDynamicMatrixmultiply; single-passisIdentitymultiply2x2/multiply4x4,isIdentityEntries)AI Assistance
Used
Composer 2.5via Cursor for parts of this change. I reviewed the fulldiff and take responsibility for everything in this PR.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.