✨ Add an unroll-modifiers pass for multi-operation modifiers - #2015
Draft
denialhaag wants to merge 3 commits into
Draft
✨ Add an unroll-modifiers pass for multi-operation modifiers#2015denialhaag wants to merge 3 commits into
unroll-modifiers pass for multi-operation modifiers#2015denialhaag wants to merge 3 commits into
Conversation
Assisted-by: Claude Opus 5 via Claude Code
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded the public ChangesModifier unrolling and conversion support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ModuleOp
participant UnrollModifiers
participant ConversionPipeline
ModuleOp->>UnrollModifiers: Process nested ctrl and inv modifiers
UnrollModifiers->>ModuleOp: Clone remapped unitary modifiers
ModuleOp->>ConversionPipeline: Convert unrolled operations
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/include/mlir/Dialect/Utils/Transforms/Passes.td`:
- Around line 40-51: Update the Passes.td description for the relevant pass to
document all implemented limitations: classical operations remain inside the
modifier when they are impure or use values defined in the body, ctrl/inv
modifiers remain unchanged when body unitaries do not thread every distinct
qubit operand to a result, and unsupported modifiers are skipped without
diagnostics. Retain the existing pow behavior and nested-modifier scope.
In `@mlir/lib/Dialect/Utils/Transforms/UnrollModifiers.cpp`:
- Around line 35-41: Add static linkage to the free functions getBodyUnitaries,
getQubitOperands, hoistClassicalOps, cloneIntoBody, hasThreadedBodyUnitaries,
and all four unrollModifier overloads. Remove these functions from the anonymous
namespace, while retaining only the UnrollModifiers struct there.
- Around line 251-256: Update the modifier loop’s `unrollModifier` handling to
inspect its result instead of discarding it, and emit a remark or `LLVM_DEBUG`
diagnostic when unrolling is skipped, including the returned reason. Keep
successful unrolling behavior unchanged and ensure the diagnostic identifies the
affected modifier.
- Around line 193-197: In the result-building logic around the qubits mapping
and qco::CtrlOp::create, replace unchecked qubits.lookup calls with checked
lookups that assert each yielded value has a non-null mapped Value before
appending or using it. Fail fast on missing mappings, including the lookup used
for replacement operands before rewriter.replaceOp.
🪄 Autofix
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: 42503a67-d662-4e1e-bff8-bebe17f8b3c5
📒 Files selected for processing (22)
mlir/include/mlir/Conversion/QCToQIR/QIRCommon/QIRCommon.hmlir/include/mlir/Dialect/Utils/Transforms/Passes.tdmlir/lib/Compiler/Programs.cppmlir/lib/Conversion/QCToQIR/QIRCommon/QIRCommon.cppmlir/lib/Dialect/Utils/Transforms/CMakeLists.txtmlir/lib/Dialect/Utils/Transforms/UnrollModifiers.cppmlir/lib/Support/Passes.cppmlir/tools/mqt-cc/mqt-cc.cppmlir/unittests/Conversion/JeffRoundTrip/CMakeLists.txtmlir/unittests/Conversion/JeffRoundTrip/test_jeff_round_trip.cppmlir/unittests/Conversion/QCToQIR/QCToQIRAdaptive/CMakeLists.txtmlir/unittests/Conversion/QCToQIR/QCToQIRAdaptive/test_qc_to_qir_adaptive.cppmlir/unittests/Conversion/QCToQIR/QCToQIRBase/CMakeLists.txtmlir/unittests/Conversion/QCToQIR/QCToQIRBase/test_qc_to_qir_base.cppmlir/unittests/Dialect/QC/IR/CMakeLists.txtmlir/unittests/Dialect/QC/IR/test_qc_ir.cppmlir/unittests/Dialect/QCO/IR/CMakeLists.txtmlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/programs/qc_programs.cppmlir/unittests/programs/qc_programs.hmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qco_programs.h
Comment on lines
+251
to
+256
| for (auto* modifier : modifiers) { | ||
| llvm::TypeSwitch<Operation*>(modifier) | ||
| .Case<qc::CtrlOp, qc::InvOp, qco::CtrlOp, qco::InvOp>([&](auto op) { | ||
| static_cast<void>(unrollModifier(op, rewriter)); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Report skipped modifiers.
The result of unrollModifier is discarded. A modifier that cannot be unrolled stays in the IR without any signal. The failure then appears much later as a generic legalization error in the QC-to-QIR or QCO-to-jeff conversion. Emit a remark or a LLVM_DEBUG message with the reason so that the skip is traceable.
🤖 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/Utils/Transforms/UnrollModifiers.cpp` around lines 251 -
256, Update the modifier loop’s `unrollModifier` handling to inspect its result
instead of discarding it, and emit a remark or `LLVM_DEBUG` diagnostic when
unrolling is skipped, including the returned reason. Keep successful unrolling
behavior unchanged and ensure the diagnostic identifies the affected modifier.
Assisted-by: Claude Opus 5 via Claude Code
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
🤖 AI text below 🤖
Since #1751, modifier bodies may contain more than one unitary operation. This adds an$(ab)^{-1} = b^{-1} a^{-1}$ . Classical operations are moved in front of the modifier; a modifier is left untouched when one of them is impure or depends on the body's qubits. $(ab)^r$ is generally not equal to $a^r b^r$ — modifiers nested inside a
unroll-modifierspass that splits suchctrlandinvmodifiers into a sequence of modifiers holding a single operation each. The targets of the new modifiers are narrowed to the qubits of the respective operation (and rewired through the SSA chain in QCO), and the order of the operations is reversed forinv, sincepowmodifiers are left untouched as well, becausepowbody are still unrolled. Following #1986, the pass is shared between QC and QCO and lives inmlir::mqt. It now runs before the QCO-to-jeffand QC-to-QIR conversions, which cannot fully handle multi-operation modifiers.Fixes #1758
AI Notice
The first version of the changes was assisted by Opus 5 via Claude Code. I have subsequently iterated on the changes.
Checklist
I have added migration instructions to the upgrade guide (if needed).If PR contains AI-assisted content:
🤖 *AI text below* 🤖(titles are exempt).