-
-
Notifications
You must be signed in to change notification settings - Fork 48
🐛 Rename CtrlOp region to region to avoid hiding trait method getBody()
#1430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughCtrlOp's region symbol renamed from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-12-05T17:45:37.602ZApplied to files:
📚 Learning: 2025-10-14T14:37:38.047ZApplied to files:
📚 Learning: 2025-12-01T11:00:40.342ZApplied to files:
📚 Learning: 2025-11-24T10:19:41.147ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
burgholzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM and I wouldn't worry too much about the different ways to get the region.
However, the proposal of simply calling it $region seems enticing.
This would simply be an override of the respective method, right?
Could we try this here in the PR?
It is not an |
This prevents the automatically generated getter to hide one of the trait interface methods. These methods do have a slightly different signature (getBody() returns a Block and getBodyRegion() accepts a index parameter) which could lead to confusion and is an inconsistent API. An alternative could be simply the name "region" because getRegion() of the OneRegion trait has the same signature and thus the override would probably not be noticed. However, there would be two identical implementations for this method and this could make debugging harder. The downside to this solution is that there are now three functions to access the region: getRegion() (by OneRegion), getBodyRegion() (by SingleBlock) and getCtrlBodyRegion() (by getter).
This will intentionally hide OneRegion::getRegion() which is exactly the same anyway. It avoids adding an additional method to the API that does exactly the same as getBodyRegion() and getRegion().
804dc3d to
f9f2e94
Compare
|
Does this change require an addition to the changelog? I'm not sure how you want to handle these small follow-ups for the dialect rewrite. |
While the dialect rewrite is not part of an official release, I would typically not include changelog fragments for changes like these. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @mlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cpp:
- Line 112: The PR text is inconsistent: the TableGen def uses
SizedRegion<1>:$region (see QCOps.td entries) so the generated getter is
getRegion(), not getCtrlBodyRegion(); update the PR title and description to
state the region was renamed to "region" and mention the generated accessor
getRegion() (and any references to ctrlBodyRegion in comments or docs) so they
match the actual TableGen symbol and generated API.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
mlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.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/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/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QCToQCO/QCToQCO.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/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QCToQCO/QCToQCO.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/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QCToQCO/QCToQCO.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/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.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/Conversion/QCOToQC/QCOToQC.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-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/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cpp
📚 Learning: 2026-01-07T12:29:02.062Z
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1040-1050
Timestamp: 2026-01-07T12:29:02.062Z
Learning: In the QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp), ConvertQCOFuncCallOp assumes that if a func::CallOp has qubit results, then all arguments and results are qubits (no mixed classical/quantum types). The conversion is scoped to handle all-qubit function calls only.
Applied to files:
mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cpp
📚 Learning: 2026-01-07T12:29:16.380Z
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1070-1085
Timestamp: 2026-01-07T12:29:16.380Z
Learning: In the QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp), the ConvertQCOFuncFuncOp pattern assumes that when a func.func operation is matched for conversion, all of its arguments are qco.qubit types (never mixed qubit/classical). The pattern unconditionally converts all arguments to qc::QubitType based on this assumption.
Applied to files:
mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cpp
📚 Learning: 2025-10-09T13:25:36.887Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReuseQubitsPattern.cpp:98-100
Timestamp: 2025-10-09T13:25:36.887Z
Learning: In MLIR code, when traversing parent operations to find a common block between two operations where one uses the result of another, explicit bounds checking is not necessary. MLIR's SSA properties and scope locality guarantees ensure that operations using results must be in compatible scopes and will always share a common ancestor in the operation hierarchy.
Applied to files:
mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.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/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QCToQCO/QCToQCO.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/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cpp
📚 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/include/mlir/Dialect/QC/IR/QCOps.tdmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.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/Conversion/QCOToQC/QCOToQC.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). (11)
- GitHub Check: 🇨 Test 🍎 (macos-15, clang, Debug) / 🍎 macos-15 clang Debug
- GitHub Check: 🇨 Test 🏁 (windows-2025, msvc, Release) / 🏁 windows-2025 msvc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🇨 Test 🏁 (windows-2025, msvc, Debug) / 🏁 windows-2025 msvc Debug
- GitHub Check: 🇨 Test 🍎 (macos-15, clang, Release) / 🍎 macos-15 clang Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: 🇨 Coverage / 📈 Coverage
🔇 Additional comments (6)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.td (1)
1070-1073: LGTM: Region symbol and assembly format correctly updated.The region symbol rename from
$bodyto$regionand the corresponding assembly format update are correct. This will generate agetRegion()method, allowing thegetBody()method from theSingleBlocktrait (line 1046) to be used without name collision.mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp (1)
155-155: LGTM: Changes consistent with QC dialect pattern.The updates mirror those in the QC dialect's CtrlOp.cpp:
- Line 155:
getBody()->front()correctly accesses the first operation in the body block- Line 275:
*getBody()correctly dereferences the Block pointer to get a Block referenceThese changes are consistent with using the trait-provided
getBody()that returnsBlock*.Also applies to: 275-275
mlir/lib/Conversion/QCToQCO/QCToQCO.cpp (1)
1130-1131: LGTM: Conversion correctly uses the new region accessor.The conversion now properly uses
getRegion()to access the CtrlOp's region for cloning:
- Line 1130:
qcoOp.getRegion()obtains the destination region- Line 1131:
op.getRegion()provides the source region for cloningThis aligns with the region symbol rename in the TableGen definitions.
mlir/include/mlir/Dialect/QC/IR/QCOps.td (1)
951-952: LGTM: QC dialect CtrlOp updated consistently with QCO.The region symbol and assembly format updates match the pattern in the QCO dialect. The
SingleBlockImplicitTerminatortrait (line 929) ensures proper single-block semantics, and the rename to$regionavoids method name collision with trait-provided methods.mlir/lib/Dialect/QC/IR/Modifiers/CtrlOp.cpp (1)
112-112: The region accessor pattern is correct.The code properly uses
getBody()->front()(Line 112) and*getBody()(Line 176), which is the correct pattern for accessing theBlock*from theSingleBlockImplicitTerminator<"::mlir::qc::YieldOp">trait. This trait is properly declared forCtrlOpin QCOps.td and ensures thatgetBody()returns aBlock*rather than aRegion&.All other
CtrlOpcode paths in the QC dialect correctly use the same accessor pattern; no migration togetRegion()is needed since theSingleBlockImplicitTerminatortrait provides the correctgetBody()accessor.mlir/lib/Conversion/QCOToQC/QCOToQC.cpp (1)
780-781: LGTM! Correctly uses the trait method for region access.The changes properly update the region accessor from
getBody()togetRegion(), aligning with the TableGen parameter rename. Using theOneRegiontrait method ensures consistency and avoids the method hiding issue described in the PR objectives.
ctrlBodyRegion to avoid hiding trait methodsregion to avoid hiding trait method getBody()
burgholzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Thanks again for the PR!
Description
Unfortunately, this became a little bit more cumbersome to figure out than expected.
The automatically generated getter for the parameter "body" was hiding one of the trait interface methods. These methods do have a slightly different signature (
getBody()returns aBlock*andgetBodyRegion()accepts an index parameter) which can lead to confusion and an inconsistent API.An alternative to this proposal could be simply the name "region" because
getRegion()of theOneRegiontrait has the same signature and thus the override would probably not be noticed. However, there would be two identical implementations for this method and this could make debugging harder.The downside to the current solution is that there are now three functions to access the region:
getRegion()(byOneRegion),getBodyRegion()(bySingleBlock) andgetCtrlBodyRegion()(by getter).Checklist: