✨ Add non-linear control flow conversions between QC and QCO dialects #1396
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
denialhaag
added
enhancement
|
Another small thing: The linter still has two issues that need fixing. Unfortunately, it cannot post the comment if the PR comes from a fork. 😕 |
## Description This PR templates the `variantToValue` function to make the function more generic and to support the creation of `IndexType` and `I1Type` constants. Required for #1396 ## Checklist: <!--- This checklist serves as a reminder of a couple of things that ensure your pull request will be merged swiftly. --> - [ ] The pull request only contains commits that are focused and relevant to this change. - [ ] 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). - [ ] The changes follow the project's style guidelines and introduce no new warnings. - [ ] The changes are fully tested and pass the CI checks. - [ ] I have reviewed my own code changes. --------- Signed-off-by: li-mingbao <[email protected]> Co-authored-by: Li-ming Bao <[email protected]>
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks @li-mingbao for this PR. This is great work already.
I tried to have a very detailed look into the changes and I hope that most of the comments are pretty easy to incorporate.
There is one big, big comment that probably needs some discussion and will likely trigger changes.
And maybe one or two optimizations that could lead to a reduction in the lines of code here.
Besides that, @denialhaag still had some suggestions and the linter is still failing. (you can always find the summary for the clang-tidy check at the workflow summary e.g., https://github.com/munich-quantum-toolkit/core/actions/runs/20959669388)
Lastly, this PR should be added to the growing list of additions to the changelog entry for the new dialects.
| //===--------------------------------------------------------------------===// | ||
| // SCF operations | ||
| //===--------------------------------------------------------------------===// | ||
|
|
||
| /** | ||
| * @brief Constructs a scf.for operation without iter args | ||
| * | ||
| * @param lowerbound Lowerbound of the loop | ||
| * @param upperbound Upperbound of the loop | ||
| * @param step Stepsize of the loop | ||
| * @param body Function that builds the body of the for operation | ||
| * @return Reference to this builder for method chaining | ||
| * | ||
| * @par Example: | ||
| * ```c++ | ||
| * builder.scfFor(lb, ub, step, [&](Value iv) { builder.x(q0); }); | ||
| * ``` | ||
| * ```mlir | ||
| * scf.for %iv = %lb to %ub step %step { | ||
| * qc.x %q0 : !qc.qubit | ||
| * } | ||
| * ``` | ||
| */ | ||
| QCProgramBuilder& scfFor(const std::variant<int64_t, Value>& lowerbound, | ||
| const std::variant<int64_t, Value>& upperbound, | ||
| const std::variant<int64_t, Value>& step, | ||
| const std::function<void(Value)>& body); |
There was a problem hiding this comment.
A thought that immediately comes to mind: How would I write a for loop where instructions in the body depend on the loop variable?
Imagine a scenario, where I would want to apply a Hadamard gate to each of N qubits. How would I actually realize that without explicitly writing N Hadamard operations, but rather by using a for loop?
@DRovara @denialhaag @ystade @flowerthrower I really hate to open up this box of pandora again, but is something like that even possible without some notion of a qubit register that could be indexed into as part of the loop?
I have the slight feeling that some kind of "bundling"/"aliasing" is almost unavoidable here. Maybe the for loop would have to receive a variadic targets qubit array, that could be indexed into as part of the loop. Pragmatic solutions and ideas welcome.
Maybe the iterargs solution from the QCO dialect is already sufficient to cover this.
There was a problem hiding this comment.
The way I imagined this to be implemented was to just fall back to some MLIR array type and then simply iterate over that.
That means, that this program could e.g. look like (pseudocode):
arr = array<qubit>(5);
for (0 < i < 5) {
arr[i] = alloc()
}
for (0 < i < 5) {
q = arr[i]
q_ = h(q)
arr[i] = q_
}
Although, writing this I am wondering:
- is there already some sort of array data type that supports linear types? (Seems unlikely)
- can there ever be any approach that's different than this population-loop -> gate-loop approach?
- if this really is the way we always expect the loop to access variables, doesn't it make sense to use/make a
foreach-like condtructs for Qubits?
There was a problem hiding this comment.
I wanted to suggest exactly the same as Damian, I.e., reuse something that already exists.
Regarding the questions, @DRovara raised: I do not know how this works in MLIR but is there potentially something like in C++ where, when I store for example unique pointers in a vector, the vector becomes automatically non-copyable?
There was a problem hiding this comment.
I agree with both of you that we should reuse something that exists.
However, before we over engineer the solution here: have you looked at the QCO solution here?
The comment here is in the QC dialect, where we do not enforce linear types.
I believe that adapting the solution from the QCO dialect slightly for the QC dialect could already be part of the solution as the QCO dialect already has these capabilities.
Edit: conversations with Claude seem to suggest that memref would be reasonable to use here.
For the QC dialect that also seems to make sense to me. We could either generate the memref upfront (in the builders when creating registers), basically by doing a scf.for with allocs in the body. Generally, such memrefs can also be formed adhoc as part of the program.
I have slight mental troubles thinking about the QCO pendant for all of this and the enforcement of linear types. But the current solution in QCO is also not particularly ideal if one wants to, e.g. represent the application of a Hadamard gate to a hundred qubits. Specifying 100 iter_args isn't particularly practical.
@li-mingbao maybe you also have some thoughts about this.
There was a problem hiding this comment.
In my opinion the solution with memref works.
In QCO you could use the memref register as iter_args and return that instead of specifying all the qubits individually.
There was a problem hiding this comment.
Do you mean to implement the registers with memref similar to what it was before in the mqtdyn/opt dialect and adapt the functions?
There was a problem hiding this comment.
To be honest, I am not 100% sure what the best approach here is. But probably yeah.
In essence, what we want to achieve eventually is to represent the programs described in unitaryfoundation/jeff#35 that use all of these structured control flow elements and dynamically index qubit values in loops, etc.
When we say we want to support SCF, then these are the programs we mean.
We should be able to import programs like these into QC and transform them to QCO for optimization.
There was a problem hiding this comment.
I think it would be better to finish this PR first and then create a new PR to support arraylike structures again with memref and adapt the functions there as I guess this would affect more parts of the codebase.
There was a problem hiding this comment.
I can live with that. Although I find it not fully ideal as the SCF support added in this PR is not really fully functional in my opinion at the moment. There is simply no way that I could use the builder to write a for loop in QC where the operations in the loop would depend on the loop index; while this is possible in QCO due to the iter_args.
Maybe a middle ground is to also use iter_args on the QC side to enable this and then to optimize it in a follow-up.
I am definitely not hard set on this, but this would be my tendency. @ystade @DRovara @denialhaag feel free to overrule me here.
There was a problem hiding this comment.
I would definitely be in favor of postponing the addition of memref registers in a follow-up PR. I was wondering what the reason was for deciding against memref registers. If I recall correctly, this was the outcome of a discussion between @burgholzer and @DRovara, right?
That said, I see the point of the middle-ground option of using iter_args for now to make this PR more complete.
Edit: After a conversation with @li-mingbao, I would say that using iter_args for this might not be feasible.
| // Conversion of qco types in func.func signatures | ||
| // Note: This currently has limitations with signature changes | ||
| populateFunctionOpInterfaceTypeConversionPattern<func::FuncOp>( | ||
| patterns, typeConverter); | ||
| target.addDynamicallyLegalOp<func::FuncOp>([&](func::FuncOp op) { | ||
| return typeConverter.isSignatureLegal(op.getFunctionType()) && | ||
| typeConverter.isLegal(&op.getBody()); | ||
| }); | ||
|
|
||
| // Conversion of qco types in func.return | ||
| populateReturnOpTypeConversionPattern(patterns, typeConverter); | ||
| target.addDynamicallyLegalOp<func::ReturnOp>( | ||
| [&](const func::ReturnOp op) { return typeConverter.isLegal(op); }); | ||
|
|
||
| // Conversion of qco types in func.call | ||
| populateCallOpTypeConversionPattern(patterns, typeConverter); | ||
| target.addDynamicallyLegalOp<func::CallOp>( | ||
| [&](const func::CallOp op) { return typeConverter.isLegal(op); }); | ||
|
|
||
| // Conversion of qco types in control-flow ops (e.g., cf.br, cf.cond_br) | ||
| populateBranchOpInterfaceTypeConversionPattern(patterns, typeConverter); |
There was a problem hiding this comment.
We adapted this code from the Catalyst compiler as it was supposed to simplify some of the things that this PR is doing. Are we sure that these populate<...>OpTypeConversionPattern functions are not rather useful to safe us some of the troubles and/or code that needs to be written above? Especially things like the argument conversions seem to be coverable with this and might turn out nicer and less code.
There was a problem hiding this comment.
i tried them out and I do not see a big benefit here as these are mainly used to simply change the type.
In the conversionpasses here, almost all operations must be replaced with a new one, since they have different number of operands or return values. So I need to match and replace them either way.
There was a problem hiding this comment.
Fair enough. Basically I am just looking for ways to reduce some boilerplate code here 😌
There was a problem hiding this comment.
I haven't reviewed this file really, but I suppose similar comments than for the other direction also apply here.
| const OpBuilder::InsertionGuard guard(*this); | ||
| setInsertionPointToStart(b.getInsertionBlock()); | ||
| body(iv); | ||
| b.create<scf::YieldOp>(loc); |
There was a problem hiding this comment.
Are you sure this is necessary? Isn't the yield implicit? also holds for further aspects down the line.
There was a problem hiding this comment.
I get an error that there is no terminator when I remove them so I think they are required when you use this builder. Same with the other operations.
There was a problem hiding this comment.
Fair enough 👍🏻
I Might play around with the PR a little myself once the major points are consolidated 😌
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 1385-1389: Add a presence check/assert before indexing qubitMap in
the loop that builds qcoQubits: verify that qubitMap contains the key qcQubit
(e.g., assert(qubitMap.count(qcQubit) && "missing qubit mapping") or an
equivalent MLIR-friendly assert) and only then push_back the mapped Value;
update the loop in QCToQCO.cpp where qcoQubits, qcQubits, and qubitMap are used
(consistent with the pattern in ConvertQCBarrierOp) to avoid inserting a
default/null Value when the key is absent.
- Around line 1624-1628: The loop building qcoQubits accesses qubitMap[qcQubit]
without verifying the key exists; add a check/assert before the access (for
example assert(qubitMap.count(qcQubit) && "missing mapping for qcQubit") or use
qubitMap.find(qcQubit) and handle the missing case) to ensure qcQubit is present
in qubitMap before pushing into qcoQubits; update the code around qcoQubits,
qcQubits, and qubitMap to perform this validation and provide a clear failure
path or diagnostic if the mapping is absent.
- Around line 1471-1475: Before pushing mapped qubits into qcoQubits, verify
that each qcQubit exists in qubitMap (same pattern used in ConvertQCScfWhileOp);
replace the direct qubitMap[qcQubit] access with a presence check (e.g., use
qubitMap.find(qcQubit) or qubitMap.contains) and assert or handle the
missing-key case, then push the found mapped value into qcoQubits to avoid
undefined behavior.
♻️ Duplicate comments (4)
mlir/unittests/Conversion/QCToQCO/test_conversion_qc_to_qco.cpp (3)
40-49: AvoidloadAllAvailableDialects()in tests.Loading all dialects can mask missing registrations; it’s safer to rely on the explicit registry. Please confirm no hidden dialect dependencies.
♻️ Proposed change
- context->appendDialectRegistry(registry); - context->loadAllAvailableDialects(); + context->appendDialectRegistry(registry);
69-75: Preferconst ModuleOp&for output printing.
OwningOpRefownership isn’t needed here and couples callers unnecessarily. Please update call sites to pass*moduleormodule.get().♻️ Proposed change
-static std::string getOutputString(mlir::OwningOpRef<mlir::ModuleOp>& module) { +static std::string getOutputString(const mlir::ModuleOp& module) { std::string outputString; llvm::raw_string_ostream os(outputString); - module->print(os); + module.print(os); os.flush(); return outputString; }
77-112: Printed-IR string equality is brittle.Consider structural checks or FileCheck-style matching (or a brief comment explaining the tradeoff). This applies to the other ASSERT_EQ comparisons below as well.
mlir/lib/Conversion/QCOToQC/QCOToQC.cpp (1)
1082-1086: Docstring direction is reversed.This conversion is QCO (value semantics) → QC (memory semantics), but the brief says the opposite.
✏️ Suggested fix
- * `@brief` Converts func.func with memory semantics to func.func with - * value semantics for qubit values. This currently assumes no mixed types as + * `@brief` Converts func.func with value semantics to func.func with + * memory semantics for qubit values. This currently assumes no mixed types as
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 128-179: The collectUniqueQubits function fails to include block
arguments, causing uniqueQubits to be empty for functions that only take qubit
args; modify collectUniqueQubits to seed uniqueQubits with any qc::QubitType
block arguments by iterating region.front().getArguments() (after the
region.hasOneBlock() check) and inserting operands whose type equals
qc::QubitType::get(ctx); keep the rest of the logic (operands/results/recursive
regions and setting "needChange") so state->regionMap[func] will contain the
qubit args for funcs even when no internal ops use them.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@mlir/lib/Conversion/QCOToQC/QCOToQC.cpp`:
- Around line 1076-1079: The recreated func.call currently drops all attributes;
update the creation and replacement so the new call preserves the original op's
attributes: capture the original call's attribute dictionary (e.g.,
op->getAttrs() or op.getAttrDictionary()), pass or apply those attributes to the
new func::CallOp created via func::CallOp::create (or set them on the returned
op) before calling rewriter.replaceOp(op, adaptor.getOperands()); ensure you
reference the original op (op), the creation site func::CallOp::create, and
rewriter.replaceOp so the new call retains metadata used for
ABI/target/analysis.
- Around line 871-879: The new scf::IfOp created with scf::IfOp::create (newIf)
currently drops attributes from the original op (op); update the transformation
to copy op->getAttrs() to newIf after creation (e.g., iterate op->getAttrs() or
use setAttrs/getAttrDictionary) so that any annotations/metadata are preserved,
and apply the same pattern for analogous scf::WhileOp and scf::ForOp rebuilds;
ensure this is done after creating newIf and before or immediately after
inlining the regions so attributes remain attached to the rebuilt op.
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 179-185: collectUniqueQubits currently includes temporaries (e.g.,
results of qc::AllocOp) and storing it directly into state->regionMap[func]
causes a return-type mismatch in ConvertQCFuncFuncOp/ConvertQCFuncReturnOp;
instead filter uniqueQubits to only the function's formal arguments before
assigning to state->regionMap: for the func::FuncOp 'func', build a collection
of its arguments (func.getArguments()) that have type qcType and are present in
uniqueQubits, and assign that filtered list to state->regionMap[func] so
temporaries from qc::AllocOp are excluded.
| // Create the new if operation | ||
| auto newIf = scf::IfOp::create(rewriter, op.getLoc(), TypeRange{}, | ||
| op.getCondition(), false); | ||
| // Inline the regions | ||
| rewriter.inlineRegionBefore(op.getThenRegion(), newIf.getThenRegion(), | ||
| newIf.getThenRegion().end()); | ||
|
|
||
| rewriter.inlineRegionBefore(op.getElseRegion(), newIf.getElseRegion(), | ||
| newIf.getElseRegion().end()); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Preserve scf.if attributes when rebuilding the op.
scf::IfOp::create drops any existing op attributes (e.g., loop/if annotations or custom metadata). Consider copying attributes from the original op to the new op to avoid silently losing metadata. This likely applies to the scf.while / scf.for conversions below as well.
♻️ Suggested fix
- auto newIf = scf::IfOp::create(rewriter, op.getLoc(), TypeRange{},
- op.getCondition(), false);
+ auto newIf = scf::IfOp::create(rewriter, op.getLoc(), TypeRange{},
+ op.getCondition(), false);
+ newIf->setAttrs(op->getAttrs());🤖 Prompt for AI Agents
In `@mlir/lib/Conversion/QCOToQC/QCOToQC.cpp` around lines 871 - 879, The new
scf::IfOp created with scf::IfOp::create (newIf) currently drops attributes from
the original op (op); update the transformation to copy op->getAttrs() to newIf
after creation (e.g., iterate op->getAttrs() or use setAttrs/getAttrDictionary)
so that any annotations/metadata are preserved, and apply the same pattern for
analogous scf::WhileOp and scf::ForOp rebuilds; ensure this is done after
creating newIf and before or immediately after inlining the regions so
attributes remain attached to the rebuilt op.
| func::CallOp::create(rewriter, op->getLoc(), adaptor.getCallee(), | ||
| TypeRange{}, adaptor.getOperands()); | ||
| rewriter.replaceOp(op, adaptor.getOperands()); | ||
| return success(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Preserve func.call attributes when recreating the call.
Rebuilding the call without copying attributes can drop callsite metadata. If attributes are used for ABI/targets/analysis, this may be lossy.
♻️ Suggested fix
- func::CallOp::create(rewriter, op->getLoc(), adaptor.getCallee(),
- TypeRange{}, adaptor.getOperands());
+ auto newCall = func::CallOp::create(rewriter, op->getLoc(),
+ adaptor.getCallee(), TypeRange{},
+ adaptor.getOperands());
+ newCall->setAttrs(op->getAttrs());🤖 Prompt for AI Agents
In `@mlir/lib/Conversion/QCOToQC/QCOToQC.cpp` around lines 1076 - 1079, The
recreated func.call currently drops all attributes; update the creation and
replacement so the new call preserves the original op's attributes: capture the
original call's attribute dictionary (e.g., op->getAttrs() or
op.getAttrDictionary()), pass or apply those attributes to the new func::CallOp
created via func::CallOp::create (or set them on the returned op) before calling
rewriter.replaceOp(op, adaptor.getOperands()); ensure you reference the original
op (op), the creation site func::CallOp::create, and rewriter.replaceOp so the
new call retains metadata used for ABI/target/analysis.
| if (llvm::isa<func::ReturnOp>(operation)) { | ||
| if (auto func = operation.getParentOfType<func::FuncOp>()) { | ||
| if (!func.getArgumentTypes().empty() && | ||
| func.getArgumentTypes().front() == qcType) { | ||
| operation.setAttr("needChange", StringAttr::get(ctx, "yes")); | ||
| state->regionMap[func] = uniqueQubits; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the QCToQCO conversion file and examine the relevant sections
fd "QCToQCO\.cpp" mlir/lib/Conversion/ | head -5Repository: munich-quantum-toolkit/core
Length of output: 110
🏁 Script executed:
# Examine the collectUniqueQubits implementation and ConvertQCFuncFuncOp/ConvertQCFuncReturnOp patterns
rg -n "collectUniqueQubits|ConvertQCFuncFuncOp|ConvertQCFuncReturnOp" mlir/lib/Conversion/QCToQCO/QCToQCO.cpp -A 3 -B 2 | head -100Repository: munich-quantum-toolkit/core
Length of output: 1593
🏁 Script executed:
# Get the full context of func conversion patterns and regionMap usage
rg -n "regionMap\[func\]" mlir/lib/Conversion/QCToQCO/QCToQCO.cpp -B 5 -A 5Repository: munich-quantum-toolkit/core
Length of output: 507
🏁 Script executed:
# Check how ConvertQCFuncReturnOp constructs return operands
rg -n "class ConvertQCFuncReturnOp" mlir/lib/Conversion/QCToQCO/QCToQCO.cpp -A 30Repository: munich-quantum-toolkit/core
Length of output: 53
🏁 Script executed:
# Look for temporary or local qubit allocation in QC dialect functions
rg -n "qc::Alloc|allocQubit" mlir/lib/Conversion/QCToQCO/QCToQCO.cpp -B 3 -A 3Repository: munich-quantum-toolkit/core
Length of output: 564
🏁 Script executed:
# Extract ConvertQCFuncFuncOp implementation (around lines 1662-1704)
sed -n '1662,1704p' mlir/lib/Conversion/QCToQCO/QCToQCO.cppRepository: munich-quantum-toolkit/core
Length of output: 1556
🏁 Script executed:
# Extract ConvertQCFuncReturnOp implementation (around lines 1707-1760)
sed -n '1707,1760p' mlir/lib/Conversion/QCToQCO/QCToQCO.cppRepository: munich-quantum-toolkit/core
Length of output: 2085
🏁 Script executed:
# Check how qubits are returned in ConvertQCFuncReturnOp - look for operands construction
rg -n "orderedQubits\|regionMap.*Return" mlir/lib/Conversion/QCToQCO/QCToQCO.cpp -B 5 -A 10Repository: munich-quantum-toolkit/core
Length of output: 53
🏁 Script executed:
# Check if there's any filtering of temporary/allocated qubits before regionMap storage
rg -n "needChange\|regionMap\[func\]" mlir/lib/Conversion/QCToQCO/QCToQCO.cpp -B 10 -A 10 | head -60Repository: munich-quantum-toolkit/core
Length of output: 53
🏁 Script executed:
# Check how qc::AllocOp fits into function context - does it escape function boundaries?
rg -n "qc::AllocOp\|getParent" mlir/lib/Conversion/QCToQCO/QCToQCO.cpp | grep -A 2 -B 2 "AllocOp"Repository: munich-quantum-toolkit/core
Length of output: 53
🏁 Script executed:
# Look at the full context where regionMap is used to understand data flow
sed -n '170,195p' mlir/lib/Conversion/QCToQCO/QCToQCO.cppRepository: munich-quantum-toolkit/core
Length of output: 1170
🏁 Script executed:
# Check if uniqueQubits passed to regionMap includes only function arguments or also local allocations
sed -n '128,175p' mlir/lib/Conversion/QCToQCO/QCToQCO.cppRepository: munich-quantum-toolkit/core
Length of output: 1940
Function return signature mismatch: regionMap includes temporary qubits not in signature.
collectUniqueQubits collects all qubits in the function body, including results from qc::AllocOp (temporary qubits). When stored in state->regionMap[func] = uniqueQubits, this set includes qubits beyond the function's arguments. However, ConvertQCFuncFuncOp sets the return type to match only the argument count, and ConvertQCFuncReturnOp returns all qubits from regionMap. This mismatch causes a type error when a function allocates temporary qubits.
Restrict regionMap to function arguments only:
- operation.setAttr("needChange", StringAttr::get(ctx, "yes"));
- state->regionMap[func] = uniqueQubits;
+ operation.setAttr("needChange", StringAttr::get(ctx, "yes"));
+ llvm::SetVector<Value> argQubits;
+ for (auto arg : func.getArguments()) {
+ if (arg.getType() == qcType) {
+ argQubits.insert(arg);
+ }
+ }
+ state->regionMap[func] = std::move(argQubits);🤖 Prompt for AI Agents
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp` around lines 179 - 185,
collectUniqueQubits currently includes temporaries (e.g., results of
qc::AllocOp) and storing it directly into state->regionMap[func] causes a
return-type mismatch in ConvertQCFuncFuncOp/ConvertQCFuncReturnOp; instead
filter uniqueQubits to only the function's formal arguments before assigning to
state->regionMap: for the func::FuncOp 'func', build a collection of its
arguments (func.getArguments()) that have type qcType and are present in
uniqueQubits, and assign that filtered list to state->regionMap[func] so
temporaries from qc::AllocOp are excluded.
5 participants
Description
This PR adds support for the conversion of the
scfoperationsscf.while,scf.for,scf.ifand the conversion of additional functions between theqcand theqcodialect. This allows the conversion of programs with nonlinear controlflow.Reopened since #1391 got automatically closed after the branch was deleted.
Checklist: