✨ Implement getInputQubits()/getOutputQubits() #1474
Conversation
taminob
added
enhancement
| // TODO: needs to materialize into internal storage; I wasn't able to find a | ||
| // way to convert concat_range -> ValueRange; however: operations cannot | ||
| // define new data members |
There was a problem hiding this comment.
@burgholzer I wasn't really able to find a nice solution for this issue here. If you have some time, maybe you can give your opinion on what to do - I see the following options:
- change signature to
void getInputQubits(SmallVector<Value>& storage)for all operations - change signature to
SmallVector<Value> getInputQubits()for all operations - change signature to
std::variant<ValueRange, SmallVector<Value>> getInputQubits()for all operations (nasty API, but might be best easy-to-implement/performance ratio solution?) - change signature to one of the above just for
CtrlOp(and probably other modifiers in the future) and use more advanced implementation inUnitaryOpInterfaceto materialize input (although I'm not sure yet where/how since interfaces also can't have data members... might also require some global cache like the next option) - use (thread-safe) global cache as storage for
CtrlOps and their input qubits (nasty, but easy to implement and nice API :) )
There was a problem hiding this comment.
@coderabbitai what is your opinion on the above?
For context: the issue is that the interface defines a function getInputQubits that currently returns a ValueRange which is as reference to a container. Here in this scenario, two ValueRanges need to be concatenated making it not a valid ValidRange anymore. I know that this problem also exists in pure C++ and that C++26 introduces a concat_view for that.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MLIR ValueRange SmallVector return type convention patterns
💡 Result:
Short summary of common MLIR conventions and patterns (with sources)
-
ValueRange is a non-owning view — Use it for parameters and local views over existing Value lists (e.g., op->getOperands(), SmallVector owned by caller). Do NOT store ValueRange in long‑lived containers or return it from functions that create temporaries. [1][2]
-
Return an owning container when you must materialize values — If a function must produce/allocate values (or hold them beyond the caller’s local scope), return an owning SmallVector (or SmallVector, SmallVector, etc.). This makes lifetime/ownership explicit and avoids dangling references into temporaries. [1][3]
-
Pattern for 1:1 vs 1:N materializations in converters — MLIR type/conversion callbacks often store 1:N callbacks and normalize 1:1 callbacks by wrapping a single Value into SmallVector for return. Follow that established convention when writing converters/patterns. [3]
-
Practical rules
- Use ValueRange for lightweight read-only access and APIs that accept caller-owned storage. [1]
- When you need to return values you created, return SmallVector<...> (owned). [1][3]
- If you must return a view, return a reference to an existing container the caller owns (document lifetimes). Avoid returning ValueRange that points into a temporary SmallVector you constructed inside the function. [1][4]
Example (pseudo):
- Good: func Accepts(ValueRange operands) { /* view into caller-owned operands */ }
- Good: SmallVector makeValues(...) { /* create and return owned values */ }
- Bad: ValueRange makeValuesAndReturnView(...) { SmallVector tmp = ...; return ValueRange(tmp); } // dangling
Sources
[1] MLIR ValueRange header / docs.
[2] MLIR TypeRange / Value docs (explain view semantics).
[3] MLIR merge/PR and converter code showing SmallVector wrapping of 1:1 -> 1:N returns.
[4] Recent MLIR refactor discussion about Value/TypeRange lifetime implications.
If you want, I can fetch exact code snippets from the headers/PRs and show a minimal safe implementation example with citations.
@ystade Based on MLIR conventions and the specific use case here, I'd recommend option 2: change signature to SmallVector<Value> getInputQubits() for all operations in the interface.
Here's my reasoning:
MLIR Convention Alignment:
The web search confirms that in MLIR, ValueRange is a non-owning view that should only be used for parameters and views over existing containers. When a function needs to materialize values (like concatenating two ranges), the established MLIR pattern is to return an owning SmallVector<Value>. This makes lifetime and ownership explicit and avoids dangling references.
Performance Considerations:
SmallVectoruses Small Size Optimization (SSO), so for typical quantum operations with 1-3 qubits, no heap allocation occurs- The current code already needs to materialize the concatenation somewhere—returning
SmallVectorjust makes this explicit - For operations that don't need concatenation (simple gates), they can construct the
SmallVectorfrom their existingValueRangewith minimal overhead
API Consistency:
Changing just CtrlOp (option 4) or using std::variant (option 3) would create API inconsistency and make the interface harder to use. A uniform return type is clearer.
Why not the other options:
- Option 1 (output parameter): Not idiomatic for modern C++/MLIR APIs
- Option 3 (variant): Complex API that pushes the problem to callers
- Option 4 (just CtrlOp): Breaks interface uniformity
- Option 5 (global cache): Thread-safety concerns and non-local state
Recommendation: Change the UnitaryOpInterface to return SmallVector<Value> for getInputQubits() and getOutputQubits(). Most implementations can simply return SmallVector<Value>(getOperands()) or similar, making the change straightforward.
🧠 Learnings used
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.
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/IR/Operations/StandardGates/BarrierOp.cpp:45-54
Timestamp: 2025-12-09T00:55:11.926Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux), qubits follow single-user/linear-type semantics where each qubit value can only be consumed once, similar to MQTOpt. This invariant makes it safe to dereference getUsers().begin() in canonicalization patterns like MergeSubsequentBarrier in BarrierOp.cpp, as there will be at most one user per qubit output.
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: 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.
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.
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: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp:599-757
Timestamp: 2026-01-12T12:05:56.683Z
Learning: In the QCO builder (mlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cpp), the SCF and func builder methods (scfFor, scfWhile, scfIf, funcCall) assume that all operands passed to them are qubit types (qco.qubit). The current validation through updateQubitTracking is sufficient for this use case. The same assumption applies to the QC builder methods.
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCToQCO/QCToQCO.cpp:1729-1763
Timestamp: 2026-01-10T16:28:41.975Z
Learning: In the QCToQCO conversion pass (mlir/lib/Conversion/QCToQCO/QCToQCO.cpp), the dynamic legality checks for func operations (func::CallOp, func::FuncOp, func::ReturnOp) and scf operations assume that operations use either all classical types or all qubit types, never mixed. Therefore, checking for the presence of qc::QubitType in operands or arguments is sufficient to determine if conversion is needed—there is no need to check both operands and results separately.
<!-- </add_learning]
There was a problem hiding this comment.
@taminob @burgholzer @coderabbitai I would follow the suggestion of @coderabbitai. I had actually the same in my mind but wanted to know @coderabittai 's opinion first.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
For the
WireIteratorI usedautoas the return type to avoid that issue. Probably not the cleanest solution, but works fine regardless. See here. Plus, in the MLIR code base they sometimes also uses it (Line 248 here).
It is actually pretty clean, but unfortunately that doesn't work for the interface because it needs to know the return type before-hand because the implementation is in a separate compile unit (header/source file).
There was a problem hiding this comment.
Ah got it. Unfortunate. Wouldn't it then be an option to use auto and provide a default implementation of the method in the tablegen definition? (Even though having C++ code in tablegen is kind of ugly)
Edit: My bad, this is of course just the ctrl op.
There was a problem hiding this comment.
Ah got it. Unfortunate. Wouldn't it then be an option to use
autoand provide a default implementation of the method in the tablegen definition? (Even though having C++ code in tablegen is kind of ugly)
I don't think that'll work either because tablegen works internally with function pointers which cannot have an unspecified return type - that's also why it's not possible to have template interface methods (if you're curious, you can checkout #1426 for a workaround).
There was a problem hiding this comment.
Interesting, thanks! Really unfortunate. Consequently, I would still require helper methods (using concat views) whenever I don't want the "overhead" of building temporary vectors.
One last question then I won't bother you anymore (I promise!): Given that all the above doesn't work and the assumption that we want to avoid temporary vectors, wouldn't an alternative solution be to use static methods or simply functions, e.g.:
ValueRange getInputQubits(HOp op) {
...
}
auto getInputQubits(CtrlOp op) {
...
}Essentially, the same as the WireIterator had. Definitely not as elegant as methods, but an option (?). Any opinions on that?
There was a problem hiding this comment.
I think the main problem here is that we want a uniform interface. We do not want to distinguish between a CtrlOp and a regular gate. The respective function should return the same type of value.
I haven't thought very deeply about this right now, but I feel the SmallVector is probably the only real option for that.
At least if we do not find a better way to get a ValueRange out of the CtrlOp.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| "std::invoke_result_t<decltype(llvm::concat<Value, ValueRange, ValueRange>), ValueRange, ValueRange>", | ||
| "getInputQubits", (ins), | ||
| [{ | ||
| if constexpr (std::is_same_v<decltype($_op.getInputQubits()), ValueRange>) { | ||
| return llvm::concat<Value>($_op.getInputQubits(), ValueRange{}); | ||
| } else { | ||
| return $_op.getInputQubits(); | ||
| } | ||
| }] |
There was a problem hiding this comment.
After some trial and error, I got this working proposal - although I'm not 100% sure if this actually has better performance than using a llvm::SmallVector (but it probably should be). Any thoughts on this?
I can't use the return type of llvm::concat directly because it's in the detail namespace and thus not really suitable for a public interface.
| } | ||
| if (numControls <= i && i < getNumQubits()) { | ||
| return getBodyUnitary().getInputQubit(i - numControls); | ||
| return getTargetsIn()[i - numControls]; |
There was a problem hiding this comment.
I think this is actually a bug in the current implementation because it doesn't make sense to return the "internal" argument qubit which has a completely different scope and is defined by the CtrlOp - at least to me, this behavior was unintuitive and caused me some headache because figuring out which mlir::Value is which qubit can be a pain in gdb. I'm just not sure how to actually properly expose the mapping "input qubit" -> "body unitary input"/"block argument" in a clean interface, but I think that's a topic for a separate issue.
This is basically a follow-up for #1436.
taminob
force-pushed
the
taminob/mlir-qco-add-qubit-range-getter
branch
from
92f0867 to
9cf79c3
Compare
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds bulk accessor methods Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@mlir/include/mlir/Dialect/QCO/IR/QCODialect.h`:
- Around line 83-87: The assertion in getInputQubits() is off-by-one and wrongly
requires operands.size() to be greater than T; change the check to allow
equality so take_front(T) is valid when operands.size() == T. Update the assert
in ValueRange getInputQubits() (method getInputQubits of the QCODialect/op
class) from asserting T < operands.size() to asserting T <= operands.size() so
it correctly permits zero-parameter cases.
In `@mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp`:
- Around line 190-198: CtrlOp::getOutputQubits returns targets then controls,
which contradicts getOutputQubit(i) that returns controls first; swap the concat
arguments so the returned ValueRange is controls followed by targets (i.e. call
llvm::concat<Value> with ValueRange{getControlsOut()} then
ValueRange{getTargetsOut()}) to match per‑qubit accessor ordering and keep
CtrlOp::getOutputQubits consistent with getOutputQubit.
- Around line 169-177: getInputQubits() currently concatenates targets then
controls (using llvm::concat<Value>(ValueRange{getTargetsIn()},
ValueRange{getControlsIn()}), producing a different ordering than
getInputQubit(i) which returns controls first. Fix getInputQubits() to
concatenate controls then targets (i.e., use llvm::concat with
ValueRange{getControlsIn()} followed by ValueRange{getTargetsIn()}) so that
getInputQubits()[i] matches getInputQubit(i); keep using the same ValueRange
wrapping to avoid changing storage semantics.
This change will hopefully shorten debug times for optimization patterns that introduce new gates. Especially when inserting gates with constant parameters, caution is advised when working with circuits containing controlled gates because the pattern will also be applied to the operation inside the control body - potentially leading to undesired operations in the control body. This assertion should make this easier to spot and debug.
taminob
force-pushed
the
taminob/mlir-qco-add-qubit-range-getter
branch
from
147f2d5 to
8baf49c
Compare
Description
Introduce
getInputQubits()andgetOutputQubits()which is especially required for a smooth interaction withCtrlOpthroughUnitaryOpInterface.Checklist: