✨ Add decompose-multi-controlled pass for multi-controlled X/Z decomposition#1810
✨ Add decompose-multi-controlled pass for multi-controlled X/Z decomposition#1810simon1hofmann wants to merge 18 commits into
decompose-multi-controlled pass for multi-controlled X/Z decomposition#1810Conversation
…ontrolled X gates into one- and two-qubit gates.
…f multi-controlled Z gates.
simon1hofmann
added
enhancement
…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. |
📝 WalkthroughWalkthroughAdds a configurable pass that decomposes multi-controlled X/Z ChangesMulti-controlled gate decomposition pass
Sequence Diagram(s)sequenceDiagram
participant mqt-cc
participant QuantumCompilerConfig
participant QuantumCompilerPipeline as QuantumCompilerPipeline::runPipeline
participant DecomposeMultiControlled as DecomposeMultiControlled pass
participant synthesizeMcx
participant synthesizeMcz
mqt-cc->>QuantumCompilerConfig: set enableDecomposeMultiControlled and min-controls
QuantumCompilerPipeline->>QuantumCompilerConfig: validate min-controls >= 2
QuantumCompilerPipeline->>DecomposeMultiControlled: createDecomposeMultiControlled(options)
DecomposeMultiControlled->>synthesizeMcx: rewrite qco.ctrl with qco.x body
DecomposeMultiControlled->>synthesizeMcz: rewrite qco.ctrl with qco.z body
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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: 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.
|
Signed-off-by: simon1hofmann <119581649+simon1hofmann@users.noreply.github.com>
Signed-off-by: simon1hofmann <119581649+simon1hofmann@users.noreply.github.com>
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.
Finally found some time to go through this.
Really cool to see this implemented and have it as part of our compiler collection!
I have a couple of comments here that are more questions on the general design. The actual implementation seems to be fine as confirmed by the tests.
I really like these kinds of tests because this also means that we can heavily rely on agentic AI to actually improve the implementation efficiency as long as the output is still correct 🙌
| [[nodiscard]] static qc::QuantumComputation | ||
| funcOpToQuantumComputation(func::FuncOp funcOp, std::size_t& numQubits) { |
There was a problem hiding this comment.
I generally really like the verification via the DD package. As such, this kind of translation is necessary at the moment.
I see at least two potential directions for follow-ups here:
- make this a more general feature that we can convert programs to quantum computations. Might require too much work for too little benefit.
- generalize the DD package so that it can also work with the MLIR code base. While it might not be completely possible to detangle the DD package from the old IR module, a large part of the functionality is actually independent. We could potentially port the simulate and buildFunctionality functions over to MLIR programs with only moderate effort. That would avoid going through the QuantumComputation class for tests and computations like these.
| EXPECT_TRUE(dd->multiply(decomposedDD, dd->conjugateTranspose(referenceDD)) | ||
| .isIdentity(/*upToGlobalPhase=*/false)); |
There was a problem hiding this comment.
I suppose that numerical accuracy is a problem here so that. Simple comparison of the resulting DDs (EXPECT_EQ) is not sufficient here and fails, right?
There was a problem hiding this comment.
EXPECT_EQ checks structural equivalence right? In our case, we compare the decomposed mcx/mcz (decomposedDD) with an undecomposed reference mcx/mcz (referenceDD).
There was a problem hiding this comment.
Yeah, but since DDs are canonical, the respective pointers and weights should actually be equivalent (in the literal sense).
In many practical cases, this doesn't hold because of numerical inaccuracies that get in the way. Then, something like the computation that is in the code at the moment is necessary.
There was a problem hiding this comment.
This might once more just be me, but is there really any value in having a dedicated header file for these functions? Couldn't these simply be part of the respective pass CPP file?
It feels like these are very particular function that do not have any public API usage or at least none that I could foresee.
Would you agree?
Edit: having gone through more of the PR, I think I just don't quite like the idea that so much of the pass logic is actively decoupled from MLIR. For the most part, we have the MLIR pass logic and that then builds some auxiliary data structures, which are passed to a collection of methods that doesn't even really care about MLIR.
I would have thought/hoped that we could integrate the respective functionality a little tighter with MLIR itself and avoid auxiliary boilerplate as much as possible.
However, I can also see that the current design also has its pro's; decoupling the infrastructure from the actual pass logic.
Maybe this is just me leaning towards the tightly integrated solution.
Tagging @munich-quantum-toolkit/mqt-cc here for visibility. Maybe someone has a comment or a different perspective
| void c3x() { | ||
| h(3); | ||
| p(0, K_PI8); | ||
| p(1, K_PI8); | ||
| p(2, K_PI8); | ||
| p(3, K_PI8); | ||
| cx(0, 1); | ||
| p(1, -K_PI8); | ||
| cx(0, 1); | ||
| cx(1, 2); | ||
| p(2, -K_PI8); | ||
| cx(0, 2); | ||
| p(2, K_PI8); | ||
| cx(1, 2); | ||
| p(2, -K_PI8); | ||
| cx(0, 2); | ||
| cx(2, 3); | ||
| p(3, -K_PI8); | ||
| cx(1, 3); | ||
| p(3, K_PI8); | ||
| cx(2, 3); | ||
| p(3, -K_PI8); | ||
| cx(0, 3); | ||
| p(3, K_PI8); | ||
| cx(2, 3); | ||
| p(3, -K_PI8); | ||
| cx(1, 3); | ||
| p(3, K_PI8); | ||
| cx(2, 3); | ||
| p(3, -K_PI8); | ||
| cx(0, 3); | ||
| h(3); | ||
| } |
There was a problem hiding this comment.
This is just a fixed decomposition for a three-controlled X gate, right?
Maybe this also fits well with the other suggestion of making this an explicit special case of the pattern instead of hard-coding it here.
| private: | ||
| [[nodiscard]] std::size_t wireIndex(std::size_t local) const { | ||
| return remap_.empty() ? local : remap_[local]; | ||
| } | ||
|
|
||
| [[nodiscard]] Value wire(std::size_t local) const { | ||
| return (*wires_)[wireIndex(local)]; | ||
| } | ||
|
|
||
| void setWire(std::size_t local, Value value) { | ||
| (*wires_)[wireIndex(local)] = value; | ||
| } | ||
|
|
||
| OpBuilder* builder_; | ||
| Location loc_; | ||
| SmallVector<Value>* wires_; | ||
| ArrayRef<std::size_t> remap_; | ||
| }; |
There was a problem hiding this comment.
These feel like generally useful helpers. Is there any good place we could put these? Are there other places where this could turn out to become useful (synthesis for example)?
It's totally fine to keep this here for now. I'd just like to avoid a situation where every pass invents its own helper routines.
Edit: this kind of feels like our program builder to some degree. Could it be generalized so that it is also genuinely useful here?
| static void addResetGadget(GateEmitter& builder, std::size_t q0, std::size_t q1, | ||
| std::size_t q2) { | ||
| builder.cx(q1, q2); | ||
| builder.t(q2); | ||
| builder.cx(q0, q2); | ||
| builder.tdg(q2); | ||
| builder.h(q2); | ||
| } |
There was a problem hiding this comment.
Does it make sense to model this as inv(action)? Now that we allow multiple unitaries in a modifier, this feels like quite a natural way to express this.
| const std::size_t controlsEnd = numControls; | ||
|
|
||
| SmallVector<std::size_t, 16> map; | ||
| const auto relativeStep = [&](const double sign, const std::size_t begin, |
There was a problem hiding this comment.
Kind of feels wrong that sign is not a bool here.
| gate of the target). | ||
| }]; | ||
| let options = [Option< | ||
| "minControls", "min-controls", "uint64_t", "2", |
There was a problem hiding this comment.
IIRC tablegen also allows to directly place constraints on options/parameters. Maybe it is worth looking into whether this would simplify some of the error handling code throughout this PR.
2 participants
Description
decompose-multi-controlledQCO pass that decomposes multi-controlled Pauli-X and Pauli-Z gates (qco.ctrlwith a singleqco.xorqco.zbody) intoh,t,tdg,p, andcxusing the Huang–Palsberg (PLDI 2024) no-ancilla algorithm (adapted from Qiskitsynth_mcx_noaux_hp24).MCZ = H·MCX·H, with redundant Hadamard bookends canceled for k ≥ 2).mqt-cc --decompose-multi-controlledandQuantumCompilerConfig::enableDecomposeMultiControlled(opt-in, default off).min-controlsoption (default2) keeps single-control gates (CX,CZ) intact; configurable viamqt-cc --decompose-multi-controlled-min-controls.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.