✨ Compose multi-op modifier bodies on N targets in getUnitaryMatrix#1850
✨ Compose multi-op modifier bodies on N targets in getUnitaryMatrix#1850simon1hofmann wants to merge 8 commits into
N targets in getUnitaryMatrix#1850Conversation
N targets in getUnitaryMatrix
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughThis PR generalizes modifier body matrix composition to N targets, updates Ctrl/Inv modifier unitary computation to use it, adds supporting DynamicMatrix premultiplication helpers, and expands tests plus changelog entries. ChangesN-target composition feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp (1)
244-257: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMissing
ASSERT_TRUE(matrix)before dereference.Unlike every other test in this file using the same
getUnitaryMatrix()→std::optional<DynamicMatrix>pattern (e.g.ControlledHOpMatrix,ControlledXHOpMatrix,ControlledInverseHTOpMatrix),CXOpMatrixdereferencesmatrix->isApprox(...)without first assertingmatrixis engaged.std::optional::operator->does not check engagement, so an unexpectednullopthere would be UB rather than a clear test failure.🛡️ Proposed fix
const auto matrix = firstCtrlOp(*moduleOp).getUnitaryMatrix(); + ASSERT_TRUE(matrix); const Matrix4x4 expected =🤖 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/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp` around lines 244 - 257, The CXOpMatrix test is missing an explicit engagement check before dereferencing the optional returned by getUnitaryMatrix(). In QCOMatrixTest::CXOpMatrix, add an ASSERT_TRUE on the matrix result before calling matrix->isApprox(...), matching the pattern used by the other unit tests in this file such as ControlledHOpMatrix and ControlledXHOpMatrix. This keeps the test from dereferencing a disengaged std::optional and makes failures clear.
🤖 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/IR/QCOUtils.cpp`:
- Around line 33-80: Add brief Doxygen-style `///` comments for the new internal
helpers `lookupWireId`, `propagateWireIds`, and `embedUnitaryInBody` in
`QCOUtils.cpp`, matching the style used by sibling static helpers like
`entryIsApprox` and `checkedDim`. Keep the comments short but descriptive enough
to explain each helper’s role so the file complies with the C++ Doxygen
guideline.
- Around line 82-154: The current composeNTargetBodyMatrix path in QCOUtils.cpp
scales poorly because each UnitaryOpInterface is embedded into a full
DynamicMatrix and then premultiplyBy does a dense multiply, which becomes cubic
near the kMaxModifierTargetQubits cap. Update composeNTargetBodyMatrix and its
unitary handling so 1Q/2Q gates apply directly to the accumulator using
index-based stride updates instead of building a full embedded matrix, while
keeping the existing BarrierOp, GPhaseOp, and wireIds propagation behavior
intact.
---
Outside diff comments:
In `@mlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp`:
- Around line 244-257: The CXOpMatrix test is missing an explicit engagement
check before dereferencing the optional returned by getUnitaryMatrix(). In
QCOMatrixTest::CXOpMatrix, add an ASSERT_TRUE on the matrix result before
calling matrix->isApprox(...), matching the pattern used by the other unit tests
in this file such as ControlledHOpMatrix and ControlledXHOpMatrix. This keeps
the test from dereferencing a disengaged std::optional and makes failures clear.
🪄 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: e420ba71-f155-4491-92ce-20bacefe05ec
📒 Files selected for processing (10)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/QCOUtils.hmlir/include/mlir/Dialect/QCO/Utils/Matrix.hmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/QCOUtils.cppmlir/lib/Dialect/QCO/Utils/Matrix.cppmlir/unittests/Dialect/QCO/IR/CMakeLists.txtmlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cppmlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
…s for improved efficiency and clarity
|
@coderabbitai review |
✅ Action performedReview finished.
|
Description
Extend
InvOpandCtrlOp::getUnitaryMatrix()to compose multi-operation modifier bodies onN ≥ 1target wires, not only single-qubit sequences on one target.Replace
composeSingleQubitBodyMatrixwithcomposeNTargetBodyMatrix(Block&, numTargets), which:0..N-1(MSB-first, matchingMatrix2x2::embedInNqubit)2^NDynamicMatrixand accumulates viapremultiplyBynumTargets > 10InvOpreturns the adjoint of the composed matrix;CtrlOpwraps it with the existingI_{2^c} ⊗ Ucontrolled embed.Also adds
DynamicMatrix::premultiplyBy(const-ref and rvalue overloads) to support efficient in-place accumulation.Prerequisite for #1812
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.