🎨 Use custom builder classes in ctrl methods
#1402
Merged
Conversation
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
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
📝 WalkthroughWalkthroughThe PR narrows Ctrl callback signatures: QC's ctrl body now takes no arguments ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/unittests/pipeline/test_compiler_pipeline.cpp
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp:187-210
Timestamp: 2025-12-14T17:02:02.997Z
Learning: In the Flux dialect builder (mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp), the coding style relies on implicit conversion from Value to ValueRange in ctrl() calls (e.g., `ctrl(control, {}, ...)` instead of `ctrl(ValueRange{control}, ValueRange{}, ...)`). This pattern is used consistently throughout all macro-generated methods and should be maintained for consistency.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1383
File: python/mqt/core/ir/operations.pyi:424-456
Timestamp: 2025-12-15T15:17:25.469Z
Learning: The Control class in munich-quantum-toolkit/core should be immutable (no setters for qubit and type_ properties) because Control objects are used in sets throughout the API (controls property, add_controls, remove_controls methods), and hashable+mutable objects in sets lead to incorrect lookups.
📚 Learning: 2025-12-14T17:02:02.997Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp:187-210
Timestamp: 2025-12-14T17:02:02.997Z
Learning: In the Flux dialect builder (mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp), the coding style relies on implicit conversion from Value to ValueRange in ctrl() calls (e.g., `ctrl(control, {}, ...)` instead of `ctrl(ValueRange{control}, ValueRange{}, ...)`). This pattern is used consistently throughout all macro-generated methods and should be maintained for consistency.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h
📚 Learning: 2025-12-08T23:58:09.648Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/unittests/pipeline/test_compiler_pipeline.cpp
📚 Learning: 2025-12-08T12:44:05.883Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h
📚 Learning: 2025-12-08T14:55:43.899Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.899Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h
📚 Learning: 2025-12-08T23:16:20.680Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-101
Timestamp: 2025-12-08T23:16:20.680Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The CtrlInlineGPhase canonicalization pattern correctly only checks getNumPosControls() and processes only positive controls when inlining a GPhaseOp.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h
📚 Learning: 2025-12-08T23:44:39.930Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/PhaseOp.cpp:0-0
Timestamp: 2025-12-08T23:44:39.930Z
Learning: In MLIR code under any mlir/ directory, avoid using const qualifiers on core MLIR types in function parameters/signatures (e.g., Value, Type, Attribute, Operation*, Block*, Region*, etc.). This aligns with MLIR's design rationale and should be applied to C++ source files (e.g., .cpp) within mlir/; see https://mlir.llvm.org/docs/Rationale/UsageOfConst/ for details.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/unittests/pipeline/test_compiler_pipeline.cpp
📚 Learning: 2025-12-17T11:32:45.843Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:83-86
Timestamp: 2025-12-17T11:32:45.843Z
Learning: In the mlir portion of munich-quantum-toolkit/core, prefer marking free functions as static (static linkage) over placing them in anonymous namespaces, per the clang-tidy rule llvm-prefer-static-over-anonymous-namespace. Do not apply this to type/class definitions; they may continue to use anonymous namespaces. This guideline should be checked across C++ source files under mlir/ (e.g., any free function in LayeredUnit.cpp) to ensure free functions have static linkage, while types/classes can remain in anonymous namespaces.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/unittests/pipeline/test_compiler_pipeline.cpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
mlir/unittests/pipeline/test_compiler_pipeline.cpp
🧬 Code graph analysis (3)
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp (5)
mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp (2)
checkFinalized(454-458)checkFinalized(454-454)mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp (2)
checkFinalized(570-575)checkFinalized(570-570)mlir/lib/Dialect/QC/Translation/TranslateQuantumComputationToQC.cpp (1)
builder(608-608)mlir/lib/Conversion/QCToQIR/QCToQIR.cpp (1)
builder(1056-1056)mlir/lib/Dialect/MQTRef/Translation/ImportQuantumComputation.cpp (1)
builder(619-619)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.h (1)
mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp (1)
QCProgramBuilder(34-38)
mlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h (1)
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp (1)
QCOProgramBuilder(36-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: 🐉 Test / 🐧 ubuntu-24.04-arm
- GitHub Check: 🐉 Test / 🐧 Coverage
- GitHub Check: 🐉 Test / 🍎 macos-15-intel
- GitHub Check: 🐉 Test / 🏁 windows-2025
- GitHub Check: 🐉 Test / 🐧 ubuntu-24.04
- GitHub Check: 🐉 Test / 🏁 windows-11-arm
- GitHub Check: 🐉 Test / 🍎 macos-15
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: Run
🔇 Additional comments (4)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.h (1)
844-845: QCProgramBuilder::ctrl API tightening looks goodExposing
QCProgramBuilder&in the callback matches actual usage and keeps existingOpBuilder&/auto&lambdas source‑compatible via inheritance. No issues from the declaration side.mlir/unittests/pipeline/test_compiler_pipeline.cpp (1)
1584-1588: MCXNested test adapts cleanly to new ctrl signatureUsing
mlir::qc::QCProgramBuilder&in the lambda matches the updated API and removes the downcast; this is a straightforward and correct adjustment.mlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.h (1)
1002-1004: QCOProgramBuilder::ctrl signature change is consistent and usefulPassing
QCOProgramBuilder&into the body matches the value‑semantics builder API and aligns with the implementation; existingOpBuilder&-typed lambdas remain usable via inheritance.mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp (1)
420-425: QCProgramBuilder::ctrl wrapper is correct; optional refinement should be disregardedThe current implementation correctly adapts the user-facing callback signature (
std::function<void(QCProgramBuilder&)>) to CtrlOp::create's internal callback signature (OpBuilder&). Using*thisto pass the QCProgramBuilder to the user's body function is the correct pattern—it provides the high-level builder interface that users expect.The suggested explicit cast refinement should not be applied. The OpBuilder passed to the lambda is a lower-level MLIR construct, not a QCProgramBuilder, and attempting to cast it would be incorrect. The current implementation is clean and correct as-is.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp (1)
579-588: Consider using the region's OpBuilder parameter instead of capturingthis.The wrapper lambda captures
thisand passes*thisto the body while ignoring theOpBuilder¶meter provided byCtrlOp::create. This approach differs from the pattern used in the macros throughout this file (e.g., lines 213-215, 251-253), where the providedOpBuilder& bparameter is used directly to create operations in the control region.As noted in the past review comment, this pattern obscures which builder instance is being used. Consider casting the region's OpBuilder to the concrete type:
std::pair<ValueRange, ValueRange> QCOProgramBuilder::ctrl( ValueRange controls, ValueRange targets, const std::function<ValueRange(QCOProgramBuilder&, ValueRange)>& body) { checkFinalized(); - auto ctrlOp = - CtrlOp::create(*this, loc, controls, targets, - [this, &body](OpBuilder& /*builder*/, ValueRange targets) { - return body(*this, targets); - }); + auto ctrlOp = CtrlOp::create( + *this, loc, controls, targets, + [&body](OpBuilder& builder, ValueRange targets) { + auto& qcoBuilder = static_cast<QCOProgramBuilder&>(builder); + return body(qcoBuilder, targets); + });This ensures the body operates on the region's builder with the correct insertion point, aligning with the macro patterns and making the builder identity explicit.
Based on learnings from past reviews and consistency with macro patterns in this file.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp:187-210
Timestamp: 2025-12-14T17:02:02.997Z
Learning: In the Flux dialect builder (mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp), the coding style relies on implicit conversion from Value to ValueRange in ctrl() calls (e.g., `ctrl(control, {}, ...)` instead of `ctrl(ValueRange{control}, ValueRange{}, ...)`). This pattern is used consistently throughout all macro-generated methods and should be maintained for consistency.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
📚 Learning: 2025-12-14T17:02:02.997Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp:187-210
Timestamp: 2025-12-14T17:02:02.997Z
Learning: In the Flux dialect builder (mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp), the coding style relies on implicit conversion from Value to ValueRange in ctrl() calls (e.g., `ctrl(control, {}, ...)` instead of `ctrl(ValueRange{control}, ValueRange{}, ...)`). This pattern is used consistently throughout all macro-generated methods and should be maintained for consistency.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-08T23:58:09.648Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-08T12:44:05.883Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-08T14:55:43.899Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.899Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-08T23:16:20.680Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-101
Timestamp: 2025-12-08T23:16:20.680Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The CtrlInlineGPhase canonicalization pattern correctly only checks getNumPosControls() and processes only positive controls when inlining a GPhaseOp.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-02T07:37:46.860Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp
📚 Learning: 2025-12-08T23:41:55.972Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp:96-117
Timestamp: 2025-12-08T23:41:55.972Z
Learning: In the QIR (Quantum Intermediate Representation) Builder (mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp), the `ptrCache` is intentionally shared between qubit and result pointer creation (in `staticQubit()` and `measure()` methods) because QIR uses opaque pointers and `inttoptr` conversions for both qubits and results. For any given index N, the LLVM IR pointer representation is identical whether it represents a qubit or a result, so the pointer only needs to be created once and can be safely reused across both contexts.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp
📚 Learning: 2025-12-08T23:44:39.930Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/PhaseOp.cpp:0-0
Timestamp: 2025-12-08T23:44:39.930Z
Learning: In MLIR code under any mlir/ directory, avoid using const qualifiers on core MLIR types in function parameters/signatures (e.g., Value, Type, Attribute, Operation*, Block*, Region*, etc.). This aligns with MLIR's design rationale and should be applied to C++ source files (e.g., .cpp) within mlir/; see https://mlir.llvm.org/docs/Rationale/UsageOfConst/ for details.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-17T11:32:45.843Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:83-86
Timestamp: 2025-12-17T11:32:45.843Z
Learning: In the mlir portion of munich-quantum-toolkit/core, prefer marking free functions as static (static linkage) over placing them in anonymous namespaces, per the clang-tidy rule llvm-prefer-static-over-anonymous-namespace. Do not apply this to type/class definitions; they may continue to use anonymous namespaces. This guideline should be checked across C++ source files under mlir/ (e.g., any free function in LayeredUnit.cpp) to ensure free functions have static linkage, while types/classes can remain in anonymous namespaces.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: 🐉 Test / 🐧 ubuntu-24.04
- GitHub Check: 🐉 Test / 🐧 Coverage
- GitHub Check: 🐉 Test / 🏁 windows-11-arm
- GitHub Check: 🐉 Test / 🍎 macos-15-intel
- GitHub Check: 🐉 Test / 🍎 macos-15
- GitHub Check: 🐉 Test / 🏁 windows-2025
- GitHub Check: 🐉 Test / 🐧 ubuntu-24.04-arm
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: Run
Signed-off-by: burgholzer <[email protected]>
Member
|
@denialhaag I tried something a little different in bf63548 and I think I really like it. Let me know what you think. |
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 (1)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.td (1)
1097-1103: Clarify ValueRange lifetime expectations for CtrlOp bodyBuilderThe new builder overload
OpBuilder<(ins "ValueRange":$controls, "ValueRange":$targets, "const std::function<ValueRange(ValueRange)>&":$bodyBuilder)>
nicely reflects value semantics (targets in, targets out). However, becauseValueRangeis a view, callers must ensure the returned range refers to stable values (e.g., operation results or existing ranges), not to temporaries such as stack-localSmallVector<Value>or initializer lists whose storage goes out of scope when the lambda returns. It would be helpful to document this contract at the QCO builder level or wrap this in a small helper that builds theYieldOpfrom a concrete container to make misuse harder.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/unittests/pipeline/test_compiler_pipeline.cpp
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp:187-210
Timestamp: 2025-12-14T17:02:02.997Z
Learning: In the Flux dialect builder (mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp), the coding style relies on implicit conversion from Value to ValueRange in ctrl() calls (e.g., `ctrl(control, {}, ...)` instead of `ctrl(ValueRange{control}, ValueRange{}, ...)`). This pattern is used consistently throughout all macro-generated methods and should be maintained for consistency.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
📚 Learning: 2025-12-08T23:58:09.648Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/unittests/pipeline/test_compiler_pipeline.cppmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-14T17:02:02.997Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp:187-210
Timestamp: 2025-12-14T17:02:02.997Z
Learning: In the Flux dialect builder (mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp), the coding style relies on implicit conversion from Value to ValueRange in ctrl() calls (e.g., `ctrl(control, {}, ...)` instead of `ctrl(ValueRange{control}, ValueRange{}, ...)`). This pattern is used consistently throughout all macro-generated methods and should be maintained for consistency.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-08T12:44:05.883Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-08T14:55:43.899Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.899Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-08T23:16:20.680Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-101
Timestamp: 2025-12-08T23:16:20.680Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The CtrlInlineGPhase canonicalization pattern correctly only checks getNumPosControls() and processes only positive controls when inlining a GPhaseOp.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/include/mlir/Dialect/QCO/Builder/QCOProgramBuilder.hmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-17T17:44:21.624Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/include/mlir/Dialect/QCO/IR/QCOOps.td:259-259
Timestamp: 2025-12-17T17:44:21.624Z
Learning: In mlir/include/mlir/Dialect/QCO/IR/QCOOps.td (QCO dialect), ensure GPhaseOp declares MemoryEffects<[MemWrite]> rather than Pure. This op has no results and is a zero-target operation, so using Pure would lead to its removal by dead-code elimination. By marking it with MemWrite, review ensures DCE preserves the operation because it has a meaningful effect on the global quantum state. This guidance applies when reviewing or updating QCO ops in this file (and broadly to similar zero-target ops with side effects in QCO).
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOOps.td
📚 Learning: 2025-12-17T17:44:31.349Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/include/mlir/Dialect/QCO/IR/QCOOps.td:259-259
Timestamp: 2025-12-17T17:44:31.349Z
Learning: In the QCO dialect (mlir/include/mlir/Dialect/QCO/IR/QCOOps.td), GPhaseOp intentionally uses `MemoryEffects<[MemWrite]>` instead of `Pure` to prevent the remove-dead-values pass from eliminating it. Since GPhaseOp is a zero-target operation with no result values, it would otherwise be removed by DCE, even though it has a meaningful effect on the global quantum state.
Applied to files:
mlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp
📚 Learning: 2025-12-02T07:37:46.860Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Applied to files:
mlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-08T23:44:39.930Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/PhaseOp.cpp:0-0
Timestamp: 2025-12-08T23:44:39.930Z
Learning: In MLIR code under any mlir/ directory, avoid using const qualifiers on core MLIR types in function parameters/signatures (e.g., Value, Type, Attribute, Operation*, Block*, Region*, etc.). This aligns with MLIR's design rationale and should be applied to C++ source files (e.g., .cpp) within mlir/; see https://mlir.llvm.org/docs/Rationale/UsageOfConst/ for details.
Applied to files:
mlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/unittests/pipeline/test_compiler_pipeline.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-17T11:32:45.843Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:83-86
Timestamp: 2025-12-17T11:32:45.843Z
Learning: In the mlir portion of munich-quantum-toolkit/core, prefer marking free functions as static (static linkage) over placing them in anonymous namespaces, per the clang-tidy rule llvm-prefer-static-over-anonymous-namespace. Do not apply this to type/class definitions; they may continue to use anonymous namespaces. This guideline should be checked across C++ source files under mlir/ (e.g., any free function in LayeredUnit.cpp) to ensure free functions have static linkage, while types/classes can remain in anonymous namespaces.
Applied to files:
mlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/unittests/pipeline/test_compiler_pipeline.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp
📚 Learning: 2025-12-08T23:41:55.972Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp:96-117
Timestamp: 2025-12-08T23:41:55.972Z
Learning: In the QIR (Quantum Intermediate Representation) Builder (mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp), the `ptrCache` is intentionally shared between qubit and result pointer creation (in `staticQubit()` and `measure()` methods) because QIR uses opaque pointers and `inttoptr` conversions for both qubits and results. For any given index N, the LLVM IR pointer representation is identical whether it represents a qubit or a result, so the pointer only needs to be created once and can be safely reused across both contexts.
Applied to files:
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp
📚 Learning: 2025-10-09T13:13:51.224Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Applied to files:
mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp
📚 Learning: 2025-12-05T17:45:37.602Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
Applied to files:
mlir/unittests/pipeline/test_compiler_pipeline.cpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
mlir/unittests/pipeline/test_compiler_pipeline.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: 🐉 Test / 🐧 ubuntu-24.04
- GitHub Check: 🐉 Test / 🍎 macos-15-intel
- GitHub Check: 🐉 Test / 🐧 Coverage
- GitHub Check: 🐉 Test / 🐧 ubuntu-24.04-arm
- GitHub Check: 🐉 Test / 🏁 windows-2025
- GitHub Check: 🐉 Test / 🍎 macos-15
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: 🐉 Test / 🏁 windows-11-arm
- GitHub Check: Run
🔇 Additional comments (12)
mlir/unittests/pipeline/test_compiler_pipeline.cpp (1)
1584-1587: MCXNested test correctly migrated to new ctrl() APIUsing
b.ctrl(reg[0], [&] { b.cx(reg[1], reg[2]); });matches the newQCProgramBuilder::ctrl(ValueRange, const std::function<void()>&)signature, relies on the intended implicitValue→ValueRangeconversion, and keeps the test’s semantics unchanged while simplifying the lambda body.mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.h (1)
827-846: QCProgramBuilder::ctrl signature and example align with QC CtrlOp semanticsThe updated
ctrl(ValueRange controls, const std::function<void()>& body)plus the examplebuilder.ctrl(q0, [&] { builder.x(q1); });are consistent with QC’s reference semantics and the underlyingqc::CtrlOpbuilder that now also expects a parameterless body callback. The use of implicitValue→ValueRangeconversion for single-qubit controls stays in line with the established builder style.mlir/include/mlir/Dialect/QC/IR/QCOps.td (1)
967-970: QC CtrlOp builder updated consistently to no-arg body callbackSwitching the second
CtrlOpbuilder to
OpBuilder<(ins "ValueRange":$controls, "const std::function<void()>&":$bodyBuilder)>
is consistent with the new QCProgramBuilder API and the implementation inCtrlOp::build, where the callback is invoked with the insertion point already set to the body block andqc::YieldOpcreated afterwards. No functional issues spotted.mlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cpp (1)
97-103: CtrlOp bodyBuilder migration preserves rewrite and build semanticsAdapting both
CtrlInlineGPhaseandCtrlOp::buildto use astd::function<void()>body callback looks correct: the build overload still guards and resets the insertion point, emits the body operations inside the newly created block, and appends aqc::YieldOpafterwards. The updatedCtrlInlineGPhasenow passes a matching parameterless lambda that creates thePOpwith the existingPatternRewriter, so the canonicalization behavior for controlledGPhaseOpremains intact.Also applies to: 162-172
mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp (2)
109-114: LGTM! CtrlInlineGPhase correctly adapted to new signature.The pattern correctly uses the new lambda signature, taking
ValueRange targetsand returning the operation result. The implicit conversion fromValuetoValueRangeis standard practice in MLIR.
261-272: LGTM! Build signature correctly updated.The CtrlOp::build method signature correctly reflects the new callback type
std::function<ValueRange(ValueRange)>, removing theOpBuilder¶meter. The implementation properly invokes the body builder with targets and yields the results.mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp (2)
169-169: LGTM! Macro-generated methods consistently updated.All controlled operation helpers correctly use the new no-parameter lambda signature
[&] { OP_CLASS::create(*this, loc, ...); }. This aligns with QC dialect's reference semantics where the body doesn't return values.The usage of
*thisis correct becauseCtrlOp::buildsets the insertion point on the builder before invoking the body lambda.Based on learnings, QC dialect uses reference semantics and doesn't return values from ctrl operations.
Also applies to: 194-194, 231-232, 265-267, 302-304, 328-329, 359-361, 395-397
418-423: LGTM! ctrl implementation correctly simplified.The ctrl method signature correctly updated to
std::function<void()>, removing the OpBuilder& parameter. The implementation directly passes the body toCtrlOp::create, which is appropriate for the QC dialect's reference semantics.mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp (4)
212-227: LGTM! Zero-target operations correctly adapted.The macro-generated methods for zero-target operations (like
gphase) correctly use the new signature:
- Lambda takes
ValueRange /*targets*/(empty for zero-target ops)- Returns empty
ValueRange{}- Uses
*thisto create operationsThe pattern is consistent and correct for QCO's value semantics.
Based on learnings, QCO dialect uses value semantics unlike QC's reference semantics.
246-262: LGTM! One-target operations correctly adapted.All one-target operation helpers (with varying numbers of parameters) consistently use the new lambda signature:
- Lambda receives
ValueRange targetsand accesses viatargets[0]- Returns
op->getResults()as ValueRange- Properly destructures results with structured bindings
The implementation correctly maintains QCO's value semantics where operations consume input qubits and produce new output values.
Also applies to: 293-310, 336-356, 383-404
426-445: LGTM! Two-target operations correctly adapted.All two-target operation helpers consistently use the new lambda signature:
- Lambda receives
ValueRange targetsand accesses viatargets[0]andtargets[1]- Returns
op->getResults()as ValueRange- Properly destructures results into control and target pairs
The pattern is uniform across all parameter variations (zero, one, two parameters).
Also applies to: 471-491, 520-541
565-583: LGTM! ctrl implementation correctly updated.The ctrl method correctly:
- Accepts the new signature
std::function<ValueRange(ValueRange)>&- Passes the body directly to
CtrlOp::create- Updates qubit tracking for both controls and targets
- Returns the updated control and target qubits
The implementation properly maintains QCO's value semantics and linear type tracking.
Member
Author
|
@li-mingbao and @Ectras, as this is also relevant for your PRs: We have decided to simplify the signatures of the |
8 tasks
This was referenced Jan 8, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 simplifies the signature of the
QCProgramBuilder::ctrlandQCOProgramBuilder::ctrlmethods.Checklist:
I have added appropriate tests that cover the new/changed functionality.I have updated the documentation to reflect these changes.I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals.I have added migration instructions to the upgrade guide (if needed).