Conversation
simon1hofmann
added
enhancement
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds explicit static vs dynamic qubit support across QC/QCO: type parameter and constraints, assembly parse/print, builders updated to preserve isStatic, verification enforcing pairwise type preservation, region-aware QC↔QCO conversions with conditional dealloc handling, mapping resynchronization, and expanded tests/program builders. Changes
Sequence Diagram(s)sequenceDiagram
participant QC as QC IR
participant QCToQCO as QC->QCO Lowering
participant QCO as QCO IR
participant QCOToQC as QCO->QC Lowering
participant FuncReturn as Func.return Rewrite
QC->>QCToQCO: lower ops (alloc/static/ctrl/inv/yield)
QCToQCO->>QCToQCO: push modifier frames & assign per-region qubit maps
QCToQCO->>QCO: emit qco ops with mapped qubit types (preserve isStatic)
QCO->>QCOToQC: conversion back to qc ops
QCOToQC->>QCOToQC: preserve isStatic in type conversion
QCOToQC->>QCOToQC: decide dealloc handling
alt qubit is static
QCOToQC-->>QCO: erase qco.dealloc
else qubit is dynamic
QCOToQC->>QC: emit qc::DeallocOp
end
QCOToQC->>FuncReturn: on return, compute escaped qubits
FuncReturn->>FuncReturn: insert qco.dealloc for non-escaped live qubits and replace return
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 136-163: state.qubitMap contains SSA values for return operands
too, so the current loop deallocates qubits that are about to escape via
func::ReturnOp; modify the qubit collection before sorting/Dealloc emission to
exclude any Values that appear in adaptor.getOperands() (the operands passed to
the new func::ReturnOp) so those escaping qubits are not deallocated; locate the
block that builds liveQubits from state.qubitMap, filter out
adaptor.getOperands() (or build a DenseSet of return operands and skip them when
inserting into liveQubits), then continue with the deterministic sort and
creation of qco::DeallocOp for the remaining qubits and clear state.qubitMap as
before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 535af8d9-20fd-4c6d-b9b8-f07a948730d4
📒 Files selected for processing (16)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Conversion/QCOToQC/test_qco_to_qc.cppmlir/unittests/Conversion/QCToQCO/test_qc_to_qco.cppmlir/unittests/Conversion/QCToQIR/test_qc_to_qir.cppmlir/unittests/Dialect/QC/IR/test_qc_ir.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/Dialect/QIR/IR/test_qir_ir.cppmlir/unittests/programs/qc_programs.cppmlir/unittests/programs/qc_programs.hmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qco_programs.hmlir/unittests/programs/qir_programs.cppmlir/unittests/programs/qir_programs.h
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 139-142: The current code builds escapedQubits from
adaptor.getOperands(), which returns stale allocation values because gate
patterns (e.g., ConvertQCOneTargetZeroParameterToQCO) call rewriter.eraseOp()
and only update state.qubitMap while ConvertQCAllocOp uses replaceOpWithNewOp()
and updates the ConversionValueMapping; fix by iterating the original return
operands but for each Value check state.qubitMap for a mapped replacement and
insert that mapped Value into escapedQubits (falling back to the original
operand only if no entry exists), so ConvertFuncReturnOp/func::ReturnOp uses the
final mapped qubit values instead of stale adaptor.getOperands() results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f33f39f-095a-4961-8d6e-a6cbabbdf6ca
📒 Files selected for processing (1)
mlir/lib/Conversion/QCToQCO/QCToQCO.cpp
|
Is this one ready for review? 🙂 |
…ly an operation across multiple program builders (QC, QCO, QIR).
… QCO, QIR) to enhance coverage and ensure functionality.
…ted test cases across various dialects and programs to reflect the new functionality of allocating multiple static qubits and applying operations.
…lated operations and conversions in QCO and QC dialects. Add tests for static qubit type propagation and ensure correct handling in various operations.
…allocation, modifying builders and operations accordingly.
simon1hofmann
force-pushed
the
static_qubit_handling
branch
from
71bbe39 to
0221058
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/include/mlir/Dialect/QC/IR/QCOps.td`:
- Around line 117-126: The builder currently creates a static qubit in C++ but
the ODS op signature still allows a generic QubitType result, so textual IR like
`qc.static 0 : !qc.qubit` can bypass the static-qubit invariant; update the op
definition in QCOps.td to constrain the result to a static qubit (e.g., change
the results from (outs QubitType:$qubit) to an explicit static qubit type such
as (outs QubitType<static>:$qubit) or the ODS equivalent !qc.qubit<static>), and
adjust the assemblyFormat if needed so the textual form requires the static
qubit type; keep the existing builder call that uses QubitType::get(...,
/*isStatic=*/true).
In `@mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp`:
- Around line 129-130: The new block argument is being created with a hardcoded
non-static QubitType which loses the promoted control's concrete staticness;
instead, fetch the original promoted control's type (e.g., from the control
operand collection like op.getControls()[<promotedIndex>] or
op.getControl(<promotedIndex>).getType()) and pass that Type into
op.getBody()->addArgument(...) so the added argument preserves the exact
QubitType (including isStatic) and location rather than using
QubitType::get(..., /*isStatic=*/false).
In `@mlir/lib/Dialect/QCO/IR/QCOOps.cpp`:
- Around line 74-75: The code is hardcoding QubitType::get(...,
/*isStatic=*/false) when creating aliased region block arguments (newArg.type),
which loses staticness; update parseTargetAliasing to accept the parsed target
type list (thread the parsed target types through the custom assembly directive
inputs) and use the original parsed target's staticness when constructing
newArg.type (i.e., derive isStatic from the corresponding parsed target type
instead of always false), ensuring QubitType::get preserves static vs dynamic
qubit semantics during parsing and verification.
In `@mlir/lib/Dialect/QTensor/IR/Operations/AllocOp.cpp`:
- Around line 34-36: AllocOp::verify currently allows a static qco::QubitType
element type but the builder now always materializes qtensor.alloc with a
non-static qubit; update AllocOp::verify() to enforce the new invariant by
checking the element type of the result RankedTensorType (or the operand/result
type used by AllocOp) and emit an error if qco::QubitType::isStatic (or the
equivalent static flag) is true, ensuring parser/hand-written IR cannot claim a
static allocation and referencing AllocOp::verify, qtensor.alloc and
qco::QubitType to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a308d815-bb91-40ea-971c-273068af3b15
📒 Files selected for processing (28)
CHANGELOG.mdmlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/include/mlir/Dialect/QC/IR/QCTypes.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QCO/IR/QCOTypes.tdmlir/lib/Conversion/JeffToQCO/JeffToQCO.cppmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QC/IR/QCOps.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cppmlir/lib/Dialect/QCO/IR/QCOOps.cppmlir/lib/Dialect/QTensor/IR/Operations/AllocOp.cppmlir/unittests/Compiler/test_compiler_pipeline.cppmlir/unittests/Conversion/QCOToQC/test_qco_to_qc.cppmlir/unittests/Conversion/QCToQCO/test_qc_to_qco.cppmlir/unittests/Conversion/QCToQIR/test_qc_to_qir.cppmlir/unittests/Dialect/QC/IR/test_qc_ir.cppmlir/unittests/Dialect/QCO/IR/test_qco_ir.cppmlir/unittests/Dialect/QIR/IR/test_qir_ir.cppmlir/unittests/programs/qc_programs.cppmlir/unittests/programs/qc_programs.hmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qco_programs.hmlir/unittests/programs/qir_programs.cppmlir/unittests/programs/qir_programs.h
… loop constructs in CtrlOp and InvOp, and ensure proper type casting in StaticOp creation.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp (1)
129-130:⚠️ Potential issue | 🟠 MajorPreserve the promoted control's concrete qubit type.
After the segment size adjustment (lines 122-126), the first original control is now
op.getInputTarget(0). Using hardcodedisStatic=falseloses the static flag if the promoted control was a static qubit.🛠️ Suggested fix
- auto arg = op.getBody()->addArgument( - QubitType::get(rewriter.getContext(), /*isStatic=*/false), op.getLoc()); + auto arg = op.getBody()->addArgument(op.getInputTarget(0).getType(), op.getLoc());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp` around lines 129 - 130, The new argument creation uses a hardcoded QubitType::get(..., /*isStatic=*/false) which discards the original control's static flag; fetch the original control's qubit type from op.getInputTarget(0) (or its type accessor), extract its isStatic property, and pass that boolean into QubitType::get(rewriter.getContext(), isStatic) when calling op.getBody()->addArgument so the promoted control preserves its concrete static/dynamic flag while keeping op.getLoc().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp`:
- Around line 129-130: The new argument creation uses a hardcoded
QubitType::get(..., /*isStatic=*/false) which discards the original control's
static flag; fetch the original control's qubit type from op.getInputTarget(0)
(or its type accessor), extract its isStatic property, and pass that boolean
into QubitType::get(rewriter.getContext(), isStatic) when calling
op.getBody()->addArgument so the promoted control preserves its concrete
static/dynamic flag while keeping op.getLoc().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ecdc976-232b-4f31-92ae-03315e16bc8c
📒 Files selected for processing (5)
mlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QC/IR/QCOps.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Modifiers/InvOp.cpp
… verification to ensure static qubit type, and update related operations to support static qubit handling.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp (1)
118-131:⚠️ Potential issue | 🔴 CriticalCapture the promoted control type before rewriting the segment sizes.
After Line 123 updates the operand/result segment sizes,
op.getControlsIn().back()no longer refers to the promoted control; it refers to the last remaining control. With mixed static/dynamic controls, this can create the new block argument with the wrong type and make the rewrittenqco.ctrlfail verification or dropisStatic.🛠️ Minimal fix
- auto newSegments = DenseI32ArrayAttr::get( - rewriter.getContext(), {segmentsAttr[0] - 1, segmentsAttr[1] + 1}); + auto promotedControlType = op.getControlsIn().back().getType(); + auto newSegments = DenseI32ArrayAttr::get( + rewriter.getContext(), {segmentsAttr[0] - 1, segmentsAttr[1] + 1}); op->setAttr(opSegmentsAttrName, newSegments); const auto opResultSegmentsAttrName = CtrlOp::getResultSegmentSizeAttr(); op->setAttr(opResultSegmentsAttrName, newSegments); // Add a block argument for the promoted target qubit, preserving the // control's type (including isStatic) - auto promotedControlType = op.getControlsIn().back().getType(); auto arg = op.getBody()->addArgument(promotedControlType, op.getLoc());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp` around lines 118 - 131, The code adjusts operand/result segment sizes before capturing the promoted control type, so op.getControlsIn().back() points to the wrong control after the rewrite; fix it by reading and storing the promoted control type into promotedControlType (from op.getControlsIn().back().getType()) before calling CtrlOp::getOperandSegmentSizeAttr/getResultSegmentSizeAttr and updating segments, then use that stored promotedControlType when calling op.getBody()->addArgument so the new block arg preserves the original control's type (including isStatic).mlir/lib/Dialect/QCO/IR/QCOOps.cpp (1)
71-75:⚠️ Potential issue | 🟠 MajorAliased region arguments still downcast static qubits to dynamic.
parseTargetAliasing()is shared byqco.ctrl,qco.inv, andqco.if, so hardcoding!qco.qubithere stripsisStaticfrom textual IR block arguments. Static-target programs will either fail verification later or lose the static bit during parsing. Please thread the corresponding operand types into this helper, or reorder the assembly so those types are available when the block arguments are created.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/IR/QCOOps.cpp` around lines 71 - 75, parseTargetAliasing currently hardcodes newArg.type to QubitType::get(..., isStatic=false), which strips the static bit for aliased region arguments used by qco.ctrl/qco.inv/qco.if; update parseTargetAliasing to accept or compute the intended operand type (preserve isStatic) instead of hardcoding — e.g., add a parameter (or retrieve the operand type before creating block args) and set newArg.type to that operand type rather than calling QubitType::get(..., /*isStatic=*/false); ensure callers in qco.ctrl, qco.inv and qco.if pass the correct static/dynamic qubit type so the static bit is preserved during parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp`:
- Around line 118-131: The code adjusts operand/result segment sizes before
capturing the promoted control type, so op.getControlsIn().back() points to the
wrong control after the rewrite; fix it by reading and storing the promoted
control type into promotedControlType (from op.getControlsIn().back().getType())
before calling CtrlOp::getOperandSegmentSizeAttr/getResultSegmentSizeAttr and
updating segments, then use that stored promotedControlType when calling
op.getBody()->addArgument so the new block arg preserves the original control's
type (including isStatic).
In `@mlir/lib/Dialect/QCO/IR/QCOOps.cpp`:
- Around line 71-75: parseTargetAliasing currently hardcodes newArg.type to
QubitType::get(..., isStatic=false), which strips the static bit for aliased
region arguments used by qco.ctrl/qco.inv/qco.if; update parseTargetAliasing to
accept or compute the intended operand type (preserve isStatic) instead of
hardcoding — e.g., add a parameter (or retrieve the operand type before creating
block args) and set newArg.type to that operand type rather than calling
QubitType::get(..., /*isStatic=*/false); ensure callers in qco.ctrl, qco.inv and
qco.if pass the correct static/dynamic qubit type so the static bit is preserved
during parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f92294df-f746-4c7d-844f-c93aef76d90e
📒 Files selected for processing (6)
mlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Dialect/QC/IR/QCOps.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/QCOOps.cppmlir/lib/Dialect/QTensor/IR/Operations/AllocOp.cpp
…nce QubitType handling in parser and printer for static qubits, and ensure proper type preservation during control operation adjustments.
Signed-off-by: simon1hofmann <119581649+simon1hofmann@users.noreply.github.com>
…erification logic for static qubit handling.
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 (2)
mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.h (1)
111-125:⚠️ Potential issue | 🟡 MinorUpdate the example to the new static type.
The snippet still shows
qc.static ... : !qc.qubit, butstaticQubit()now builds!qc.qubit<static>. Leaving the old form in the public builder docs makes the new type split easy to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.h` around lines 111 - 125, Update the documentation example for staticQubit(uint64_t) to use the new static qubit MLIR type; replace the MLIR snippet that shows "%q0 = qc.static 0 : !qc.qubit" with the new form "%q0 = qc.static 0 : !qc.qubit<static>" so the builder docs reflect that staticQubit() constructs a !qc.qubit<static> type.mlir/include/mlir/Dialect/QC/IR/QCOps.td (1)
72-90:⚠️ Potential issue | 🟠 MajorConstrain
qc.alloctoDynamicQubittoo.These builder changes only fix the C++ call sites. The op definition still advertises a generic
QubitTyperesult, so textual IR can still spellqc.alloc : !qc.qubit<static>, which weakens the new static/dynamic split at the ODS level.Suggested fix
- let results = (outs QubitType:$result); + let results = (outs DynamicQubit:$result);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/include/mlir/Dialect/QC/IR/QCOps.td` around lines 72 - 90, The op definition for qc.alloc in QCOps.td still declares a generic QubitType result; change the result constraint to the dynamic qubit type (e.g. replace "let results = (outs QubitType:$result);" with "let results = (outs DynamicQubitType:$result);"), update the assemblyFormat type($result) if necessary to reflect DynamicQubit, and update the builders block so the builder calls use DynamicQubitType::get(...) instead of QubitType::get(...) (the build(...) invocations in the builders array should be updated accordingly) so the ODS-level op signature enforces DynamicQubit.
♻️ Duplicate comments (2)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.td (2)
117-137:⚠️ Potential issue | 🟠 MajorDon’t leave
qco.staticasPure.Line 117 still marks
qco.staticas side-effect free, so generic MLIR CSE/hoisting may merge identicalqco.static 0ops even though each result is meant to start its own linear SSA ownership chain. That also leaves this file out of sync with the PR objective to treat static qubits as allocation-like.🔧 Minimal fix
-def StaticOp : QCOOp<"static", [Pure]> { +def StaticOp : QCOOp<"static", [MemoryEffects<[MemAlloc]>]> {In MLIR ODS, does marking an operation `Pure` allow CSE/hoisting of identical ops, and should an operation that creates a fresh linear SSA ownership chain be modeled with `MemoryEffects<[MemAlloc]>` instead?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/include/mlir/Dialect/QCO/IR/QCOOps.td` around lines 117 - 137, The StaticOp (qco.static) should not be marked Pure because identical ops must not be CSE/hoisted; remove the Pure trait from the op definition and instead declare memory effects to model allocation-like behavior (e.g., add MemoryEffects<[MemAlloc]> to the op traits or effects list), keeping the existing result type (StaticQubit) and builder (build(..., /*isStatic=*/true, index)) unchanged so the op is treated as allocation-like and won't be merged by generic CSE/hoisting.
101-104:⚠️ Potential issue | 🟡 MinorUse
static-originhere, not juststatic.Lines 101-104 are broader than the actual conversion rule. A dynamic-origin qubit can later have type
!qco.qubit<static>, but only qubits originating fromqco.staticmay have theirqco.deallocdropped during QCO→QC conversion.Based on learnings: `ConvertQCODeallocOp` erases `qco.dealloc` only for static-origin qubits.✏️ Suggested wording
- corresponding to static qubits may be erased. + corresponding to qubits originating from `qco.static` may be erased.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/include/mlir/Dialect/QCO/IR/QCOOps.td` around lines 101 - 104, The comment says the doc text is too broad; update the wording to mention static-origin specifically: change the description around QCO dealloc conversion to state that only deallocs for qubits with static-origin (i.e., those produced by qco.static) may be erased when converting back to QC; reference ConvertQCODeallocOp and qco.dealloc so readers know this is the conversion rule that only removes deallocs for static-origin qubits rather than any qubit typed !qco.qubit<static>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 77-83: The nested-region qubit map (qubitMap) is not being
populated for cloned modifier bodies, so when adding block arguments and storing
aliases only in targetsIn the inner regions have empty maps and asserts fail;
update the code paths that create modifier clones (e.g., the block-argument
handling in ConvertQCInvOp and the corresponding modifier conversion block in
QCToQCO.cpp) to seed qubitMap[clonedBodyRegion] with the same Value->Value alias
entries you place in targetsIn right after you add the block arguments, ensuring
nested qc.ctrl/qc.inv lookups find the captured qubit aliases in the
region-keyed map.
In `@mlir/lib/Dialect/QC/Builder/QCProgramBuilder.cpp`:
- Around line 82-86: QCProgramBuilder::staticQubit widened the parameter to
uint64_t but still builds a StaticOp that expects an I64Attr; add a fast-fail
bounds check in QCProgramBuilder::staticQubit (after checkFinalized() and before
calling StaticOp::create) that verifies index <=
std::numeric_limits<int64_t>::max() and fails immediately (e.g., throw, return
an error, or call llvm::report_fatal_error consistent with project conventions)
instead of allowing wraparound into negative values; reference
QCProgramBuilder::staticQubit and StaticOp::create when locating where to add
the check.
---
Outside diff comments:
In `@mlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.h`:
- Around line 111-125: Update the documentation example for
staticQubit(uint64_t) to use the new static qubit MLIR type; replace the MLIR
snippet that shows "%q0 = qc.static 0 : !qc.qubit" with the new form "%q0 =
qc.static 0 : !qc.qubit<static>" so the builder docs reflect that staticQubit()
constructs a !qc.qubit<static> type.
In `@mlir/include/mlir/Dialect/QC/IR/QCOps.td`:
- Around line 72-90: The op definition for qc.alloc in QCOps.td still declares a
generic QubitType result; change the result constraint to the dynamic qubit type
(e.g. replace "let results = (outs QubitType:$result);" with "let results =
(outs DynamicQubitType:$result);"), update the assemblyFormat type($result) if
necessary to reflect DynamicQubit, and update the builders block so the builder
calls use DynamicQubitType::get(...) instead of QubitType::get(...) (the
build(...) invocations in the builders array should be updated accordingly) so
the ODS-level op signature enforces DynamicQubit.
---
Duplicate comments:
In `@mlir/include/mlir/Dialect/QCO/IR/QCOOps.td`:
- Around line 117-137: The StaticOp (qco.static) should not be marked Pure
because identical ops must not be CSE/hoisted; remove the Pure trait from the op
definition and instead declare memory effects to model allocation-like behavior
(e.g., add MemoryEffects<[MemAlloc]> to the op traits or effects list), keeping
the existing result type (StaticQubit) and builder (build(...,
/*isStatic=*/true, index)) unchanged so the op is treated as allocation-like and
won't be merged by generic CSE/hoisting.
- Around line 101-104: The comment says the doc text is too broad; update the
wording to mention static-origin specifically: change the description around QCO
dealloc conversion to state that only deallocs for qubits with static-origin
(i.e., those produced by qco.static) may be erased when converting back to QC;
reference ConvertQCODeallocOp and qco.dealloc so readers know this is the
conversion rule that only removes deallocs for static-origin qubits rather than
any qubit typed !qco.qubit<static>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 108b6e8a-64ba-4ecc-a1af-e4da8861d049
📒 Files selected for processing (13)
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/include/mlir/Dialect/QCO/Utils/ValueOrdering.hmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QCO/Builder/QCOProgramBuilder.cppmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppmlir/lib/Dialect/QTensor/IR/CMakeLists.txtmlir/unittests/programs/qc_programs.cppmlir/unittests/programs/qco_programs.cppmlir/unittests/programs/qir_programs.cpp
Signed-off-by: burgholzer <burgholzer@me.com>
There was a problem hiding this comment.
I very briefly scrolled through the changes. As my requested changes have been implemented and to not block this PR from being merged, I'll go ahead and approve this now. Thanks a lot, @simon1hofmann and @burgholzer!
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 138-143: The free helper function lookupMappedQubit has external
linkage but per project convention should use internal linkage; change its
declaration to static (i.e., make lookupMappedQubit static) and apply the same
change to the other free helper functions in this translation unit that are
defined outside an anonymous namespace so they use static linkage as well;
locate these helpers by finding free functions in QCToQCO.cpp (those not in a
class or namespace) and prefix their declarations with static.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 591a03fa-e78e-4ffc-a729-0f3606655aa0
📒 Files selected for processing (2)
mlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Conversion/QCToQCO/QCToQCO.cpp
Signed-off-by: burgholzer <burgholzer@me.com>
burgholzer
force-pushed
the
static_qubit_handling
branch
from
e3281e5 to
2b6f808
Compare
Signed-off-by: burgholzer <burgholzer@me.com>
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Okay, I think I am through with this now.
Cleaned up the conversion code from QC to QCO a little bit to make it less repetitive.
Fixed one bug in the Mapping related code where it did not cover all cases of operations. Would still be interested in an opinion from @MatthiasReumann whether walking the entire program is really the best option here; probably not.
After that, this should be good to go.
MatthiasReumann
left a comment
MatthiasReumann
left a comment
There was a problem hiding this comment.
Great work @simon1hofmann 🚀
It's really interesting to see how a seemingly small change (such as adding an attribute to a type) can have significant consequences for some tasks.
I have one small and one bigger remark - the latter being the more important one.
| rewriter.setInsertionPoint(q.getDefiningOp()); | ||
| auto op = rewriter.replaceOpWithNewOp<StaticOp>(q.getDefiningOp(), hw); | ||
| statics[hw] = op.getQubit(); | ||
| statics[hw] = llvm::cast<TypedValue<QubitType>>(op.getQubit()); |
There was a problem hiding this comment.
IntelliSense tells me that the return type here is already ::mlir::TypedValue< ::mlir::qco::QubitType>. Why the cast?
There was a problem hiding this comment.
Have you regenerated your build folder? I was wondering the same, but apparently something changed by adding the additional attribute so that the getter is no longer returning the actual type, but rather a generic type.
Let's look into this a little bit more.
There was a problem hiding this comment.
We just had a look at this. Apparently, this is because qco.static returns the StaticQubit type which exists in Tablegen but doesn't exist as class in C++ (In other words: Tablegen doesn't generate a class for it). Consequently, the getQubit uses the generic ::mlir::Type class. If we update the qco.static operation to return the QubitType (which exists as generated C++ class) it correctly types the return value.
There was a problem hiding this comment.
Thus, there are two options:
- Use the
QubitTypeas return type for theqco.staticoperation (Instead ofStaticQubit) - Cast to
QubitTypein the passes
There was a problem hiding this comment.
Hm. The first one loses a bit of type information, but leads to cleaner pass code. I think I would prefer that.
Maybe as a third option: Find a solution so that different C++ types are emitted for the different qubit types. I remember playing with this for a bit while working on the PR, but did not find a solution that I was really happy with.
There was a problem hiding this comment.
I think the only option to generate two different types is to define two different types. Then the Unitary operations had to do something like
def HOp : Op<"h", [ ... ]> {
let arguments = (ins AnyTypeOf<[StaticQubitType, DynamicQubitType]>:$qubit);
}
There was a problem hiding this comment.
Which is not particularly pretty as well..
Hm.. I am starting to dislike the complexity this distinction introduces..
There was a problem hiding this comment.
I was thinking the same. I discussed this a little bit with @simon1hofmann, but there is no big benefit of adding another type besides less "management" logic in the conversion passes, right?
| const auto qIn = dyn_cast<QubitType>(input.getType()); | ||
| const auto qOut = dyn_cast<QubitType>(output.getType()); | ||
| if (qIn && qOut && qIn != qOut) { | ||
| output.setType(qIn); |
There was a problem hiding this comment.
I think the best practice would be to replace each unitary op (which consumes and produces !qco.qubit types) with the same one but different typing. Generally speaking, any IR rewriting should be done via the IRRewriter (or PatternRewriter if you work on the conversions).
From the documentation:
Note that this [.setType()] is an extremely dangerous operation which can create completely invalid IR very easily. It is strongly recommended that you recreate IR objects with the right types instead of mutating them in place.
That being said, I don't think there is a cleaner option than traversing the IR another time. Alternatively, one could do it in the commitTrial function - but things will get messy then. Gemini's suggestion was to use the Conversion framework and the TypeConverter but that's a lot of boilerplate code for little functionality.
There was a problem hiding this comment.
Thanks for the hint with the IR rewriter 👍🏼
The conversion framework seems a little overkill here.
I was hoping that we could do this "on the fly" as part of the commitTrial function.
Let's entertain that idea for a bit. I am not sure whether I will have the capacity to look at this today; maybe @simon1hofmann or @MatthiasReumann you got some time to take a look?
There was a problem hiding this comment.
Yeah. I think it is possible to do this in the commitTrial function, the question is how ugly does that get. Maybe it's not even that bad. @simon1hofmann Added you on Discord. Maybe let's discuss this in person.
Maybe something else entirely that could help this process: Have you ever thought about implementing the InferTypeOpInterface interface? @burgholzer @denialhaag
There was a problem hiding this comment.
fun fact: before this PR, most operations in this PR automatically got that interface, simply by including the respective header, which we found out a couple months back. Now, with this PR, we apparently lost some of that. It's a nice exercise to compare the generated dialect files in the build folder before and after this PR.
while working on this PR a couple days back, I had already implemented the interface for the modifier and SCF operations, where the benefit was most apparent. However, I reverted those again as there was little to no perceived benefit for quite some boilerplate.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp (1)
577-580:⚠️ Potential issue | 🔴 CriticalThe pass still breaks when a function already contains
qco.staticwires.
collectDynamicQubits()only seeds routing fromqco.alloc, butplace()later materializes a freshqco.staticbank for every hardware slot. That means mixed static/dynamic two-qubit gates never enter the layering logic,dyn.size()undercounts already-occupied hardware, and this can emit duplicateqco.static Nvalues or assign a dynamic wire to an already-fixed hardware index. Even a pure-static circuit is no longer a no-op here. Please either reject pre-existing static inputs up front or reserve/seed those hardware indices during layering and placement.Also applies to: 692-740
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp` around lines 577 - 580, The logic undercounts occupied hardware because collectDynamicQubits() only seeds routing from qco.alloc and ignores pre-existing qco.static wires, causing dyn.size() checks and place() to produce duplicate qco.static indices or assign dynamics into already-fixed slots; update the pass to either (A) reject functions that already contain qco.static inputs up front by detecting any qco.static op in the function body and emitting an error from the pass, or (B) incorporate existing qco.static indices into the layering/placement seed by extending collectDynamicQubits() (and any routing/placement state used by place()) to also scan for qco.static ops and reserve those hardware indices before layering so that dyn.size() and place() account for already-occupied slots; modify the code paths around collectDynamicQubits(), the dyn.size() check, and place() to use the extended reservation model or the early rejection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 95-105: Add Doxygen-style comments for the new helper functions to
document their intent and invariants: for ctrlNeedsQubitTypeResync(CtrlOp),
invNeedsQubitTypeResync(InvOp), replaceCtrlPreservingBody(...), and
replaceInvPreservingBody(...). For each, add a brief description of what the
function does, list parameters and their roles (e.g., CtrlOp/InvOp and which
operands/regions are examined), state the return value semantics (true/false
meaning), and document any important invariants or side effects (e.g., that they
preserve the op body, resync qubit types, or rely on
modifierBodyArgsMismatchTargetOperands/qubitOperandAndResultTypesDiffer). Ensure
the comments follow Doxygen format (/// or /** */) consistent with nearby
helpers and include any preconditions callers must satisfy.
---
Outside diff comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 577-580: The logic undercounts occupied hardware because
collectDynamicQubits() only seeds routing from qco.alloc and ignores
pre-existing qco.static wires, causing dyn.size() checks and place() to produce
duplicate qco.static indices or assign dynamics into already-fixed slots; update
the pass to either (A) reject functions that already contain qco.static inputs
up front by detecting any qco.static op in the function body and emitting an
error from the pass, or (B) incorporate existing qco.static indices into the
layering/placement seed by extending collectDynamicQubits() (and any
routing/placement state used by place()) to also scan for qco.static ops and
reserve those hardware indices before layering so that dyn.size() and place()
account for already-occupied slots; modify the code paths around
collectDynamicQubits(), the dyn.size() check, and place() to use the extended
reservation model or the early rejection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f49cc0d7-479b-4714-920f-01f5ef263f80
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp`:
- Around line 665-666: The synchronizeMappedQubitTypes call currently runs after
commitTrial finishes, which allows SWAP rewiring (replaceAllUsesExcept) to
create uses mismatched with CtrlOp/InvOp block-argument types; move the call
into commitTrial so that immediately after place() completes (where dynamic
allocations are replaced via replaceOpWithNewOp) you call
synchronizeMappedQubitTypes to update block-argument and operand types before
any SWAP insertion loop runs, and remove the redundant
synchronizeMappedQubitTypes call that follows commitTrial.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93f7d809-4788-406e-93a1-7c84032b3877
📒 Files selected for processing (1)
mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
| commitTrial(*best, dyn, func.getFunctionBody(), rewriter); | ||
| synchronizeMappedQubitTypes(func.getFunctionBody(), rewriter); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runOnOperation ordering =="
sed -n '659,667p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== commitTrial placement and SWAP rewiring =="
sed -n '1013,1045p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== SWAP result accessors / result order =="
rg -n -C3 'def .*SWAP|Qubit0Out|Qubit1Out|getQubit0Out|getQubit1Out' \
mlir/include/mlir/Dialect/QCO/IR/QCOOps.td \
mlir/include/mlir/Dialect/QCO/IR \
mlir/lib/Dialect/QCO/IR 2>/dev/null || trueRepository: munich-quantum-toolkit/core
Length of output: 3883
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== place() function signature and implementation =="
rg -n -A20 '^[^/]*place\(' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp | head -50
echo
echo "== synchronizeMappedQubitTypes() implementation =="
rg -n -A15 'synchronizeMappedQubitTypes' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp | head -50
echo
echo "== Full SWAP operation definition (qubit0_out, qubit1_out) =="
sed -n '781,810p' mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdRepository: munich-quantum-toolkit/core
Length of output: 4654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full commitTrial() implementation =="
sed -n '1007,1100p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== toWires() function =="
rg -n -A10 'toWires' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp | head -30
echo
echo "== ctrlNeedsQubitTypeResync / invNeedsQubitTypeResync =="
rg -n -B5 -A10 'ctrlNeedsQubitTypeResync\|invNeedsQubitTypeResync' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp | head -60Repository: munich-quantum-toolkit/core
Length of output: 3581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for ctrlNeedsQubitTypeResync definition =="
rg -n 'ctrlNeedsQubitTypeResync|invNeedsQubitTypeResync|measureNeedsQubitTypeResync' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Lines around synchronizeMappedQubitTypes to understand the type resync =="
sed -n '305,360p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Check if these helper functions are in a header or same file =="
find mlir/lib/Dialect/QCO/Transforms -name "*.cpp" -o -name "*.h" | xargs grep -l 'ctrlNeedsQubitTypeResync' 2>/dev/null || echo "Not found in Transforms directory"
echo
echo "== Search wider in QCO directory =="
find mlir -path "*/QCO/*" -name "*.cpp" -o -name "*.h" | xargs grep -l 'ctrlNeedsQubitTypeResync' 2>/dev/null || echo "Pattern not found"Repository: munich-quantum-toolkit/core
Length of output: 2767
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full ctrlNeedsQubitTypeResync and invNeedsQubitTypeResync implementations =="
sed -n '107,135p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Full replaceCtrlPreservingBody and replaceInvPreservingBody implementations =="
sed -n '275,300p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppRepository: munich-quantum-toolkit/core
Length of output: 2422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WireIterator class definition =="
rg -n -B5 -A20 'class WireIterator|struct WireIterator' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== modifierBodyArgsMismatchTargetOperands definition =="
sed -n '60,90p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== qubitOperandAndResultTypesDiffer definition =="
rg -n -A8 'qubitOperandAndResultTypesDiffer' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp | head -20Repository: munich-quantum-toolkit/core
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for WireIterator =="
rg -n 'WireIterator' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp | head -20
echo
echo "== Search for modifierBodyArgsMismatchTargetOperands =="
rg -n -B3 -A12 'modifierBodyArgsMismatchTargetOperands' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp | head -50
echo
echo "== Search for qubitOperandAndResultTypesDiffer =="
rg -n -B2 -A8 'qubitOperandAndResultTypesDiffer' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppRepository: munich-quantum-toolkit/core
Length of output: 5581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WireIterator header file =="
cat mlir/include/mlir/Dialect/QCO/Utils/WireIterator.h
echo
echo "== Check if place() is indeed updating qubits or just in dynQubits map =="
sed -n '769,800p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppRepository: munich-quantum-toolkit/core
Length of output: 3810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full commitTrial function signature and start =="
sed -n '1007,1020p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Check what operations might have bodies after place() =="
echo "Looking for cloned operations and region handling..."
rg -n 'clone|Region|getBody|Body' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp | grep -E 'clone|Region.*Body' | head -15
echo
echo "== Check if CtrlOp/InvOp can appear as consumers during SWAP rewiring =="
rg -n 'CtrlOp|InvOp' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Examine the full runOnOperation to understand complete context =="
sed -n '650,670p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppRepository: munich-quantum-toolkit/core
Length of output: 3155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if operations with regions (CtrlOp, InvOp, etc) can be in the IR when commitTrial is called =="
echo "These would be consumers that have body qubits not yet replaced by place()"
echo
echo "== Understand the specific scenario: do CtrlOp/InvOp bodies consume dynamic qubits? =="
rg -n 'getBody\|getQubitsIn\|getTargetsIn' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp | head -20
echo
echo "== Check modifierBodyArgsMismatchTargetOperands more carefully =="
sed -n '82,90p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== The key question: when place() replaces dyn with static, are body args still pointing to old dyn? =="
echo "Looking at what place() actually modifies..."
sed -n '773,779p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Check if the body operations themselves need updating after place() =="
echo "Looking for calls to cloneModifierBodyAndResyncUnitary or similar during SWAP rewiring..."
sed -n '1015,1070p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppRepository: munich-quantum-toolkit/core
Length of output: 337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Specific search for getBody calls in Mapping.cpp =="
rg -n 'getBody()' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Search for how place() affects CtrlOp/InvOp =="
rg -n -B2 -A5 'replaceCtrlPreservingBody\|replaceInvPreservingBody' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp | head -40
echo
echo "== Look at lines 1015-1070 in smaller chunks =="
sed -n '1015,1040p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Look for any synchronization or body handling during SWAP loop =="
sed -n '1040,1070p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppRepository: munich-quantum-toolkit/core
Length of output: 529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check the actual structure of replaceCtrlPreservingBody =="
sed -n '239,255p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Check the actual structure of replaceInvPreservingBody =="
sed -n '261,272p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Understand cloneModifierBodyAndResyncUnitary =="
sed -n '199,225p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== The key: does modifierBodyArgsMismatchTargetOperands detect type changes? =="
sed -n '82,90p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppRepository: munich-quantum-toolkit/core
Length of output: 2970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if there are tests or examples with CtrlOp/InvOp in Mapping context =="
find mlir -name "*test*" -o -name "*Test*" | xargs grep -l "CtrlOp\|InvOp" 2>/dev/null | xargs grep -l "Mapping\|mapping" 2>/dev/null | head -5
echo
echo "== Check if dynamic qubits can flow through CtrlOp/InvOp =="
rg -n 'ctrl.*dynamic\|inv.*dynamic\|dynamic.*ctrl\|dynamic.*inv' mlir/lib/Dialect/QCO/ 2>/dev/null | head -10
echo
echo "== Verify: does replaceAllUsesExcept perform type checking? =="
echo "MLIR doesn't type-check during op creation, only during verify() or when explicitly requested"
echo "So SWAP rewiring could create invalid types that are caught later"
echo
echo "== Check the actual line numbers and context more carefully =="
sed -n '660,670p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppRepository: munich-quantum-toolkit/core
Length of output: 215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Examine the mapping test file =="
wc -l mlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
echo
echo "== Search for tests with CtrlOp or InvOp =="
rg -n 'CtrlOp\|InvOp\|qco.ctrl\|qco.inv' mlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp | head -20
echo
echo "== Check test structure and dynamic qubits =="
rg -n 'dynamic\|alloc' mlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp | head -10Repository: munich-quantum-toolkit/core
Length of output: 218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check what's in the mapping test file =="
head -50 mlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp
echo
echo "== Search for test patterns =="
rg -n 'TEST\|qco\.' mlir/unittests/Dialect/QCO/Transforms/Mapping/test_mapping.cpp | head -20
echo
echo "== Check if Mapping pass is only applied to leaf unitaries (no modifiers) =="
echo "Looking at what quantum operations are typically in mapping scope..."
rg -n 'UnitaryOpInterface|dyn_cast<.*Op>' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp | head -20Repository: munich-quantum-toolkit/core
Length of output: 1548
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check what operations the Mapping pass handles in its legality =="
rg -n 'isLegal\|addDynamicallyLegalOp\|addIllegalOp' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Check if there's a pass definition or operation filtering =="
rg -n 'class.*Pass\|struct.*Pass\|runOnOperation\|getOperationName' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp | head -15
echo
echo "== Look at the learning notes about nested regions and qubit tracking =="
echo "From learnings: nested modifier bodies (qc.ctrl/qc.inv) do NOT use state.qubitMap"
echo "This suggests they may be present in IR but handled differently"
echo
echo "== Check if synchronizeMappedQubitTypes is called in other places =="
rg -n 'synchronizeMappedQubitTypes' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppRepository: munich-quantum-toolkit/core
Length of output: 139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Get the full MappingPass class definition =="
sed -n '337,370p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Check runOnOperation signature =="
rg -n 'void runOnOperation' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== View the complete runOnOperation function =="
sed -n '625,670p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
echo
echo "== Understand: are CtrlOp/InvOp with dynamic qubits even expected in this pass? =="
echo "Looking at what dyn contains..."
sed -n '640,655p' mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cppRepository: munich-quantum-toolkit/core
Length of output: 3708
Resync must occur between place() and SWAP rewiring, not after both operations.
place() replaces dynamic allocations with static qubits via replaceOpWithNewOp, updating operand uses in the main computation. However, block arguments in CtrlOp/InvOp bodies bound to old dynamic qubits remain unchanged. When SWAP rewiring then executes via replaceAllUsesExcept, it can connect fresh static outputs to operations (especially modifiers) whose body arguments still expect dynamic types, creating type mismatches before synchronizeMappedQubitTypes() runs.
Move synchronizeMappedQubitTypes() into commitTrial() immediately after place() completes, before the SWAP insertion loop. This ensures all operand and body-argument types align before rewiring introduces new uses.
Suggested fix
auto wires = toWires(place(dynQubits, result.layout, funcBody, rewriter));
+ synchronizeMappedQubitTypes(funcBody, rewriter);
DenseMap<Operation*, WireIterator*> seen;And remove the call at line 666:
commitTrial(*best, dyn, func.getFunctionBody(), rewriter);
- synchronizeMappedQubitTypes(func.getFunctionBody(), rewriter);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp` around lines 665 - 666,
The synchronizeMappedQubitTypes call currently runs after commitTrial finishes,
which allows SWAP rewiring (replaceAllUsesExcept) to create uses mismatched with
CtrlOp/InvOp block-argument types; move the call into commitTrial so that
immediately after place() completes (where dynamic allocations are replaced via
replaceOpWithNewOp) you call synchronizeMappedQubitTypes to update
block-argument and operand types before any SWAP insertion loop runs, and remove
the redundant synchronizeMappedQubitTypes call that follows commitTrial.
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Hm. Sorry to open up the box again, but I somewhat do not really feel comfortable with the degree of changes this necessitates, especially in the passes.
Can we take one step back once more and think about less invasive solutions to our overall problem?
Am I correct in assuming that our problems would be entirely gone if we would simply deallocate all the qubits, no matter if they are dynamic or static?
The only downside we identified from that was that the semantics in QC for static qubits are not "perfect" as the deallocation of a static qubit really does not make too much sense.
However, in the grand scheme of things, this might be the easiest solution.
@DRovara @simon1hofmann @MatthiasReumann @denialhaag opinions?
And sorry for the back and forth on this. Feels like some things only become apparent if one tries them out. I am really glad that we have the mapping pass implemented by now because it revealed the kind of complexity that changes like these may cause.
| /** @brief Copies op results into `SmallVector<Value>` (OpResult → Value). */ | ||
| [[nodiscard]] static llvm::SmallVector<Value> opResultsAsValues(Operation* op) { | ||
| llvm::SmallVector<Value> vals; | ||
| llvm::append_range(vals, op->getResults()); | ||
| return vals; | ||
| } |
There was a problem hiding this comment.
This is basically llvm::to_vector, right?
| /** | ||
| * @brief True if @p ctrl must be rebuilt so qubit types match after placement. | ||
| * | ||
| * @param ctrl Operation examined: sole region (targets + body args), operands | ||
| * (`getControlsIn()`, `getTargetsIn()`), and qubit results | ||
| * (`getOutputQubits()`). | ||
| * @return True when either (1) a body block argument type differs from the | ||
| * matching `getTargetsIn()` operand, or (2) any qubit result type | ||
| * differs from the paired input qubit type (via | ||
| * @ref modifierBodyArgsMismatchTargetOperands and | ||
| * @ref qubitOperandAndResultTypesDiffer). | ||
| * | ||
| * @details Pure predicate: no IR changes. Callers use this to skip redundant | ||
| * `replaceOpWithNewOp` work. | ||
| */ |
There was a problem hiding this comment.
Can't really help it. These docstrings feel very AI generated and somehow more complicated than they need to be. Also holds for other docstrings generated through this file as part of the PR.
There was a problem hiding this comment.
I'm not really a fan of the changes in Mapping.cpp anyways and first wanted to get feedback from @MatthiasReumann. I added the docstrings for context, but I guess they just make it even more confusing.
I think you could even keep the "The QC dialect doesn't dealloc static qubits" with just one qubit type. It just requires one more datastructure in the conversion passes that remembers which static qubits it still has to deallocate (QC -> QCO) and which deallocs are to be removed (QCO -> QC). (Even though I still have the strong opinion that I am not even sure if that much more logic in the conversions would be required:
Overall this is probably less code than the one currently added to |
This feels like a very reasonable proposal. The only concern that I have is the one that @DRovara brought up elsewhere: can we really always track which qubits are static; especially in the presence of control flow that depends on runtime values? |
Ah. I remember. For later reference: This would cause some troubles. Or: We constrain qubit-tensors to either have all static or all dynamic qubits. Otherwise, it's invalid IR1. Then the above shouldn't be a problem. Even if we were to SWAP the qubit states. Maybe @DRovara can comment on this! 🙋🏻 Footnotes
|
Currently, |
yeah, we kind of do. we would potentially deallocate wrong qubits in the example from above. Another thought that came to mind: what if deallocation in QC is always implicit, when a qubit or register goes out of scope. and in QCO every qubit, no matter which type, is deallocated before the value leaves the scope. does that somehow make sense? |
I don't really hate this solution - that's more or less what the individual types would have done. Of course, verifying that is another issue. The first thing I immediately think of here is: What if I have a function that takes a qubit as input and adds the qubit to a tensor inside the function body? Once again, no simple way to verify that (without iterating over the entire program and analyzing all calls) - whereas once again, this is literally what data types and type checking would be made for (and this verification would be in constant time then).
I don't have a super deep overview on how the I feel like this should not be that hard to handle for the translation though. Unless I'm missing something here, the dataflow is still unique in the context of SSA and linear types, so the
That would mean, however, that when translating from |
If we were to go the separate type route (in the sense of actually different C++ types), then we would run into the same/similar problems as we already see in the PR here that it is pretty awkward to convert from dynamic to static qubits, e.g., as part of placement and routing. Two observations:
I don't yet know what to make of that, but I wanted to bring it up.
I am pretty sure that the original example that we had, which we identified as problematic, was slightly different. A
"Deallocation in the middle of the block" does not really make sense to me. After a deallocation, the qubit is dead and its value is consumed. It does not make a difference whether that dealloc is moved to the end of the block in the sequence of operations or not as it does not interact with any other SSA value than the qubit value it consumed. |
I agree and that's a really good point. At the end of the day, no matter how much we argue from the "that's what the classical world would do", situations like these don't really exist in the classical world. So I don't really want to say "we need to have different types for these concepts", I just wanted to make sure we have a good explanation if we are ever asked why we don't.
Right, I didn't think about that. That's a non-issue then, agreed.
That's true. Pretty strong arguments, at least I can't think of anything against it. Although then I'd just add that I wouldn't be super opposed to @MatthiasReumann's suggestion of using a different term that's not Final question though:
Once again in the context of dynamic indices to arrays/tensors, if we have the following code: Would it be possible to translate from |
The last point is very important. Maybe, at the end of all of this, we need an LLM to go through the discussions on this issue and summarize the points so that we could have something like an RFC that describes how we reached the consensus.
I am also fine with renaming in QCO. I'd also be fine with
Generally, I would think this feels a bit artificial. The problem itself would be solved by writing the program as At which point it is clear which qubit needs to be deallocated. |
I guess that's the point of contention I still have. At least intuitively, it feels like we should support all variations of code that a user might come up with. Otherwise, a user might end up providing a code that they view as valid but is then compiled incorrectly. In classical compiler development, "Compiler Fuzzing" is a testing strategy where a very large number of random programs are created and used as input for the compiler, simply to make sure that it produces correct results. So at least in the classical world, it seems like they don't care how realistic an example is. Now, the one argument I see against that is: MLIR is an IR, so we expect to get the code from some other front-end (e.g. a higher-level language) that would never generate code like above. However, I only designed that example to be a minimally reproducible unit. More realistic situations where something like this may happen are when we have a |
I see this as a very valid counter argument. QC will probably never be what people write. People will write Guppy, Qiskit, PennyLane, Qrisp, etc. But even if QC is the starting point, the validity of your example very much depends on the semantics of qubit tensors. |
5 participants
Description
Add explicit isStatic flag to QC/QCO qubit types, make static qubits distinct from dynamic ones, preserve staticness through conversions, treat StaticOp as allocation, skip lowering dealloc for static qubits, emit deallocs at top-level returns for remaining dynamic qubits, and add tests and builders for static-qubit scenarios.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.