✨ Add bottom-up makeThreeQubitGateDD for RCCX - #1950
Conversation
Direct DD construction for three-qubit gates (same style as makeGateDD / makeTwoQubitGateDD), with shared control-wrapping helpers, GateMatrixDefinitions, and tests. Assisted-by: Cursor Grok 4.5 via Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
7fd25f8 to
b0ecac6
Compare
makeThreeQubitGateDD for RCCX
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds native RCCX three-qubit matrix conversion and decision-diagram construction, including controlled variants. Shared DD-building helpers replace RCCX-specific construction, with expanded QCO integration, matrix, circuit, invalid-operation, and controlled-gate tests. ChangesRCCX decision-diagram support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operations
participant Package
participant GateMatrixDefinitions
participant DecisionDiagram
Operations->>Package: makeThreeQubitGateDD
Package->>GateMatrixDefinitions: opToThreeQubitGateMatrix
GateMatrixDefinitions-->>Package: return RCCX matrix
Package->>DecisionDiagram: reduce matrix and wrap controls
DecisionDiagram-->>Operations: return mEdge
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
- Updated DDFunctionality.h to include three-qubit gates in the supported programs. - Modified RCCXOp to correctly define the unitary matrix for three-qubit operations. - Implemented logic in DDFunctionality.cpp to handle three-qubit gate matrices. - Added tests for three-qubit gates in the unit test suite to ensure proper functionality.
|
@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)
src/dd/Package.cpp (1)
156-174: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject empty packages before subtracting from
nqubits.For
Package(0),nqubits - 1Uunderflows, so target/control0passes validation and construction proceeds with an out-of-range DD level. This affects all three gate builders using this helper.Proposed fix
[[noreturn]] void throwGateQubitOutOfRange(const std::size_t nqubits) { + if (nqubits == 0U) { + throw std::runtime_error( + "Cannot construct a gate in a package with zero qubits."); + } throw std::runtime_error{ "Requested gate acting on qubit(s) with index larger than " + std::to_string(nqubits - 1U) + @@ void ensureGateQubitsInRange(const std::size_t nqubits, const qc::Controls& controls, const std::initializer_list<qc::Qubit> targets) { - const auto maxQubit = static_cast<Qubit>(nqubits - 1U); - if (std::ranges::any_of( - controls, [maxQubit](const auto& c) { return c.qubit > maxQubit; }) || - std::ranges::any_of(targets, - [maxQubit](const Qubit t) { return t > maxQubit; })) { + if (nqubits == 0U || + std::ranges::any_of(controls, [nqubits](const auto& c) { + return static_cast<std::size_t>(c.qubit) >= nqubits; + }) || + std::ranges::any_of(targets, [nqubits](const Qubit target) { + return static_cast<std::size_t>(target) >= nqubits; + })) { throwGateQubitOutOfRange(nqubits); } }🤖 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 `@src/dd/Package.cpp` around lines 156 - 174, Update ensureGateQubitsInRange to reject nqubits == 0 before computing nqubits - 1U, using the existing throwGateQubitOutOfRange error path so all gate builders receive consistent validation. Preserve the current control and target range checks for non-empty packages.
🤖 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 `@src/dd/Package.cpp`:
- Around line 156-174: Update ensureGateQubitsInRange to reject nqubits == 0
before computing nqubits - 1U, using the existing throwGateQubitOutOfRange error
path so all gate builders receive consistent validation. Preserve the current
control and target range checks for non-empty packages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1156ed52-c2ba-45c6-ba10-a52932b0df80
📒 Files selected for processing (6)
mlir/include/mlir/Dialect/QCO/Utils/DDFunctionality.hmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RCCXOp.cppmlir/lib/Dialect/QCO/Utils/DDFunctionality.cppmlir/unittests/Dialect/QCO/Utils/test_dd_functionality.cppsrc/dd/GateMatrixDefinitions.cppsrc/dd/Package.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/dd/Package.cpp`:
- Around line 169-181: Extend ensureGateQubitsInRange to reject duplicate target
qubits and any control qubit that matches a target, while preserving the
existing range validation and throwing through throwGateQubitOutOfRange. Add
regression coverage for duplicate targets and control/target overlap, including
the currently skipped RCCX cases.
🪄 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 Plus
Run ID: 12002321-85db-487c-a80b-42dab6b7e6dd
📒 Files selected for processing (2)
src/dd/Package.cpptest/dd/test_package.cpp
- Introduced a new function `throwGateQubitsNotDistinct` to handle errors for duplicate or overlapping control/target qubits. - Enhanced `ensureGateQubitsInRange` to check for distinct qubit targets and overlapping controls, throwing appropriate exceptions when violations occur. - Added unit tests to `DDPackageTest` to verify that overlapping gate qubits are correctly rejected during gate construction.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
burgholzer
left a comment
There was a problem hiding this comment.
This feels really clean. Let's get this in 🚀
🤖 AI text below 🤖
Summary
ThreeQubitGateMatrix/makeThreeQubitGateDDfor direct bottom-up DD construction of three-qubit gates (same style asmakeGateDD/makeTwoQubitGateDD), starting with RCCX viaopToThreeQubitGateMatrix.targets[0]= high bit), matchingmakeTwoQubitGateDDand QCO (embed/CtrlOp); RCCX is the5/6/7form (not the Qiskit little-endian3/5/7labeling).getStandardOperationDDinstead of the multiply-based construction.buildFunctionality/simulatedense path throughmakeThreeQubitGateDDfor 3-wire unitaries (plain copy; full-widthmakeDDFromMatrixstill remaps QCO MSB → global DD LSB).invin QCO DD functionality tests.Checklist
If PR contains AI-assisted content:
🤖 *AI text below* 🤖(titles are exempt).Assisted-by: [Model Name] via [Tool Name]footer.