🔥 Remove Eigen dependency from QCO#1774
Conversation
…om matrix types. Assisted-by: Composer via Cursor
simon1hofmann
force-pushed
the
remove-eigen
branch
from
d5c7000 to
3e32169
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThis PR introduces a new QCO-native unitary matrix library ( ChangesQCO unitary matrix migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Really like the direction here @simon1hofmann!
Feels good to see that dependency gone.
I have a bit of cosmetic feedback and a bunch of comments on the matrix library that should hopefully be fairly easy to address. Overall I believe the current implementation is a tiny bit too raw and needs a bit of polish.
No major blockers though.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.td (1)
31-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep fixed-size
getUnitaryMatrix<T>()working for dynamic modifier results.This exact-type dispatch breaks callers that ask a modifier for a fixed-size matrix. In this stack,
CtrlOpandInvOpbuild their unitaries viaDynamicMatrix, so a 1-control single-qubitCtrlOpcan no longer satisfygetUnitaryMatrix<Matrix4x4>(), and an inverse of a single-qubit gate can no longer satisfygetUnitaryMatrix<Matrix2x2>(). That is a behavioral break from the previous dimension-based fallback. Please convert 1x1/2x2/4x4-sizedDynamicMatrixresults back into the requested fixed type instead of returningstd::nullopt.Also applies to: 204-226
🤖 Prompt for 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. In `@mlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.td` around lines 31 - 102, The size-specific method bodies (unitaryMatrix1x1MethodBody, unitaryMatrix2x2MethodBody, unitaryMatrix4x4MethodBody) currently only accept exact matching return types from $_op.getUnitaryMatrix(), which breaks callers that return a ::mlir::qco::DynamicMatrix (used by CtrlOp/InvOp); change each method body so that when getUnitaryMatrix() yields a DynamicMatrix whose runtime dimensions match the requested fixed size (1x1, 2x2, 4x4) you convert/copy it into the corresponding ::mlir::qco::Matrix1x1/Matrix2x2/Matrix4x4, assign to out, and return true instead of returning std::nullopt; keep the existing exact-type checks but add the DynamicMatrix-size check and conversion path before returning false or error.
🤖 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/lib/Dialect/QCO/Utils/UnitaryMatrix.cpp`:
- Around line 71-83: Add a shared validation that rejects negative dimensions
and invalid block sizes and prevents size_t underflow/overflow before any
indexing: create a helper (e.g., validateMatrixAndBlockDims) used by
DynamicMatrix constructor, identity, setBottomRightCorner, and
copyBottomRightCorner that checks dim >= 0, blockDim <= matrixDim, and that
static_cast<std::size_t>(dim) * static_cast<std::size_t>(dim) does not overflow
(or that dim <= max_size_t_sqrt); call this helper before allocating or
computing offsets and bail out (return/emit error/throw) if checks fail so no
negative dims are cast to size_t and no out-of-bounds indexing occurs.
In `@mlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp`:
- Around line 129-133: The test currently constructs "copied" as a const
DynamicMatrix which prevents invoking the move constructor; change the test to
create a non-const DynamicMatrix (e.g., remove const from "copied") and
construct "moved" by calling the move constructor with std::move(copied) (i.e.,
DynamicMatrix moved(std::move(copied))); then assert moved.isApprox(original) to
verify the move-ctor path in DynamicMatrix is exercised.
---
Outside diff comments:
In `@mlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.td`:
- Around line 31-102: The size-specific method bodies
(unitaryMatrix1x1MethodBody, unitaryMatrix2x2MethodBody,
unitaryMatrix4x4MethodBody) currently only accept exact matching return types
from $_op.getUnitaryMatrix(), which breaks callers that return a
::mlir::qco::DynamicMatrix (used by CtrlOp/InvOp); change each method body so
that when getUnitaryMatrix() yields a DynamicMatrix whose runtime dimensions
match the requested fixed size (1x1, 2x2, 4x4) you convert/copy it into the
corresponding ::mlir::qco::Matrix1x1/Matrix2x2/Matrix4x4, assign to out, and
return true instead of returning std::nullopt; keep the existing exact-type
checks but add the DynamicMatrix-size check and conversion path before returning
false or error.
🪄 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: 4a15c822-d785-43aa-8cb6-89792c0baf98
📒 Files selected for processing (37)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.tdmlir/include/mlir/Dialect/QCO/Utils/UnitaryMatrix.hmlir/lib/Dialect/QCO/IR/CMakeLists.txtmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/DCXOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/ECROp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/HOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/IdOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/POp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/ROp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RXOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RXXOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RYOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RYYOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RZOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RZXOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RZZOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/SOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/SWAPOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/SXOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/SXdgOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/SdgOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/TOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/TdgOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/U2Op.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/UOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/XOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/XXMinusYYOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/YOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/ZOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/iSWAPOp.cppmlir/lib/Dialect/QCO/Utils/CMakeLists.txtmlir/lib/Dialect/QCO/Utils/UnitaryMatrix.cppmlir/unittests/Dialect/QCO/Utils/CMakeLists.txtmlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/lib/Dialect/QCO/Utils/UnitaryMatrix.cpp (1)
277-285: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding debug-only bounds assertions.
Negative or out-of-bounds indices will produce undefined behavior when cast to
size_t. While the header documents valid ranges, defensive assertions would catch caller bugs during development.🔧 Suggested assertions
Complex& DynamicMatrix::operator()(const std::int64_t row, const std::int64_t col) { + assert(row >= 0 && row < impl_->dim && "row index out of bounds"); + assert(col >= 0 && col < impl_->dim && "col index out of bounds"); return impl_->data[static_cast<std::size_t>((row * impl_->dim) + col)]; } Complex DynamicMatrix::operator()(const std::int64_t row, const std::int64_t col) const { + assert(row >= 0 && row < impl_->dim && "row index out of bounds"); + assert(col >= 0 && col < impl_->dim && "col index out of bounds"); return impl_->data[static_cast<std::size_t>((row * impl_->dim) + col)]; }🤖 Prompt for 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. In `@mlir/lib/Dialect/QCO/Utils/UnitaryMatrix.cpp` around lines 277 - 285, Add debug-only bounds assertions to both DynamicMatrix::operator()(std::int64_t row, std::int64_t col) overloads: assert that row and col are >= 0 and < impl_->dim and that the computed index (row * impl_->dim + col) is < impl_->data.size() before casting to size_t and indexing impl_->data. Use a debug-only mechanism (e.g., assert) so checks are present in development builds only; apply the same checks in both the non-const and const operator() implementations to catch negative or out-of-range indices early.
🤖 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.
Outside diff comments:
In `@mlir/lib/Dialect/QCO/Utils/UnitaryMatrix.cpp`:
- Around line 277-285: Add debug-only bounds assertions to both
DynamicMatrix::operator()(std::int64_t row, std::int64_t col) overloads: assert
that row and col are >= 0 and < impl_->dim and that the computed index (row *
impl_->dim + col) is < impl_->data.size() before casting to size_t and indexing
impl_->data. Use a debug-only mechanism (e.g., assert) so checks are present in
development builds only; apply the same checks in both the non-const and const
operator() implementations to catch negative or out-of-range indices early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26d4a71d-d908-458f-8c67-a6a770225726
📒 Files selected for processing (2)
mlir/lib/Dialect/QCO/Utils/UnitaryMatrix.cppmlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp
|
Doing a full review of this now. I'll fix the open clang-tidy comments while I am at it. |
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Calls getUnitaryMatrix only when the type check is successful Signed-off-by: burgholzer <burgholzer@me.com>
… message Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
In the end, the respective functionality is general enough Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Alright. Finished with my round of cleanups.
Please have a look at the series of commits and check if they make sense.
I am happy with this now. Maybe you could do a last sanity check and/or CodeRabbit run before we merge this 😌
Signed-off-by: burgholzer <burgholzer@me.com>
burgholzer
added
dependencies
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
@burgholzer asked me to have a brief look at this so we can get #1751 ready as soon as possible. I pushed two very minor improvements just now. Given that CodeRabbit is happy as well, I'll set this PR to auto-merge. 🎉
denialhaag
changed the title
|
@denialhaag @burgholzer Thanks a lot for the final changes and finishing this PR 🙏 |
3 participants
Description
UnitaryMatrixlibrary (Matrix1x1,Matrix2,Matrix4,DynamicMatrix).getUnitaryMatrix()implementations offEigen::Matrix*types.FetchContent/MLIRQCODialectlink dependencies.This removes the need for
unsupported/Eigen/KroneckerProductand other Eigen headers in QCO IR.Motivation
Eigen was introduced primarily for matrix types and a Kronecker-product helper for controlled gates. For QCO's current needs that is heavy:
unsupported/Eigen/KroneckerProducthas no stability guarantee.CtrlOpnow buildsI_{2^n} ⊗ UviaDynamicMatrix::identity(dim)+setBottomRightCorner().InvOpuses in-place adjoint.AI Assistance
Used Composer via Cursor for parts of this change. I reviewed the full diff and take responsibility for everything in this PR.
Checklist
I have updated the documentation to reflect these changes.I have added migration instructions to the upgrade guide (if needed).If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.