✨ Add an unroll-modifiers pass for unrolling multi-operation modifiers - #2015
Open
denialhaag wants to merge 9 commits into
Open
✨ Add an unroll-modifiers pass for unrolling multi-operation modifiers#2015denialhaag wants to merge 9 commits into
unroll-modifiers pass for unrolling multi-operation modifiers#2015denialhaag wants to merge 9 commits into
Conversation
Assisted-by: Claude Opus 5 via Claude Code
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded the public ChangesModifier unrolling and conversion support
Estimated code review effort: 4 (Complex) | ~45 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)
✨ 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
Assisted-by: Claude Opus 5 via Claude Code
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Assisted-by: Claude Opus 5 via Claude Code
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
denialhaag
changed the title
unroll-modifiers pass for multi-operation modifiersunroll-modifiers pass for unrolling multi-operation modifiers
Assisted-by: Claude Opus 5 via Claude Code
denialhaag
marked this pull request as ready for review
August 7, 2026 13:14
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mlir/include/mlir/Dialect/Utils/Transforms/Passes.td (1)
45-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win🤖 AI text below 🤖
Correct the pass limitation and typo.
hoistClassicalOpsleaves the modifier unchanged if a classical operation is impure or depends on a value defined in the body. State this condition. Replaceintouchedwithuntouched.As per coding guidelines, MLIR pass descriptions must align with implemented limitations.
Proposed documentation update
- Classical operations of the body are moved in front of the new modifiers. + Pure classical operations that do not depend on values defined in the body + are moved in front of the new modifiers. Otherwise, the modifier is left + untouched. - If a modifier cannot be unrolled, the modifier is left intouched. Such + If a modifier cannot be unrolled, the modifier is left untouched. Such🤖 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/Utils/Transforms/Passes.td` around lines 45 - 48, Update the pass description for hoistClassicalOps to state that modifiers remain unchanged when classical operations are impure or depend on values defined in the body, and replace “intouched” with “untouched.”Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@mlir/include/mlir/Dialect/Utils/Transforms/Passes.td`:
- Around line 45-48: Update the pass description for hoistClassicalOps to state
that modifiers remain unchanged when classical operations are impure or depend
on values defined in the body, and replace “intouched” with “untouched.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a2d21c2f-9695-4a07-a806-ddf468cba053
📒 Files selected for processing (11)
mlir/include/mlir/Dialect/Utils/Transforms/Passes.tdmlir/lib/Dialect/QC/IR/Modifiers/ModifierUtils.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/PowOp.cppmlir/lib/Dialect/Utils/Transforms/UnrollModifiers.cppmlir/unittests/Conversion/JeffRoundTrip/test_jeff_round_trip.cppmlir/unittests/Dialect/QC/IR/test_qc_ir.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qco_programs.h
💤 Files with no reviewable changes (1)
- mlir/unittests/Dialect/QCO/IR/test_qco_ir.cpp
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 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).