✨ Compose multi-gate unitary matrices in CtrlOp and InvOp bodies#1809
Conversation
simon1hofmann
added
enhancement
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughA shared utility composes single-qubit body matrices and is wired into ChangesShared body-matrix composition utility and modifier integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
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}
There was a problem hiding this comment.
Actionable comments posted: 1
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/IR/Modifiers/InvOp.cpp (1)
416-418: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't short-circuit single-target phase-only bodies to identity.
The
getNumBodyUnitaries() == 0fast path skipscomposeSingleQubitBodyMatrix, soinvbodies with one target and onlyGPhaseOp/barriers still return identity instead of the adjoint phase matrix. Narrow this early return to truly empty or barrier-only identities, and let one-target phase-only bodies reach the shared composer.Also applies to: 432-435
🤖 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/IR/Modifiers/InvOp.cpp` around lines 416 - 418, The early return in InvOp’s body-matrix construction is too broad: it treats any zero-unitary body as identity and skips composeSingleQubitBodyMatrix, which breaks single-target bodies that contain only GPhaseOp and barriers. Narrow the fast path in InvOp so it only returns identity for truly empty or barrier-only cases, and let one-target phase-only bodies flow through the shared composer/adjoint logic; apply the same fix wherever this zero-unitary shortcut appears.
🤖 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/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp`:
- Around line 101-112: The QCO IR matrix test dereferences the result of
CtrlOp::getUnitaryMatrix() without first checking it for null, so a failed
lookup would crash instead of producing a clean test failure. In
test_qco_ir_matrix.cpp, update the test around ctrlOp and matrix to assert the
returned matrix object before calling matrix->isApprox(expected). Keep the
existing matrix comparison, but ensure the assertion on matrix happens first so
the test fails safely.
---
Outside diff comments:
In `@mlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp`:
- Around line 416-418: The early return in InvOp’s body-matrix construction is
too broad: it treats any zero-unitary body as identity and skips
composeSingleQubitBodyMatrix, which breaks single-target bodies that contain
only GPhaseOp and barriers. Narrow the fast path in InvOp so it only returns
identity for truly empty or barrier-only cases, and let one-target phase-only
bodies flow through the shared composer/adjoint logic; apply the same fix
wherever this zero-unitary shortcut appears.
🪄 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: 1c3aa8c8-b77e-4405-acbf-e6a462d5d44c
📒 Files selected for processing (9)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/QCOUtils.hmlir/lib/Dialect/QCO/IR/CMakeLists.txtmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/QCOUtils.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cppmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qco_programs.h
|
@coderabbitai review |
✅ Action performedReview finished.
|
Ahh, I just realized that this PR is only targeting single-target modifiers. I think this is a perfectly mergeable improvement on the status quo, but it might be better to adapt the wording in #1760 rather than marking this PR as closing it. At some point, we probably want to compose the unitary matrices of arbitrary modifier operations, which will add some more complexity. Sorry for the confusion! |
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
This generally LGTM with some small exceptions that are probably worth revisiting.
Co-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: simon1hofmann <119581649+simon1hofmann@users.noreply.github.com>
Signed-off-by: simon1hofmann <119581649+simon1hofmann@users.noreply.github.com>
… single-qubit operations and update tests for new functionality.
Signed-off-by: simon1hofmann <119581649+simon1hofmann@users.noreply.github.com>
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
One more round of changes until we can get this in 😊
Co-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: simon1hofmann <119581649+simon1hofmann@users.noreply.github.com>
…ected matrix calculations.
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Hm. The code here generally looks good to me now; except for the endianness thing you brought up.
I think we should strive to use a consistent endianness representation throughout the MQT. Currently, the interpretation between the MQT Core IR/the DD Package and the MLIR code base seem to differ. This sounds like it could lead to problems down the way.
In "old" MQT Core, the higher valued qubit index is always the more significant entry. DDs are always constructed with highest index on top. Thus, CX(1, 0) (1 as control, 0 as target) has the textbook CNOT matrix CX(0, 1) has the "less usual matrix representation.
This is also compatible with Qiskit: https://quantum.cloud.ibm.com/docs/en/api/qiskit/qiskit.circuit.library.CXGate#cxgate
Any thoughts on how to best handle this?
So there was basically an inconsistency before that already, as this was the test for CNOT: So what we would have to change:
The code won't be as nice anymore, as it's not as straightforward as creating an identity matrix and setting the bottom right corner by the body matrix anymore. Should we do that as part of this PR? |
Hm. The thing is that the two-target gates should be fine already. We run unit tests that compare the generated matrix with the DD Package one. And I believe these already use the same ordering of the qubits. Worth checking though. I would guess that the only "mistake" here is in the control modifier. But then again, I am not even sure if this is an error in the matrix computation itself. The current code simply puts the matrix in the bottom right corner of the larger identity matrix. This is correct in one case ( I think the problem is coming from the fact that the current implementation does not dictate any kind of endianness, but rather works as "control is higher, target is lower"-convention. Okay.. lots of loud thinking. Overall this has led me to believe that there is nothing actually wrong here. As long as the unitary synthesis rewires the matrices accordingly so that they are assembled and combined "correctly" with the assumption on the qubit ordering from above, we should be good. Does my rambling make any sense? Might also be worth to fact check with an LLM. @coderabbitai can you do that? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Locate relevant files =="
fd 'CtrlOp.cpp|Matrix.cpp|test_qco_ir_matrix.cpp|QCOUtils.cpp|qco_programs.cpp' mlir
echo
echo "== Outline: CtrlOp.cpp =="
ast-grep outline mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp --view expanded || true
echo
echo "== CtrlOp.cpp =="
sed -n '1,220p' mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp
echo
echo "== Outline: Matrix.cpp candidates =="
fd 'Matrix.cpp' mlir --exec ast-grep outline {} --view expanded || true
echo
echo "== Matrix.cpp content (first 260 lines) =="
fd 'Matrix.cpp' mlir --exec sed -n '1,260p' {}
echo
echo "== test_qco_ir_matrix.cpp relevant excerpts =="
sed -n '1,260p' mlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cpp
echo
echo "== qco_programs.cpp relevant excerpts =="
sed -n '1,220p' mlir/unittests/programs/qco_programs.cpp
echo
echo "== Search for DD-based matrix comparisons in tests =="
rg -n -C3 'getDD|StandardOperation|matrix4FromDefinition|DCX|CXOpMatrix|CtrlOp' mlir/unittestsLength of output: 103485 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== CtrlOp.cpp lower half =="
sed -n '220,420p' mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp
echo
echo "== Matrix.h relevant declarations =="
sed -n '1,260p' mlir/include/mlir/Dialect/QCO/Utils/Matrix.h
echo
echo "== Search for embed APIs/usages =="
rg -n -C3 'embedInNqubit|kron|copyBottomRightCorner|getUnitaryMatrix\(' mlir/include/mlir/Dialect/QCO/Utils/Matrix.h mlir/lib/Dialect/QCO/Utils/Matrix.cpp mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppLength of output: 21671
the codebase does already encode an endianness convention. What I found:
The suspicious part is indeed
So the fact-check is:
So I’d phrase it like this:
My practical recommendation for this PR:
Short version: your conclusion is close, but I would tighten it to “the bug is probably in (•ᴗ•)🐇 |
Should we merge this PR and then fix |
|
Tip For best results, initiate chat on the files or code changes.
Your experiment is a strong signal that So my recommendation would be:
A good follow-up issue would cover:
So: yes, merge now, but only if you also keep the scope explicit and file the follow-up. If you want, I can open that GitHub issue for you with the relevant context and acceptance criteria. (•ᵕᴗᵕ•)つ🐇 ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes.
My recommendation:
Reasoning:
So I’d frame it as:
Concrete follow-up issue scope:
If you want, I can open that GitHub issue for you now. ✏️ Learnings added
|
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Following the suggestion by the rabbit, let's merge this for now, and resolve the underlying issue in a follow up.
I think it might also be worth checking what Qiskit does here. As far as I can tell, they implement CX as a singleton gate with a fixed matrix. As such, I am not sure that they have the concept of "give me the matrix for this particar instatiation of an operation applied to these qubits". So it has to do this rewiring somewhere as well.
But let's maybe take this discussion to a follow-up issue or pull request.
It does not affect the functionality of this PR.
Sounds good, created a follow-up issue: #1821 |
3 participants
Description
composeSingleQubitBodyMatrixinQCOUtilsto compose compile-time single-qubit unitaries inside modifier bodies (in program order).CtrlOp::getUnitaryMatrix()to use composition when the body is not a single unitary gate (e.g.ctrl { x; h }).InvOp::getUnitaryMatrix()to reuse the same helper and take the adjoint, removing duplicated logic.invinsidectrl.Problem
CtrlOp::getUnitaryMatrix()only handled bodies with exactly one unitary op viagetSoleBodyUnitary. Multi-gate bodies likectrl { x; h }returnednullopt.Solution
Extract
composeSingleQubitBodyMatrix(Block&)intomlir/lib/Dialect/QCO/IR/QCOUtils.cppand use it from both modifiers:CtrlOp: sole body unitary → existing fast path; otherwise (1 target) compose body gates and embed as controlled block.InvOp: sole body unitary → adjoint of target matrix; otherwise compose body and returnfromAdjoint(composed).Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.