Conversation
simon1hofmann
added
enhancement
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdd an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QCToQCO as QC→QCO Pass
participant State as ConversionState
participant ConvertReturn as ConvertFuncReturnOp
participant QCO as QCO Dialect
Client->>QCToQCO: run conversion on function
QCToQCO->>State: update state.qubitMap for alloc/rewrites
loop process ops
QCToQCO->>State: update mapping / mark escapes
end
QCToQCO->>ConvertReturn: encounter func.return
ConvertReturn->>State: collect live mapped qubits
ConvertReturn->>ConvertReturn: sort qubits deterministically
loop per live qubit
ConvertReturn->>QCO: emit `qco.dealloc` (dynamic only)
end
ConvertReturn->>State: clear qubitMap
ConvertReturn->>QCToQCO: replace func.return operands
QCToQCO->>Client: return converted function
sequenceDiagram
participant Client
participant QCOToQC as QCO→QC Pass
participant ConvertDealloc as ConvertQCODeallocOp
participant QC as QC Dialect
Client->>QCOToQC: process `qco.dealloc`
QCOToQC->>ConvertDealloc: lower dealloc
ConvertDealloc->>ConvertDealloc: check operand.isStatic?
alt static qubit
ConvertDealloc->>QCOToQC: erase `qco.dealloc`
else dynamic qubit
ConvertDealloc->>QC: create `qc.dealloc` with converted qubit
ConvertDealloc->>QCOToQC: replace op
end
QCOToQC->>Client: continue lowering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
📝 Coding Plan
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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@burgholzer Would be great to get some first feedback about the proposed changes. The mapping currently does not work as only the type of dynamic qubits gets changed to static, which also has to be taken into account for all other gates that consumed those dynamic qubits and therefore still return the wrong type. |
denialhaag
left a comment
denialhaag
left a comment
There was a problem hiding this comment.
Thanks a lot for the improvements! This is already looking really good to me!
I have a few comments below, and most should be straightforward to fix. I'm not super sure how to proceed with the conversion from QC to QCO. For now, it should be fine to deallocate all qubits that are still being tracked, but this will change once we allow functions that can return qubits.
| * `qco.dealloc`. This pattern inserts `qco.dealloc` operations for all | ||
| * still-live qubits tracked in the lowering state right before the return. | ||
| */ | ||
| struct ConvertFuncReturnOp final : StatefulOpConversionPattern<func::ReturnOp> { |
There was a problem hiding this comment.
Here, we probably have to be a bit more careful. Once functions can return qubits, this won't work anymore.
There was a problem hiding this comment.
Does the implementation not treat returned qubits correctly? escapedQubits holds the return operands, and liveQubits is built by skipping those, so returned qubits are never deallocated.
There was a problem hiding this comment.
Ahh, my bad... After looking at the implementation more carefully now, I have three comments:
- Is the check for nested regions necessary? I don't think our current implementation would allow a
func.returnin any of our modifiers. - Is the
seenmap necessary? This would be relevant if several QC qubits are mapped to the same QCO qubit, right? If that happened, something went horribly wrong, right? - The current implementation doesn't allow for multiple functions anyway, but at the moment, the qubit map is created once for the entire conversion, so we probably shouldn't clear it at the end of the first function. 🤔
There was a problem hiding this comment.
Addressing your comments:
- makes sense, can be removed.
- I tried to simplify it.
- Not entirely sure, but I think the qubit map has to be cleared for
func.returnto be considered legal and for multiple functions there would be individual qubit maps anyway right?
There was a problem hiding this comment.
Regarding 3): I think this is what @li-mingbao already had set up to some degree in #1396
| auto qcoOp = qco::StaticOp::create(rewriter, op.getLoc(), | ||
| static_cast<int64_t>(op.getIndex())); |
There was a problem hiding this comment.
Is this cast necessary? The type should be the same for both operations, right?
There was a problem hiding this comment.
Because getIndex returns uint64_t and StaticOp::create expects int64_t.
There was a problem hiding this comment.
See also my other comment, I was also confused by this.
…bit and StaticQubit types, update conversion logic for deallocation operations, and enhance documentation for clarity.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.td (1)
102-126: 🧹 Nitpick | 🔵 TrivialConsider adding ODS-level type constraint for StaticOp result.
Unlike QC's
StaticOpwhich now usesStaticQubit:$qubit, QCO'sStaticOpresult is stillQubitType:$qubit(line 116) with runtime verification viahasVerifier=1. While the verifier enforces the invariant, textual IR likeqco.static 0 : !qco.qubitcan be parsed without error until verification runs.For consistency with QC and to catch malformed IR earlier, consider defining
StaticQubit/DynamicQubittype constraints in QCO (similar to lines 33-41 in QCOps.td) and constraining the result type:+def StaticQubit : Type< + And<[CPred<"::mlir::isa<::mlir::qco::QubitType>($_self)">, + CPred<"::mlir::cast<::mlir::qco::QubitType>($_self).getIsStatic()">]>, + "static qubit type (!qco.qubit<static>)">; def StaticOp : QCOOp<"static", [MemoryEffects<[MemAlloc]>]> { ... - let results = (outs QubitType:$qubit); + let results = (outs StaticQubit:$qubit);This is optional since the verifier provides functional correctness, but would provide stricter IR well-formedness at parse time.
🤖 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 102 - 126, The StaticOp currently returns a generic QubitType:$qubit with runtime-only checks (hasVerifier=1); add ODS-level type constraints named StaticQubit and DynamicQubit in QCO (mirroring the existing QC pattern) and change StaticOp to return StaticQubit:$qubit so malformed IR is rejected at parse time; update the Op builders/signatures that reference QubitType::$qubit (e.g., the OpBuilder in StaticOp) to construct the StaticQubit type and ensure the verifier remains consistent with the new constrained result type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@mlir/include/mlir/Dialect/QCO/IR/QCOOps.td`:
- Around line 102-126: The StaticOp currently returns a generic QubitType:$qubit
with runtime-only checks (hasVerifier=1); add ODS-level type constraints named
StaticQubit and DynamicQubit in QCO (mirroring the existing QC pattern) and
change StaticOp to return StaticQubit:$qubit so malformed IR is rejected at
parse time; update the Op builders/signatures that reference QubitType::$qubit
(e.g., the OpBuilder in StaticOp) to construct the StaticQubit type and ensure
the verifier remains consistent with the new constrained result type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1b777a5-c1d8-4bc8-a204-6b32dc3adbd2
📒 Files selected for processing (3)
mlir/include/mlir/Dialect/QC/IR/QCOps.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Conversion/QCOToQC/QCOToQC.cpp
…pdate StaticOp to return StaticQubit, and improve type casting in mapping transformations for better static qubit handling.
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/include/mlir/Dialect/QCO/IR/QCOOps.td`:
- Around line 82-88: The AllocOp currently declares its result as the generic
QubitType which allows static qubits in textual IR; change the op result type in
the op definition to the DynamicQubit constraint (replace QubitType with
DynamicQubit) so the op signature only accepts dynamic qubits, and update the
OpBuilder factory signatures (the build overloads that call QubitType::get(...,
/*isStatic=*/false)) to use DynamicQubit as the result type reference; ensure
any related verify/build code (AllocOp::verify and the build(...) calls) remains
consistent with DynamicQubit so textual IR cannot create a static allocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ad2d62c-d4ea-43d0-ab3e-63043bdc20f3
📒 Files selected for processing (4)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Conversion/QCOToQC/QCOToQC.cppmlir/lib/Dialect/QCO/IR/QCOOps.cppmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
…urnOp to improve live qubit collection, and enhance dynamic qubit handling in mapping transformations.
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/QCO/IR/QCOOps.td`:
- Around line 953-955: The BarrierOp (qco.barrier) must be updated to
participate in the pairwise type-preservation rollout: change its declaration so
each qubits_out operand/result is tied to the corresponding qubits_in (same
pattern used by XXPlusYYOp/TwoTargetTwoParameter) and add a verifier to ensure
the qubits_out types match qubits_in types; specifically update the BarrierOp
(BarrierOp / qco.barrier) TD to include the appropriate TypesMatchWith/trait or
explicit result-to-operand tie and implement/enable a verifier that rejects
mismatched types (e.g., disallow !qco.qubit<static> -> !qco.qubit), mirroring
the approach used by XXPlusYYOp and other two-target two-parameter ops.
- Around line 29-41: The SameOperandsAndResultType trait on the qco.ctrl op is
too strict given the split between !qco.qubit and !qco.qubit<static>; replace it
with a custom check that enforces pairwise input->output type equality
separately for controls and for targets rather than one global equality.
Concretely, remove SameOperandsAndResultType from the qco.ctrl op definition in
QCOOps.td and add either a custom trait or a verifier that iterates over
controls.getTypes() and the corresponding control result types to assert
equality, then does the same for targets.getTypes() and their results (match the
pattern used by the builder that uses controls.getTypes()/targets.getTypes() and
keep compatibility with the ReduceCtrl canonicalization logic). Ensure the
verifier/trai t uses the op name qco.ctrl and maps each input index to the
corresponding result index rather than requiring a single unified type across
all qubits.
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 149-152: The if-statement that conditionally inserts into
liveQubitsSet should use braces to avoid lint errors and accidental breakage: in
the loop over state.qubitMap (variables qcQubit, qcoQubit) wrap the body of the
if (!escapedQubits.contains(qcoQubit)) in { ... } and place
liveQubitsSet.insert(qcoQubit); inside those braces so the insert is clearly
scoped to the conditional.
- Around line 1231-1235: Remove the call to
populateReturnOpTypeConversionPattern() so the generic func::ReturnOp conversion
is not registered alongside the specialized ConvertFuncReturnOp; specifically,
keep patterns.add<ConvertFuncReturnOp>(typeConverter, context, &state) and the
target.addDynamicallyLegalOp<func::ReturnOp>(...) dynamic legality check, but
delete the populateReturnOpTypeConversionPattern(patterns, typeConverter) line
to ensure ConvertFuncReturnOp (which uses state.qubitMap and emits qco.dealloc)
is the sole return conversion and prevent the generic pattern from rebuilding
returns from adaptor.getOperands().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06603ade-89f1-4ab7-ac3b-a60fa5defeda
📒 Files selected for processing (3)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
| def XXPlusYYOp : QCOOp<"xx_plus_yy", traits = [UnitaryOpInterface, TwoTargetTwoParameter, | ||
| TypesMatchWith<"result type matches input", "qubit0_in", "qubit0_out", "$_self">, | ||
| TypesMatchWith<"result type matches input", "qubit1_in", "qubit1_out", "$_self">]> { |
There was a problem hiding this comment.
Include qco.barrier in this pairwise type-preservation rollout.
The simple value-producing qubit ops updated here now tie outputs to inputs, but BarrierOp still has independent qubits_out types and no verifier. That still admits IR like qco.barrier %q : !qco.qubit<static> -> !qco.qubit, which reintroduces the same type drift this PR is fixing.
🤖 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 953 - 955, The
BarrierOp (qco.barrier) must be updated to participate in the pairwise
type-preservation rollout: change its declaration so each qubits_out
operand/result is tied to the corresponding qubits_in (same pattern used by
XXPlusYYOp/TwoTargetTwoParameter) and add a verifier to ensure the qubits_out
types match qubits_in types; specifically update the BarrierOp (BarrierOp /
qco.barrier) TD to include the appropriate TypesMatchWith/trait or explicit
result-to-operand tie and implement/enable a verifier that rejects mismatched
types (e.g., disallow !qco.qubit<static> -> !qco.qubit), mirroring the approach
used by XXPlusYYOp and other two-target two-parameter ops.
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 458-461: The explicit llvm::cast in the map_range lambda is likely
unnecessary; inspect AllocOp's generated accessor (AllocOp::getResult) and if it
already returns TypedValue<QubitType> (or a compatible typed accessor), replace
the lambda body to return op.getResult() directly (keeping QubitValue as the
SmallVector element type) and remove llvm::cast; if the generated accessor is
untyped, keep the cast but consider using a safer .template
cast<TypedValue<QubitType>>() or a typed getter if available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e169feff-bd24-4e4e-b4b2-99f015262d3f
📒 Files selected for processing (3)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
…e equality checks for BarrierOp and CtrlOp, and update BarrierOp documentation for clarity on type preservation. Refactor ConvertFuncReturnOp to improve live 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 (1)
mlir/lib/Conversion/QCToQCO/QCToQCO.cpp (1)
1217-1240:⚠️ Potential issue | 🔴 Critical
func.func/func.callstill bypassLoweringState.
ConvertFuncReturnOpfixes escaping qubits, but function arguments and call results still go through the generic FuncConversions helpers. Those patterns never seed or updatestate.qubitMap, so a valid helper likefunc.func@f(%q: !qc.qubit) -> !qc.qubit { qc.h %q; func.return %q }will still hit thequbitMap.contains(...)asserts on the first gate, and the same applies after a qubit-returningfunc.call. This needs matching statefulfunc.func/func.callpatterns, or the legality checks should reject qubit-typed helper functions until that support exists. Based on learnings: In the QCToQCO conversion pass, the dynamic legality checks forfunc::CallOp,func::FuncOp, andfunc::ReturnOpassume operations are either all classical types or all qubit types.🛠️ Registration sketch
- populateFunctionOpInterfaceTypeConversionPattern<func::FuncOp>( - patterns, typeConverter); + patterns.add<ConvertFuncFuncOp>(typeConverter, context, &state); target.addDynamicallyLegalOp<func::FuncOp>([&](func::FuncOp op) { return typeConverter.isSignatureLegal(op.getFunctionType()) && typeConverter.isLegal(&op.getBody()); }); @@ - populateCallOpTypeConversionPattern(patterns, typeConverter); + patterns.add<ConvertFuncCallOp>(typeConverter, context, &state); target.addDynamicallyLegalOp<func::CallOp>( [&](const func::CallOp op) { return typeConverter.isLegal(op); });
ConvertFuncFuncOpshould seedstate.qubitMapfrom each original QC block argument to the converted QCO block argument, andConvertFuncCallOpshould record the mapping from each original QC call result to the new QCO call result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp` around lines 1217 - 1240, The func.func/func.call conversions are bypassing LoweringState (state.qubitMap) because you used populateFunctionOpInterfaceTypeConversionPattern and populateCallOpTypeConversionPattern; implement stateful conversion patterns instead: add a ConvertFuncFuncOp pattern that, when lowering a func::FuncOp, seeds state.qubitMap by mapping each original QC block argument to the new converted QCO block argument (so subsequent ops see those qubits), and add a ConvertFuncCallOp pattern that, when lowering a func::CallOp, records mappings from each original QC call result to the new QCO call result in state.qubitMap; register these patterns in place of (or in addition to) the populate* helpers and keep the dynamic legality checks (target.addDynamicallyLegalOp for func::FuncOp and func::CallOp) so qubit-typed helpers are only considered legal when state.qubitMap handling is present.
🤖 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/IR/Modifiers/CtrlOp.cpp`:
- Around line 258-277: The verify() method in CtrlOp must first validate that
the result segments' arities match the input counts: add checks that
getControlsOut().size() == getNumControls() and getTargetsOut().size() ==
getNumTargets() before the pairwise type loops; if a mismatch, return
emitOpError(...) indicating the controls_out/targets_out arity mismatch. After
these guards, keep the existing pairwise comparisons that compare
getControlsIn()[i].getType() vs getControlsOut()[i].getType() and
getTargetsIn()[i].getType() vs getTargetsOut()[i].getType().
In `@mlir/lib/Dialect/QCO/IR/Operations/StandardGates/BarrierOp.cpp`:
- Around line 80-92: In BarrierOp::verify(), ensure the output arity matches the
expected number before the pairwise loop: compute numTargets via getNumTargets()
then check that getQubitsOut().size() == numTargets (or equivalently
getQubitsOut().size() == getQubitsIn().size()) and emit an error via emitOpError
when it does not; only after this guard iterate over i from 0 to numTargets and
compare getQubitsIn()[i].getType() with getQubitsOut()[i].getType() to preserve
safety when textual IR supplies fewer results than inputs.
---
Outside diff comments:
In `@mlir/lib/Conversion/QCToQCO/QCToQCO.cpp`:
- Around line 1217-1240: The func.func/func.call conversions are bypassing
LoweringState (state.qubitMap) because you used
populateFunctionOpInterfaceTypeConversionPattern and
populateCallOpTypeConversionPattern; implement stateful conversion patterns
instead: add a ConvertFuncFuncOp pattern that, when lowering a func::FuncOp,
seeds state.qubitMap by mapping each original QC block argument to the new
converted QCO block argument (so subsequent ops see those qubits), and add a
ConvertFuncCallOp pattern that, when lowering a func::CallOp, records mappings
from each original QC call result to the new QCO call result in state.qubitMap;
register these patterns in place of (or in addition to) the populate* helpers
and keep the dynamic legality checks (target.addDynamicallyLegalOp for
func::FuncOp and func::CallOp) so qubit-typed helpers are only considered legal
when state.qubitMap handling is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 345a36cd-c98f-4011-8321-85e37966cdf9
📒 Files selected for processing (5)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/BarrierOp.cppmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
| LogicalResult CtrlOp::verify() { | ||
| // Pairwise input->output type equality for controls and targets separately | ||
| // (allows !qco.qubit and !qco.qubit<static> to differ between | ||
| // controls/targets) | ||
| for (size_t i = 0; i < getNumControls(); ++i) { | ||
| if (getControlsIn()[i].getType() != getControlsOut()[i].getType()) { | ||
| return emitOpError("qco.ctrl control ") | ||
| << i << " input type must match output type (got " | ||
| << getControlsIn()[i].getType() << " vs " | ||
| << getControlsOut()[i].getType() << ")"; | ||
| } | ||
| } | ||
| for (size_t i = 0; i < getNumTargets(); ++i) { | ||
| if (getTargetsIn()[i].getType() != getTargetsOut()[i].getType()) { | ||
| return emitOpError("qco.ctrl target ") | ||
| << i << " input type must match output type (got " | ||
| << getTargetsIn()[i].getType() << " vs " | ||
| << getTargetsOut()[i].getType() << ")"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate the result segment arities before the pairwise type checks.
getNumControls() and getNumTargets() come from the input segments, but these loops index the output segments immediately. Malformed textual IR with too few controls_out / targets_out results will walk past the result range here, and extra results currently go unnoticed. Add explicit getControlsOut().size() == getNumControls() and getTargetsOut().size() == getNumTargets() checks before the pairwise comparisons.
🛠️ Suggested guard
LogicalResult CtrlOp::verify() {
+ if (getControlsOut().size() != getNumControls()) {
+ return emitOpError(
+ "number of control results must match number of control operands");
+ }
+ if (getTargetsOut().size() != getNumTargets()) {
+ return emitOpError(
+ "number of target results must match number of target operands");
+ }
+
// Pairwise input->output type equality for controls and targets separately
// (allows !qco.qubit and !qco.qubit<static> to differ between
// controls/targets)🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 270-270: The getTargetsIn function returns NULL on error and this line dereferences the return value without checking for NULL.
Context: getTargetsIn()[i]
Note: [CWE-476] NULL Pointer Dereference. [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
(null-library-function-cpp)
[warning] 270-270: The getTargetsOut function returns NULL on error and this line dereferences the return value without checking for NULL.
Context: getTargetsOut()[i]
Note: [CWE-476] NULL Pointer Dereference. [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
(null-library-function-cpp)
[warning] 273-273: The getTargetsIn function returns NULL on error and this line dereferences the return value without checking for NULL.
Context: getTargetsIn()[i]
Note: [CWE-476] NULL Pointer Dereference. [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
(null-library-function-cpp)
[warning] 274-274: The getTargetsOut function returns NULL on error and this line dereferences the return value without checking for NULL.
Context: getTargetsOut()[i]
Note: [CWE-476] NULL Pointer Dereference. [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
(null-library-function-cpp)
🤖 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 258 - 277, The
verify() method in CtrlOp must first validate that the result segments' arities
match the input counts: add checks that getControlsOut().size() ==
getNumControls() and getTargetsOut().size() == getNumTargets() before the
pairwise type loops; if a mismatch, return emitOpError(...) indicating the
controls_out/targets_out arity mismatch. After these guards, keep the existing
pairwise comparisons that compare getControlsIn()[i].getType() vs
getControlsOut()[i].getType() and getTargetsIn()[i].getType() vs
getTargetsOut()[i].getType().
| LogicalResult BarrierOp::verify() { | ||
| // Pairwise input->output type equality (disallows e.g. !qco.qubit<static> -> | ||
| // !qco.qubit) | ||
| const auto numTargets = getNumTargets(); | ||
| for (size_t i = 0; i < numTargets; ++i) { | ||
| if (getQubitsIn()[i].getType() != getQubitsOut()[i].getType()) { | ||
| return emitOpError("qco.barrier qubit ") | ||
| << i << " input type must match output type (got " | ||
| << getQubitsIn()[i].getType() << " vs " | ||
| << getQubitsOut()[i].getType() << ")"; | ||
| } | ||
| } | ||
| return success(); |
There was a problem hiding this comment.
Verify the output arity before the pairwise type loop.
numTargets is derived from qubits_in, so getQubitsOut()[i] is unsafe when textual IR provides fewer results than inputs, and extra results are silently accepted today. Add a getQubitsOut().size() == getNumTargets() check before iterating.
🛠️ Suggested guard
LogicalResult BarrierOp::verify() {
// Pairwise input->output type equality (disallows e.g. !qco.qubit<static> ->
// !qco.qubit)
const auto numTargets = getNumTargets();
+ if (getQubitsOut().size() != numTargets) {
+ return emitOpError(
+ "number of output qubits must match number of input qubits");
+ }
for (size_t i = 0; i < numTargets; ++i) {
if (getQubitsIn()[i].getType() != getQubitsOut()[i].getType()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlir/lib/Dialect/QCO/IR/Operations/StandardGates/BarrierOp.cpp` around lines
80 - 92, In BarrierOp::verify(), ensure the output arity matches the expected
number before the pairwise loop: compute numTargets via getNumTargets() then
check that getQubitsOut().size() == numTargets (or equivalently
getQubitsOut().size() == getQubitsIn().size()) and emit an error via emitOpError
when it does not; only after this guard iterate over i from 0 to numTargets and
compare getQubitsIn()[i].getType() with getQubitsOut()[i].getType() to preserve
safety when textual IR supplies fewer results than inputs.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
mlir/lib/Dialect/QCO/IR/Operations/StandardGates/BarrierOp.cpp (1)
80-92:⚠️ Potential issue | 🟠 MajorAdd the missing output-arity guard before the pairwise type loop.
numTargetscomes fromqubits_in, sogetQubitsOut()[i]is still unsafe when textual IR provides fewer results, and extra results are accepted silently today. Please rejectgetQubitsOut().size() != numTargetsbefore indexing the result range.Suggested guard
LogicalResult BarrierOp::verify() { // Pairwise input->output type equality (disallows e.g. !qco.qubit<static> -> // !qco.qubit) const auto numTargets = getNumTargets(); + if (getQubitsOut().size() != numTargets) { + return emitOpError( + "number of output qubits must match number of input qubits"); + } for (size_t i = 0; i < numTargets; ++i) { if (getQubitsIn()[i].getType() != getQubitsOut()[i].getType()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlir/lib/Dialect/QCO/IR/Operations/StandardGates/BarrierOp.cpp` around lines 80 - 92, The verify() implementation in BarrierOp must check that the output arity matches the input arity before indexing getQubitsOut(); add a guard at the top of BarrierOp::verify that compares getQubitsOut().size() (or getNumResults()) to getNumTargets() (which is derived from getQubitsIn()) and return emitOpError(...) if they differ so you don't index out of bounds, then proceed with the existing pairwise type loop using getQubitsIn() and getQubitsOut().mlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cpp (1)
258-277:⚠️ Potential issue | 🟠 MajorValidate result segment sizes before indexing
controls_out/targets_out.These loops still derive their bounds from the input segments, so malformed textual IR with too few results will walk past the result ranges, and extra results currently go unnoticed. Guard both output segment sizes first.
Suggested guard
LogicalResult CtrlOp::verify() { + if (getControlsOut().size() != getNumControls()) { + return emitOpError( + "number of control results must match number of control operands"); + } + if (getTargetsOut().size() != getNumTargets()) { + return emitOpError( + "number of target results must match number of target operands"); + } + // Pairwise input->output type equality for controls and targets separately // (allows !qco.qubit and !qco.qubit<static> to differ between // controls/targets)🤖 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 258 - 277, The verify() method should first validate that the result segments for controls and targets have the expected sizes before indexing: check that getControlsOut().size() (or the appropriate result segment access) matches getControlsIn().size() / getNumControls() and that getTargetsOut().size() matches getTargetsIn().size() / getNumTargets(); if either mismatches, return emitOpError from CtrlOp::verify (e.g., "mismatched control/target result segment size") so malformed IR can't index past available results, then continue with the existing pairwise type-equality loops over getControlsIn()/getControlsOut() and getTargetsIn()/getTargetsOut().
🤖 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 258-277: The verify() method should first validate that the result
segments for controls and targets have the expected sizes before indexing: check
that getControlsOut().size() (or the appropriate result segment access) matches
getControlsIn().size() / getNumControls() and that getTargetsOut().size()
matches getTargetsIn().size() / getNumTargets(); if either mismatches, return
emitOpError from CtrlOp::verify (e.g., "mismatched control/target result segment
size") so malformed IR can't index past available results, then continue with
the existing pairwise type-equality loops over getControlsIn()/getControlsOut()
and getTargetsIn()/getTargetsOut().
In `@mlir/lib/Dialect/QCO/IR/Operations/StandardGates/BarrierOp.cpp`:
- Around line 80-92: The verify() implementation in BarrierOp must check that
the output arity matches the input arity before indexing getQubitsOut(); add a
guard at the top of BarrierOp::verify that compares getQubitsOut().size() (or
getNumResults()) to getNumTargets() (which is derived from getQubitsIn()) and
return emitOpError(...) if they differ so you don't index out of bounds, then
proceed with the existing pairwise type loop using getQubitsIn() and
getQubitsOut().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1eee49df-392b-4413-83d6-88748239223b
📒 Files selected for processing (5)
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/lib/Conversion/QCToQCO/QCToQCO.cppmlir/lib/Dialect/QCO/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QCO/IR/Operations/StandardGates/BarrierOp.cppmlir/lib/Dialect/QCO/Transforms/Mapping/Mapping.cpp
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
This feels very clean already 🙌🏼 Thanks @simon1hofmann 🙏🏼
Most of the comments are rather on the nitpick side of things, so take them with a grain of salt.
I very much like where this is heading.
After main is merged in here, there will probably be some auto formatting to the Tablegen files.
| } | ||
|
|
||
| def StaticOp : QCOOp<"static", [Pure]> { | ||
| def StaticOp : QCOOp<"static", [MemoryEffects<[MemAlloc]>]> { |
There was a problem hiding this comment.
Is there a particular reason for why we could not keep Pure here?
Or, asked differently, do we lose anything by not declaring this operation as pure?
| `!qc.qubit<static>` denotes a qubit that has been mapped to a fixed | ||
| hardware position. |
There was a problem hiding this comment.
| `!qc.qubit<static>` denotes a qubit that has been mapped to a fixed | |
| hardware position. | |
| `!qc.qubit<static>` denotes a qubit with a statically known identifier. |
I believe it is helpful to keep this a little more generic.
| `!qco.qubit<static>` denotes a qubit that has been mapped to a fixed | ||
| hardware position. |
There was a problem hiding this comment.
| `!qco.qubit<static>` denotes a qubit that has been mapped to a fixed | |
| hardware position. | |
| `!qco.qubit<static>` denotes a qubit with a statically known identifier. |
I believe it is helpful to keep this a little more generic.
|
|
||
| addConversion([ctx](jeff::QubitType /*type*/) -> Type { | ||
| return qco::QubitType::get(ctx); | ||
| return qco::QubitType::get(ctx, /*isStatic=*/false); |
There was a problem hiding this comment.
| return qco::QubitType::get(ctx, /*isStatic=*/false); | |
| return qco::QubitType::get(ctx); |
Shouldn't this be the default?
I guess it doesn't really hurt to be explicit, but it also does not really feel necessary.
| for (size_t i = 0; i < numTargets; ++i) { | ||
| if (getQubitsIn()[i].getType() != getQubitsOut()[i].getType()) { | ||
| return emitOpError("qco.barrier qubit ") | ||
| << i << " input type must match output type (got " | ||
| << getQubitsIn()[i].getType() << " vs " | ||
| << getQubitsOut()[i].getType() << ")"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Haven't checked in the IDE, but there might be a more concise way to write this, either via llvm::zip or via something like getQubitsIn().getTypes() == getQubitsOut().getTypes()
(i vaguely remember that something like that exists)
| returnValues.push_back(latest); | ||
| escapedQubits.insert(latest); | ||
| } else { | ||
| returnValues.push_back(adaptorOperand); |
There was a problem hiding this comment.
| returnValues.push_back(adaptorOperand); | |
| returnValues.emplace_back(adaptorOperand); |
| } | ||
|
|
||
| // Collect non-escaped live qubits for deallocation. | ||
| llvm::DenseSet<Value> liveQubitsSet; |
There was a problem hiding this comment.
| llvm::DenseSet<Value> liveQubitsSet; | |
| llvm::DenseSet<Value> liveQubits; |
This is me just being pedantic, but it is clear from the type that this is a set.
| for (const auto& [qcQubit, qcoQubit] : state.qubitMap) { | ||
| if (!escapedQubits.contains(qcoQubit)) { | ||
| liveQubitsSet.insert(qcoQubit); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't know if it changes much, but LLVM has the concept of intersections of sets, which could be a shorthand for some of the code here. Obviously not a must-have, but might create cleaner code overall.
| llvm::sort(liveQubits, [](Value a, Value b) { | ||
| auto* opA = a.getDefiningOp(); | ||
| auto* opB = b.getDefiningOp(); | ||
| if (!opA || !opB || opA->getBlock() != opB->getBlock()) { | ||
| return a.getAsOpaquePointer() < b.getAsOpaquePointer(); | ||
| } | ||
| return opA->isBeforeInBlock(opB); |
There was a problem hiding this comment.
Since this is also used in the QCO program builder, it might be nice to extract this into the utils file so that it can be reused and doesn't go out of sync over time.
| auto qcoOp = qco::StaticOp::create(rewriter, op.getLoc(), | ||
| static_cast<int64_t>(op.getIndex())); |
There was a problem hiding this comment.
See also my other comment, I was also confused by this.
3 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.