Merged
Conversation
denialhaag
added
enhancement
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRenames MLIR conversion targets to MLIR-prefixed names, replaces macro-based conversion patterns with template-based converter structs across conversion passes, switches many op constructions to static Op::create(...) factories, and updates CMakeLists, unit-test linkages, and documentation includes. Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Source IR (Jeff / QC)
participant JeffPass as MLIRJeffToQCO
participant QCOPass as MLIRQCOToQC / MLIRQCOToJeff
participant QIRPass as MLIRQCToQIR
participant Pipeline as MQTCompilerPipeline
Source->>JeffPass: match ops, apply template patterns
JeffPass->>QCOPass: emit QCO/QC ops (stateful mappings)
QCOPass->>QIRPass: lower to QIR calls / LLVM::CallOps
Pipeline->>JeffPass: register patterns (MLIR target)
Pipeline->>QCOPass: register patterns (MLIR target)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
mlir/lib/Conversion/QCOToQC/CMakeLists.txt (1)
25-25:⚠️ Potential issue | 🟡 MinorTarget name mismatch in
mqt_mlir_target_use_project_options.Same issue as in
QCToQCO/CMakeLists.txt: the library target is nowMLIRQCOToQC, but this call still referencesQCOToQC.Proposed fix
-mqt_mlir_target_use_project_options(QCOToQC) +mqt_mlir_target_use_project_options(MLIRQCOToQC)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Conversion/QCOToQC/CMakeLists.txt` at line 25, The call to mqt_mlir_target_use_project_options currently references the old target name QCOToQC; update this to the new library target MLIRQCOToQC (i.e., replace the argument in mqt_mlir_target_use_project_options to MLIRQCOToQC) so the project options are applied to the correct target defined for this directory.mlir/lib/Conversion/QCToQCO/CMakeLists.txt (1)
25-25:⚠️ Potential issue | 🟡 MinorUpdate
mqt_mlir_target_use_project_optionsto match the actual MLIR-prefixed target name.The library target created by
add_mlir_conversion_libraryon line 12 isMLIRQCToQCO, but the call on line 25 referencesQCToQCO. This causes the function to silently fail to find the target and skip applying project options.Proposed fix
-mqt_mlir_target_use_project_options(QCToQCO) +mqt_mlir_target_use_project_options(MLIRQCToQCO)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Conversion/QCToQCO/CMakeLists.txt` at line 25, The call to mqt_mlir_target_use_project_options uses the wrong target name (QCToQCO) so it doesn't apply project options; update that call to use the actual MLIR target name MLIRQCToQCO (the target created by add_mlir_conversion_library) by replacing the QCToQCO argument with MLIRQCToQCO so mqt_mlir_target_use_project_options can locate and apply the options to the correct target.mlir/lib/Conversion/QCOToQC/QCOToQC.cpp (1)
757-814:⚠️ Potential issue | 🔴 CriticalRe-add the SCF conversion patterns before applying this pass.
This registration block no longer installs any
ConvertQCOScf*patterns orscf::*legality rules.populateBranchOpInterfaceTypeConversionPatternonly covers branch-like ops, notscf.if/scf.while/scf.for, so all-qubit SCF regions that this pass previously handled will now be left behind or accepted with!qco.qubittypes still embedded in legalscfops.Please restore the dedicated
ConvertQCOScfYieldOp,ConvertQCOScfConditionOp,ConvertQCOScfIfOp,ConvertQCOScfWhileOp, andConvertQCOScfForOpregistrations together with the corresponding dynamic legality checks. Based on learnings: ensuremlir/lib/Conversion/QCOToQC/QCOToQC.cppkeeps dedicated all-qubitConvertQCOScf*patterns for SCF conversion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Conversion/QCOToQC/QCOToQC.cpp` around lines 757 - 814, The SCF conversion patterns were removed and must be re-added: register ConvertQCOScfYieldOp, ConvertQCOScfConditionOp, ConvertQCOScfIfOp, ConvertQCOScfWhileOp, and ConvertQCOScfForOp into the same patterns.add<...>(typeConverter, context) call (or immediately after) so SCF regions get converted; also add dynamic legality checks for scf.if, scf.for, scf.while and scf.yield (using target.addDynamicallyLegalOp with lambdas that call typeConverter.isLegal(...) or isSignatureLegal(...) as appropriate) before calling applyPartialConversion so those ops are treated as illegal until converted (do not rely solely on populateBranchOpInterfaceTypeConversionPattern).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp`:
- Line 439: Change the inconsistent use of this->getState() in the
ConvertQCOSXOpToJeff class to match the file's style by calling getState()
without the this-> qualifier; locate the occurrence in ConvertQCOSXOpToJeff
(currently using this->getState()) and replace it with getState(), keeping
behavior unchanged and aligning with other converters like
ConvertQCOSXdgOpToJeff and ConvertQCOiSWAPOpToJeff.
In `@mlir/lib/Conversion/QCToQIR/CMakeLists.txt`:
- Around line 11-12: The target created by add_mlir_conversion_library was
renamed to MLIRQCToQIR, but the call
mqt_mlir_target_use_project_options(QCToQIR) still references the old target
name; update the call to mqt_mlir_target_use_project_options to use MLIRQCToQIR
(i.e., replace QCToQIR with MLIRQCToQIR) so the CMake target names match (check
add_mlir_conversion_library and the mqt_mlir_target_use_project_options
invocation).
---
Outside diff comments:
In `@mlir/lib/Conversion/QCOToQC/CMakeLists.txt`:
- Line 25: The call to mqt_mlir_target_use_project_options currently references
the old target name QCOToQC; update this to the new library target MLIRQCOToQC
(i.e., replace the argument in mqt_mlir_target_use_project_options to
MLIRQCOToQC) so the project options are applied to the correct target defined
for this directory.
In `@mlir/lib/Conversion/QCOToQC/QCOToQC.cpp`:
- Around line 757-814: The SCF conversion patterns were removed and must be
re-added: register ConvertQCOScfYieldOp, ConvertQCOScfConditionOp,
ConvertQCOScfIfOp, ConvertQCOScfWhileOp, and ConvertQCOScfForOp into the same
patterns.add<...>(typeConverter, context) call (or immediately after) so SCF
regions get converted; also add dynamic legality checks for scf.if, scf.for,
scf.while and scf.yield (using target.addDynamicallyLegalOp with lambdas that
call typeConverter.isLegal(...) or isSignatureLegal(...) as appropriate) before
calling applyPartialConversion so those ops are treated as illegal until
converted (do not rely solely on
populateBranchOpInterfaceTypeConversionPattern).
In `@mlir/lib/Conversion/QCToQCO/CMakeLists.txt`:
- Line 25: The call to mqt_mlir_target_use_project_options uses the wrong target
name (QCToQCO) so it doesn't apply project options; update that call to use the
actual MLIR target name MLIRQCToQCO (the target created by
add_mlir_conversion_library) by replacing the QCToQCO argument with MLIRQCToQCO
so mqt_mlir_target_use_project_options can locate and apply the options to the
correct target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f48e9f0-c0ea-4e07-8911-d5d57a9b5655
📒 Files selected for processing (21)
docs/mlir/Conversions.mddocs/mlir/QCO.mdmlir/lib/Compiler/CMakeLists.txtmlir/lib/Conversion/JeffToQCO/CMakeLists.txtmlir/lib/Conversion/JeffToQCO/JeffToQCO.cppmlir/lib/Conversion/QCOToJeff/CMakeLists.txtmlir/lib/Conversion/QCOToJeff/QCOToJeff.cppmlir/lib/Conversion/QCOToQC/CMakeLists.txtmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/CMakeLists.txtmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Conversion/QCToQIR/CMakeLists.txtmlir/lib/Conversion/QCToQIR/QCToQIR.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/BarrierOp.cppmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/unittests/Conversion/JeffRoundTrip/CMakeLists.txtmlir/unittests/Conversion/QCOToQC/CMakeLists.txtmlir/unittests/Conversion/QCToQCO/CMakeLists.txtmlir/unittests/Conversion/QCToQIR/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Mapping/CMakeLists.txt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Conversion/QCOToJeff/QCOToJeff.cpp`:
- Around line 366-418: The four ConvertQCO... template matchAndRewrite
implementations duplicate the same modifier/result bookkeeping (checking
state.inModifier()/selecting target from state.targetsIn, choosing between
rewriter.replaceOp vs rewriter.eraseOp and assigning state.targetsOut, and
setting state.controlsOut from jeffOp.getOutCtrlQubits()); factor that into one
or two small helpers (e.g., handleSingleTargetPostprocess(StateType&, Operation*
originalOp, Value outQubit, ArrayRef<Value> outCtrlQubits) and a two-target
variant) and call them from
ConvertQCOOneTargetZeroParameterToJeff::matchAndRewrite (and the other three
template families referenced). Ensure the helpers use state.inModifier(),
state.targetsIn, rewriter.replaceOp/eraseOp, state.targetsOut, and
state.controlsOut to preserve current behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 209325b7-208f-466b-9d18-e25f4573b5c3
📒 Files selected for processing (6)
CHANGELOG.mdmlir/lib/Conversion/QCOToJeff/QCOToJeff.cppmlir/lib/Conversion/QCOToQC/CMakeLists.txtmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/CMakeLists.txtmlir/lib/Conversion/QCToQIR/CMakeLists.txt
burgholzer
approved these changes
Mar 10, 2026
Member
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
This looks great! I think the one suggestion by the rabbit might make sense. But then this can go in 🙌
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced Mar 11, 2026
Merged
8 tasks
This was referenced Apr 7, 2026
Merged
11 tasks
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
2 participants
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
This PR streamlines the conversions in several aspects:
add_mlir_conversion_library()instead ofadd_mlir_library()MLIRrewriter.create<Op>(...)with `Op::create(rewriter, ...)Checklist:
I have added appropriate tests that cover the new/changed functionality.I have updated the documentation to reflect these changes.I have added migration instructions to the upgrade guide (if needed).