✨ Add decompose-multi-controlled pass for multi-controlled X/Z/phase decomposition#1810
✨ Add decompose-multi-controlled pass for multi-controlled X/Z/phase decomposition#1810simon1hofmann wants to merge 51 commits into
decompose-multi-controlled pass for multi-controlled X/Z/phase decomposition#1810Conversation
…ontrolled X gates into one- and two-qubit gates.
…f multi-controlled Z gates.
…ted comments in McxSynthesis.cpp.
…criptions in test_multi_controlled_decomposition.cpp to reflect the one- vs two-ancilla boundary.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds the ChangesMulti-controlled gate decomposition
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant QCOProgram
participant PassPipeline
participant DecomposeMultiControlled
participant QCO_Module
Caller->>QCOProgram: decompose_multi_controlled(min_controls)
QCOProgram->>PassPipeline: populate configured pipeline
PassPipeline->>DecomposeMultiControlled: run pass
DecomposeMultiControlled->>QCO_Module: rewrite qco.ctrl and qco.rccx
QCO_Module-->>Caller: updated QCO program
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/Compiler/CompilerPipeline.cpp`:
- Around line 150-154: The `runPipeline` setup in `QuantumCompilerPipeline`
should validate `config_.decomposeMultiControlledMinControls` before any stages
mutate the module, instead of relying on `qco::createDecomposeMultiControlled`
to reject bad values later. Add an upfront config check alongside the other
early validations in `runPipeline`, using the `enableDecomposeMultiControlled` /
`decomposeMultiControlledMinControls` fields, and fail fast before Stage 1 if
the value is below 2.
In `@mlir/lib/Dialect/QCO/Transforms/Decomposition/McxSynthesis.cpp`:
- Around line 461-462: Clarify the one-dirty-ancilla branch in
McxSynthesis::decompose so the condition matches the comment’s “22+ controls”
wording and the parity-sensitive logic is unambiguous. Update the check on n to
explicitly reflect the intended control-count threshold in the dirty-ancilla
selection path, and keep the branch condition aligned with the surrounding
synthesis helpers so future cleanup does not invert the rule by mistake.
In `@mlir/unittests/Compiler/test_compiler_pipeline.cpp`:
- Around line 259-265: The CompilerPipelineTest end-to-end coverage only
exercises the X-path in DecomposeMultiControlledPass, so add a second
integration case for the Z-path using mlir::qc::multipleControlledZ. Update the
test fixture in test_compiler_pipeline.cpp by following the same pattern as the
existing runPipeline call and assert the MCZ pipeline behavior through the same
QCProgramBuilder::build and CompilationRecord flow. Keep the new case alongside
the current DecomposeMultiControlledPass coverage so both multipleControlledX
and multipleControlledZ are validated through the QC→QCO→optimization pipeline.
🪄 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: 83afc255-d120-40bc-8ed5-582fe1000e54
📒 Files selected for processing (12)
CHANGELOG.mdmlir/include/mlir/Compiler/CompilerPipeline.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/MultiControlled.hmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/lib/Compiler/CompilerPipeline.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/DecomposeMultiControlled.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/McxSynthesis.cppmlir/tools/mqt-cc/mqt-cc.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_multi_controlled_decomposition.cpppyproject.toml
|
@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/unittests/Compiler/test_compiler_pipeline.cpp (1)
259-281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDisable unrelated Stage 5 passes in these decomposition checks.
Both tests treat any
afterQCOCanon != afterOptimizationdiff as proof that multi-controlled decomposition ran, butrunPipeline(..., false, false, false, true, ...)still leavesMergeSingleQubitRotationGatesenabled. That means an unrelated Stage 5 change could keep these tests green even ifDecomposeMultiControlledregresses.Suggested fix
- runPipeline(module.get(), false, false, false, true, record); + runPipeline(module.get(), false, true, false, true, record);🤖 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/Compiler/test_compiler_pipeline.cpp` around lines 259 - 281, The decomposition tests in CompilerPipelineTest are currently too permissive because runPipeline still enables an unrelated Stage 5 pass, so a change in MergeSingleQubitRotationGates could make the afterQCOCanon versus afterOptimization comparison pass even if multi-controlled decomposition fails. Update DecomposeMultiControlledPass and DecomposeMultiControlledPassMcz to run with only the decomposition-relevant pipeline enabled, or otherwise disable the Stage 5 rotation-merge path in runPipeline for these checks, so the EXPECT_NE assertion specifically validates DecomposeMultiControlled and the multipleControlledX/multipleControlledZ cases.
🤖 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/Compiler/test_compiler_pipeline.cpp`:
- Around line 283-297: The test currently only checks that
QuantumCompilerPipeline::runPipeline fails, which still allows the rejection to
happen later in DecomposeMultiControlled after earlier stages have run. Update
CompilerPipelineTest.RejectsDecomposeMultiControlledMinControlsBelowTwo to also
assert that CompilationRecord::afterQCImport remains empty when
decomposeMultiControlledMinControls is below two, so the preflight validation is
enforced before Stage 1 or any intermediate records are produced.
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/test_multi_controlled_decomposition.cpp`:
- Around line 378-404: The two “untouched” tests only verify that one
multi-controlled op remains, so they can miss regressions where the surviving
operation changes kind; update McxDecompositionTest::LeavesMultiOpCtrlUntouched
and McxDecompositionTest::LeavesMultiControlledHUntouched to assert the
preserved op is still the expected CtrlOp/MCH form and that its body is
unchanged after runDecomposePass, using the existing QCOProgramBuilder setup and
countMultiControlledOps as a secondary check only.
---
Outside diff comments:
In `@mlir/unittests/Compiler/test_compiler_pipeline.cpp`:
- Around line 259-281: The decomposition tests in CompilerPipelineTest are
currently too permissive because runPipeline still enables an unrelated Stage 5
pass, so a change in MergeSingleQubitRotationGates could make the afterQCOCanon
versus afterOptimization comparison pass even if multi-controlled decomposition
fails. Update DecomposeMultiControlledPass and DecomposeMultiControlledPassMcz
to run with only the decomposition-relevant pipeline enabled, or otherwise
disable the Stage 5 rotation-merge path in runPipeline for these checks, so the
EXPECT_NE assertion specifically validates DecomposeMultiControlled and the
multipleControlledX/multipleControlledZ 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
Run ID: f9c18476-abd2-4d99-94d9-f4385100f19b
📒 Files selected for processing (5)
mlir/lib/Compiler/CompilerPipeline.cppmlir/lib/Dialect/QCO/Transforms/CMakeLists.txtmlir/lib/Dialect/QCO/Transforms/Decomposition/MultiControlled.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_multi_controlled_decomposition.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@CHANGELOG.md`:
- Around line 15-17: Update the changelog entry for the
decompose-multi-controlled pass to mention controlled-phase and RCCX
decomposition in addition to controlled X and Z gates, while retaining the
configurable min-controls threshold and existing attribution.
In `@mlir/include/mlir/Dialect/QCO/IR/QCOOps.td`:
- Around line 788-816: Update RCCXOp’s trait list to include Pure alongside
UnitaryOpInterface and ThreeTargetZeroParameter, and remove the MemRead
annotations from qubit0_in, qubit1_in, and qubit2_in so the linear qubit
arguments are modeled without memory-read effects.
In `@mlir/include/mlir/Dialect/QCO/Transforms/Passes.td`:
- Around line 65-67: Update the basis option declaration in the pass options to
remove the unsupported “r” value from its description, leaving exactly zyz, zxz,
xzx, xyx, u, and zsxx as advertised supported Euler bases.
In `@mlir/lib/Dialect/QCO/IR/Operations/StandardGates/RCCXOp.cpp`:
- Around line 18-42: Cache the result of elementaryRCCXUnitary in
RCCXOp::getUnitaryMatrix using a function-local static initialized by the
existing computation, relying on C++11 thread-safe magic statics. Ensure
subsequent calls return the cached constant DynamicMatrix without rebuilding it
or performing repeated allocations and multiplications.
In `@mlir/lib/Dialect/QCO/Transforms/Decomposition/DecomposeMultiControlled.cpp`:
- Around line 76-78: Update the CCPhase decomposition sequences, including the
default min-controls=2 path, to use the existing p operation for each balanced
±θ rotation instead of rz, including the additional occurrences in the related
decomposition blocks. Preserve the paired angles and sequence ordering so their
global-phase differences continue to cancel and the decomposition remains within
the {h, t, tdg, p, cx} basis.
🪄 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: 0847b1fb-da87-40d8-a644-d5fbad15fc91
📒 Files selected for processing (15)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/include/mlir/Support/Passes.hmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RCCXOp.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/DecomposeMultiControlled.cppmlir/lib/Support/Passes.cppmlir/tools/mqt-cc/mqt-cc.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_multi_controlled_decomposition.cpppyproject.toml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🤖 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 `@CHANGELOG.md`:
- Around line 15-17: Update the changelog entry for the
decompose-multi-controlled pass to mention controlled-phase and RCCX
decomposition in addition to controlled X and Z gates, while retaining the
configurable min-controls threshold and existing attribution.
In `@mlir/include/mlir/Dialect/QCO/IR/QCOOps.td`:
- Around line 788-816: Update RCCXOp’s trait list to include Pure alongside
UnitaryOpInterface and ThreeTargetZeroParameter, and remove the MemRead
annotations from qubit0_in, qubit1_in, and qubit2_in so the linear qubit
arguments are modeled without memory-read effects.
In `@mlir/include/mlir/Dialect/QCO/Transforms/Passes.td`:
- Around line 65-67: Update the basis option declaration in the pass options to
remove the unsupported “r” value from its description, leaving exactly zyz, zxz,
xzx, xyx, u, and zsxx as advertised supported Euler bases.
In `@mlir/lib/Dialect/QCO/IR/Operations/StandardGates/RCCXOp.cpp`:
- Around line 18-42: Cache the result of elementaryRCCXUnitary in
RCCXOp::getUnitaryMatrix using a function-local static initialized by the
existing computation, relying on C++11 thread-safe magic statics. Ensure
subsequent calls return the cached constant DynamicMatrix without rebuilding it
or performing repeated allocations and multiplications.
In `@mlir/lib/Dialect/QCO/Transforms/Decomposition/DecomposeMultiControlled.cpp`:
- Around line 76-78: Update the CCPhase decomposition sequences, including the
default min-controls=2 path, to use the existing p operation for each balanced
±θ rotation instead of rz, including the additional occurrences in the related
decomposition blocks. Preserve the paired angles and sequence ordering so their
global-phase differences continue to cancel and the decomposition remains within
the {h, t, tdg, p, cx} basis.
🪄 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: 0847b1fb-da87-40d8-a644-d5fbad15fc91
📒 Files selected for processing (15)
CHANGELOG.mdmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/include/mlir/Support/Passes.hmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RCCXOp.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/DecomposeMultiControlled.cppmlir/lib/Support/Passes.cppmlir/tools/mqt-cc/mqt-cc.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir_matrix.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_multi_controlled_decomposition.cpppyproject.toml
🛑 Comments failed to post (5)
CHANGELOG.md (1)
15-17: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document controlled-phase and RCCX decomposition.
The entry currently advertises only X and Z, omitting two noteworthy capabilities introduced by this PR.
Proposed wording
-- ✨ Add a `decompose-multi-controlled` pass for decomposing controlled X and Z - gates with a configurable `min-controls` threshold ([`#1810`]) +- ✨ Add a `decompose-multi-controlled` pass for decomposing multi-controlled + X, Z, phase, and RCCX operations with a configurable `min-controls` threshold + ([`#1810`])As per coding guidelines, “Update
CHANGELOG.mdfor user-facing, breaking, or otherwise noteworthy changes.”📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.- ✨ Add a `decompose-multi-controlled` pass for decomposing multi-controlled X, Z, phase, and RCCX operations with a configurable `min-controls` threshold ([`#1810`]) ([**`@simon1hofmann`**])🤖 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 `@CHANGELOG.md` around lines 15 - 17, Update the changelog entry for the decompose-multi-controlled pass to mention controlled-phase and RCCX decomposition in addition to controlled X and Z gates, while retaining the configurable min-controls threshold and existing attribution.Source: Coding guidelines
mlir/include/mlir/Dialect/QCO/IR/QCOOps.td (1)
788-816: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Add
Puretrait and remove[MemRead]from linear value arguments.
RCCXOpis missing thePuretrait, and its arguments are incorrectly annotated with[MemRead]. Qubit values in the QCO dialect are linear SSA values, not memory pointers, so they do not incur memory read side effects.Adding the
Puretrait ensures that standard MLIR optimizations (like CSE and canonicalization) correctly model this operation as side-effect free, consistent with all other unitary gates (e.g.,XOp,SWAPOp).🐛 Proposed fix
-def RCCXOp - : QCOOp<"rccx", traits = [UnitaryOpInterface, ThreeTargetZeroParameter]> { +def RCCXOp : QCOOp<"rccx", + traits = [UnitaryOpInterface, ThreeTargetZeroParameter, Pure]> { let summary = "Apply a relative-phase CCX gate to three qubits"; let description = [{ Applies a relative-phase Toffoli (`RCCX`) to three qubits and returns the transformed qubits. The gate acts as a controlled `@f`$X@f$ on `@f`$q_2@f$ when `@f`$q_0@f$ and `@f`$q_1@f$ are `@f`$|1\rangle@f$, but may introduce a non-trivial phase on `@f`$|111\rangle@f$ relative to a standard `CCX`. This matches the `rccx` gate in OpenQASM 3 `qelib1.inc` and is commonly used as a building block in multi-controlled `@f`$X@f$ decompositions. Example: ```mlir %c0_out, %c1_out, %t_out = qco.rccx %c0_in, %c1_in, %t_in : !qco.qubit, !qco.qubit, !qco.qubit -> !qco.qubit, !qco.qubit, !qco.qubit ``` }]; let arguments = - (ins Arg<QubitType, "the first control qubit", [MemRead]>:$qubit0_in, - Arg<QubitType, "the second control qubit", [MemRead]>:$qubit1_in, - Arg<QubitType, "the target qubit", [MemRead]>:$qubit2_in); + (ins Arg<QubitType, "the first control qubit">:$qubit0_in, + Arg<QubitType, "the second control qubit">:$qubit1_in, + Arg<QubitType, "the target qubit">:$qubit2_in); let results = (outs QubitType:$qubit0_out, QubitType:$qubit1_out, QubitType:$qubit2_out);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def RCCXOp : QCOOp<"rccx", traits = [UnitaryOpInterface, ThreeTargetZeroParameter, Pure]> { let summary = "Apply a relative-phase CCX gate to three qubits"; let description = [{ Applies a relative-phase Toffoli (`RCCX`) to three qubits and returns the transformed qubits. The gate acts as a controlled `@f`$X@f$ on `@f`$q_2@f$ when `@f`$q_0@f$ and `@f`$q_1@f$ are `@f`$|1\rangle@f$, but may introduce a non-trivial phase on `@f`$|111\rangle@f$ relative to a standard `CCX`. This matches the `rccx` gate in OpenQASM 3 `qelib1.inc` and is commonly used as a building block in multi-controlled `@f`$X@f$ decompositions. Example:🤖 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/QCOOps.td` around lines 788 - 816, Update RCCXOp’s trait list to include Pure alongside UnitaryOpInterface and ThreeTargetZeroParameter, and remove the MemRead annotations from qubit0_in, qubit1_in, and qubit2_in so the linear qubit arguments are modeled without memory-read effects.mlir/include/mlir/Dialect/QCO/Transforms/Passes.td (1)
65-67: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the unsupported
rEuler basis.Advertising
rmay lead users to select a basis this pass cannot synthesize.Proposed fix
- "Target Euler basis (zyz, zxz, xzx, xyx, u, zsxx, r).">]; + "Target Euler basis (zyz, zxz, xzx, xyx, u, zsxx).">];Based on learnings, the supported bases are exactly
zyz,zxz,xzx,xyx,u, andzsxx.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let options = [Option< "basis", "basis", "std::string", "\"zyz\"", "Target Euler basis (zyz, zxz, xzx, xyx, u, zsxx).">];🤖 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/Transforms/Passes.td` around lines 65 - 67, Update the basis option declaration in the pass options to remove the unsupported “r” value from its description, leaving exactly zyz, zxz, xzx, xyx, u, and zsxx as advertised supported Euler bases.Source: Learnings
mlir/lib/Dialect/QCO/IR/Operations/StandardGates/RCCXOp.cpp (1)
18-42: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Cache the static unitary matrix to avoid repeated dynamic allocations.
RCCXOp::getUnitaryMatrix()returns a constant 8x8 unitary. Building this matrix from scratch dynamically on every call requires numerous heap allocations and heavy matrix multiplications, which can become a bottleneck during compiler passes.Please consider caching the result using a thread-safe static variable (C++11 magic statics) so the matrix is only computed once.
⚡ Proposed optimization
static DynamicMatrix elementaryRCCXUnitary() { - constexpr Matrix4x4 cx = - Matrix4x4::fromElements(1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, - 0.0, 1.0, 0.0, 0.0, 1.0, 0.0); - - DynamicMatrix unitary = DynamicMatrix::identity(8); - const auto apply1 = [&](const Matrix2x2& gate, const std::size_t qubit) { - unitary = gate.embedInNqubit(3, qubit) * unitary; - }; - const auto applyCx = [&](const std::size_t control, - const std::size_t target) { - unitary = cx.embedInNqubit(3, control, target) * unitary; - }; - - apply1(HOp::getUnitaryMatrix(), 2); - apply1(TOp::getUnitaryMatrix(), 2); - applyCx(1, 2); - apply1(TdgOp::getUnitaryMatrix(), 2); - applyCx(0, 2); - apply1(TOp::getUnitaryMatrix(), 2); - applyCx(1, 2); - apply1(TdgOp::getUnitaryMatrix(), 2); - apply1(HOp::getUnitaryMatrix(), 2); - return unitary; + static const DynamicMatrix unitary = [] { + constexpr Matrix4x4 cx = + Matrix4x4::fromElements(1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, + 0.0, 1.0, 0.0, 0.0, 1.0, 0.0); + + DynamicMatrix u = DynamicMatrix::identity(8); + const auto apply1 = [&](const Matrix2x2& gate, const std::size_t qubit) { + u = gate.embedInNqubit(3, qubit) * u; + }; + const auto applyCx = [&](const std::size_t control, + const std::size_t target) { + u = cx.embedInNqubit(3, control, target) * u; + }; + + apply1(HOp::getUnitaryMatrix(), 2); + apply1(TOp::getUnitaryMatrix(), 2); + applyCx(1, 2); + apply1(TdgOp::getUnitaryMatrix(), 2); + applyCx(0, 2); + apply1(TOp::getUnitaryMatrix(), 2); + applyCx(1, 2); + apply1(TdgOp::getUnitaryMatrix(), 2); + apply1(HOp::getUnitaryMatrix(), 2); + return u; + }(); + return unitary; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.static DynamicMatrix elementaryRCCXUnitary() { static const DynamicMatrix unitary = [] { constexpr Matrix4x4 cx = Matrix4x4::fromElements(1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 1.0, 0.0); DynamicMatrix u = DynamicMatrix::identity(8); const auto apply1 = [&](const Matrix2x2& gate, const std::size_t qubit) { u = gate.embedInNqubit(3, qubit) * u; }; const auto applyCx = [&](const std::size_t control, const std::size_t target) { u = cx.embedInNqubit(3, control, target) * u; }; apply1(HOp::getUnitaryMatrix(), 2); apply1(TOp::getUnitaryMatrix(), 2); applyCx(1, 2); apply1(TdgOp::getUnitaryMatrix(), 2); applyCx(0, 2); apply1(TOp::getUnitaryMatrix(), 2); applyCx(1, 2); apply1(TdgOp::getUnitaryMatrix(), 2); apply1(HOp::getUnitaryMatrix(), 2); return u; }(); return unitary; }🤖 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/Operations/StandardGates/RCCXOp.cpp` around lines 18 - 42, Cache the result of elementaryRCCXUnitary in RCCXOp::getUnitaryMatrix using a function-local static initialized by the existing computation, relying on C++11 thread-safe magic statics. Ensure subsequent calls return the cached constant DynamicMatrix without rebuilding it or performing repeated allocations and multiplications.mlir/lib/Dialect/QCO/Transforms/Decomposition/DecomposeMultiControlled.cpp (1)
76-78: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the decomposition within the promised
{h, t, tdg, p, cx}basis.The default
min-controls=2path emitsRZOps through the CCPhase decomposition. Replace these balancedrz(±θ)calls withp(±θ); their global-phase differences cancel across each sequence.Proposed fix
- void rz(std::size_t q, double theta) { - setWire(q, RZOp::create(*builder_, loc_, wire(q), theta).getOutputQubit(0)); - } - void crz(std::size_t control, std::size_t target, double theta) { const double half = theta / 2.0; - rz(target, half); + p(target, half); cx(control, target); - rz(target, -half); + p(target, -half); cx(control, target); } @@ - rz(target, -quarter); + p(target, -quarter); @@ - rz(target, quarter); + p(target, quarter); @@ - rz(target, -quarter); + p(target, -quarter); @@ - rz(target, quarter); + p(target, quarter);Also applies to: 109-115, 148-162
🤖 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/Transforms/Decomposition/DecomposeMultiControlled.cpp` around lines 76 - 78, Update the CCPhase decomposition sequences, including the default min-controls=2 path, to use the existing p operation for each balanced ±θ rotation instead of rz, including the additional occurrences in the related decomposition blocks. Preserve the paired angles and sequence ordering so their global-phase differences continue to cancel and the decomposition remains within the {h, t, tdg, p, cx} basis.
|
@coderabbitai review |
✅ Action performedReview finished.
|
…Controlled` method
…e redundant phrasing regarding `minControls` parameter.
decompose-multi-controlled pass for multi-controlled X/Z decompositiondecompose-multi-controlled pass for multi-controlled X/Z/phase decomposition
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/Compiler/Programs.cpp (1)
135-142: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse
WalkResultto short-circuit the dialect search.Currently,
mod->walkvisits every operation in the module, even if the target dialect is found early. For large modules, this can cause unnecessary overhead. Both duplicated implementations ofmoduleUsesDialectcan be optimized by usingWalkResult::interrupt()to short-circuit the traversal once the dialect is found.
mlir/lib/Compiler/Programs.cpp#L135-L142: Refactor to useWalkResultand returnresult.wasInterrupted().mlir/tools/mqt-cc/mqt-cc.cpp#L127-L134: Apply the identical short-circuiting refactor here.♻️ Proposed refactor
[[nodiscard]] static bool moduleUsesDialect(ModuleOp mod, const StringRef dialect) { - auto found = false; - mod->walk([&](Operation* operation) { - found |= operation->getDialect()->getNamespace() == dialect; - }); - return found; + const auto result = mod->walk([&](Operation* operation) { + if (operation->getDialect()->getNamespace() == dialect) { + return WalkResult::interrupt(); + } + return WalkResult::advance(); + }); + return result.wasInterrupted(); }🤖 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/Compiler/Programs.cpp` around lines 135 - 142, Update both moduleUsesDialect implementations in mlir/lib/Compiler/Programs.cpp (lines 135-142) and mlir/tools/mqt-cc/mqt-cc.cpp (lines 127-134) to use the walk result: interrupt traversal with WalkResult::interrupt() when the target dialect is found and return result.wasInterrupted() instead of walking every operation and accumulating a boolean.
🤖 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/Support/Passes.cpp`:
- Around line 36-41: Revert the unrelated pass-runner cleanup in
runWithPassManager and the affected code ranges, restoring the original
parameter names, formatting, and cleanup-pipeline diagnostic behavior. Keep only
changes required for multi-controlled decomposition and do not alter pass-runner
output or refactor surrounding code.
---
Outside diff comments:
In `@mlir/lib/Compiler/Programs.cpp`:
- Around line 135-142: Update both moduleUsesDialect implementations in
mlir/lib/Compiler/Programs.cpp (lines 135-142) and mlir/tools/mqt-cc/mqt-cc.cpp
(lines 127-134) to use the walk result: interrupt traversal with
WalkResult::interrupt() when the target dialect is found and return
result.wasInterrupted() instead of walking every operation and accumulating a
boolean.
🪄 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: 3c5b2523-c669-409d-8739-3bf619eef03f
📒 Files selected for processing (14)
CHANGELOG.mdbindings/mlir/register_mlir.cppmlir/include/mlir/Compiler/Programs.hmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/include/mlir/Support/Passes.hmlir/lib/Compiler/Programs.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RCCXOp.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/DecomposeMultiControlled.cppmlir/lib/Support/Passes.cppmlir/tools/mqt-cc/mqt-cc.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_multi_controlled_decomposition.cpppython/mqt/core/mlir.pyitest/python/test_mlir.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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/Compiler/Programs.cpp (1)
135-142: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse
WalkResultto short-circuit the dialect search.Currently,
mod->walkvisits every operation in the module, even if the target dialect is found early. For large modules, this can cause unnecessary overhead. Both duplicated implementations ofmoduleUsesDialectcan be optimized by usingWalkResult::interrupt()to short-circuit the traversal once the dialect is found.
mlir/lib/Compiler/Programs.cpp#L135-L142: Refactor to useWalkResultand returnresult.wasInterrupted().mlir/tools/mqt-cc/mqt-cc.cpp#L127-L134: Apply the identical short-circuiting refactor here.♻️ Proposed refactor
[[nodiscard]] static bool moduleUsesDialect(ModuleOp mod, const StringRef dialect) { - auto found = false; - mod->walk([&](Operation* operation) { - found |= operation->getDialect()->getNamespace() == dialect; - }); - return found; + const auto result = mod->walk([&](Operation* operation) { + if (operation->getDialect()->getNamespace() == dialect) { + return WalkResult::interrupt(); + } + return WalkResult::advance(); + }); + return result.wasInterrupted(); }🤖 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/Compiler/Programs.cpp` around lines 135 - 142, Update both moduleUsesDialect implementations in mlir/lib/Compiler/Programs.cpp (lines 135-142) and mlir/tools/mqt-cc/mqt-cc.cpp (lines 127-134) to use the walk result: interrupt traversal with WalkResult::interrupt() when the target dialect is found and return result.wasInterrupted() instead of walking every operation and accumulating a boolean.
🤖 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/Support/Passes.cpp`:
- Around line 36-41: Revert the unrelated pass-runner cleanup in
runWithPassManager and the affected code ranges, restoring the original
parameter names, formatting, and cleanup-pipeline diagnostic behavior. Keep only
changes required for multi-controlled decomposition and do not alter pass-runner
output or refactor surrounding code.
---
Outside diff comments:
In `@mlir/lib/Compiler/Programs.cpp`:
- Around line 135-142: Update both moduleUsesDialect implementations in
mlir/lib/Compiler/Programs.cpp (lines 135-142) and mlir/tools/mqt-cc/mqt-cc.cpp
(lines 127-134) to use the walk result: interrupt traversal with
WalkResult::interrupt() when the target dialect is found and return
result.wasInterrupted() instead of walking every operation and accumulating a
boolean.
🪄 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: 3c5b2523-c669-409d-8739-3bf619eef03f
📒 Files selected for processing (14)
CHANGELOG.mdbindings/mlir/register_mlir.cppmlir/include/mlir/Compiler/Programs.hmlir/include/mlir/Dialect/QCO/Transforms/Passes.tdmlir/include/mlir/Support/Passes.hmlir/lib/Compiler/Programs.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/RCCXOp.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/DecomposeMultiControlled.cppmlir/lib/Support/Passes.cppmlir/tools/mqt-cc/mqt-cc.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_multi_controlled_decomposition.cpppython/mqt/core/mlir.pyitest/python/test_mlir.py
🛑 Comments failed to post (1)
mlir/lib/Support/Passes.cpp (1)
36-41: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Revert the unrelated pass-runner cleanup.
These parameter renames and cleanup-pipeline diagnostic changes do not support multi-controlled decomposition. Keep them in a separate PR to avoid unrelated output and review churn.
As per coding guidelines, “Keep changes focused on the assigned task; do not perform unrelated cleanup, broad reformatting, dependency upgrades, or refactors without explicit authorization.”
Also applies to: 78-94, 121-141
🤖 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/Support/Passes.cpp` around lines 36 - 41, Revert the unrelated pass-runner cleanup in runWithPassManager and the affected code ranges, restoring the original parameter names, formatting, and cleanup-pipeline diagnostic behavior. Keep only changes required for multi-controlled decomposition and do not alter pass-runner output or refactor surrounding code.Source: Coding guidelines
Description
Add a
decompose-multi-controlledQCO pass that decomposes controlled X/Z/phase(
qco.ctrlwith a singleqco.x,qco.z, or constant-angleqco.pbody) withat least
min-controlscontrols.(adapted from Qiskit
synth_mcx_noaux_hp24); (k = 3) and (k = 2) useelementary CCCX/CCX/CCZ/CCPhase sequences. Also expands
qco.rccxwhenmin-controls <= 2.size-based chooser — the recursive "v24" method for 3–4 controls and the
linear-depth da Silva–Park 2022 (SP22) method for 5+ controls (adapted from
Qiskit's
MCPhaseGatesynthesis), matching the CX-count crossover of the twomethods. (k = 2) keeps the existing two-controlled phase sequence.
MCZ for (k >= 4) uses the HP24 core directly (MCZ = H·MCX·H, with redundant
Hadamard bookends canceled). A compile-time phase of ±π equals Z and is routed
through the same MCZ path.
Usage
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.