🗺️ MLIR Dialect Redesign #1264
🗺️ MLIR Dialect Redesign #1264
Conversation
burgholzer
added
refactor
|
At the moment, this PR is purely additive to the existing infrastructure.
For both of these dialects, the following operations are supported at the moment:
The following conversions or translations are available:
Convenience builders for all three program types (Quartz, Flux, QIR) have been created for testing. with canonicalizations applied at every stage. Noteworthy changes so far:
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
burgholzer
force-pushed
the
dialect-implementation
branch
from
0d43836 to
36b6748
Compare
denialhaag
requested review from
denialhaag
and removed request for
denialhaag
denialhaag
force-pushed
the
dialect-implementation
branch
2 times, most recently
from
9206426 to
dd59932
Compare
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
@denialhaag just some drive-by comments to knock you in the right direction 🙂
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks again for your continued work on this @denialhaag 🙌 this is great progress!
I think I managed to go through all of the latest changes and collect my thoughts.
I tried to keep the comments as redundancy-free as possible. So everything I annotated in one dialect or one conversion probably also holds for the others.
…oneously suggested)
Co-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com>
Co-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
mlir/include/mlir/Dialect/Quartz/Builder/QuartzProgramBuilder.h (2)
137-155:ClassicalRegisterstores non-owningStringRef- document lifetime requirements.The
ClassicalRegister::namefield is aStringRef, which doesn't own the underlying string data. If a caller passes a temporarystd::string, the stored reference will dangle. Current usages appear to pass string literals, but the API permits unsafe usage.Consider either:
- Changing to
std::string nameto own the data, or- Adding documentation that callers must ensure the string outlives the builder
168-169: Returned reference may become invalid after subsequent allocations.
allocClassicalBitRegisterreturnsClassicalRegister&, but the backing store isSmallVector<ClassicalRegister>(line 874). If the vector reallocates during a later call, previously returned references become dangling pointers.auto& c1 = builder.allocClassicalBitRegister(3, "c1"); auto& c2 = builder.allocClassicalBitRegister(3, "c2"); // may reallocate builder.measure(q, c1[0]); // c1 potentially dangling!Consider returning by value or using a stable container (
std::deque,std::list).mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp (1)
102-109: Reference invalidation applies here.As noted in the header review,
emplace_backreturns a reference to an element in aSmallVectorthat may be invalidated by future allocations. This is the implementation side of the issue flagged in the header.mlir/lib/Compiler/CompilerPipeline.cpp (1)
244-258:currentStagecounter only increments whenrecordIntermediatesis true.The summary at line 252 uses
currentStageto report processed stages, but this counter is only incremented inside therecord != nullptr && config_.recordIntermediatesblocks. IfprintIRAfterAllStagesis enabled independently (withoutrecordIntermediates), the summary will incorrectly report "Successfully processed 0 stages".Consider tracking
currentStageindependently:+ auto currentStage = 0; + const auto incrementStage = [&]() { ++currentStage; }; + // Stage 1: Quartz import + incrementStage(); if (record != nullptr && config_.recordIntermediates) { record->afterQuartzImport = captureIR(module); if (config_.printIRAfterAllStages) { - prettyPrintStage(module, "Quartz Import", ++currentStage, totalStages); + prettyPrintStage(module, "Quartz Import", currentStage, totalStages); } }Or ensure
printIRAfterAllStagesimpliesrecordIntermediatesat the config level.mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (1)
120-159: Potential duplicate global symbol if called with the same label twice.
createResultLabelcreates a global without checking if one already exists with the same name. If called multiple times with identicallabelandsymbolPrefix, this will create duplicate globals, potentially causing LLVM verification errors.Consider checking for existing globals before creating:
LLVM::AddressOfOp createResultLabel(OpBuilder& builder, Operation* op, const StringRef label, const StringRef symbolPrefix) { // Save current insertion point const OpBuilder::InsertionGuard insertGuard(builder); // Create the declaration at the start of the module auto module = dyn_cast<ModuleOp>(op); if (!module) { module = op->getParentOfType<ModuleOp>(); } + if (!module) { + llvm::reportFatalInternalError("Module not found"); + } + + const auto symbolName = + builder.getStringAttr((symbolPrefix + "_" + label).str()); + + // Check if global already exists + if (module.lookupSymbol<LLVM::GlobalOp>(symbolName)) { + // Reuse existing - create AddressOfOp to it + auto main = getMainFunction(op); + if (!main) { + llvm::reportFatalInternalError("Main function not found"); + } + auto& firstBlock = *(main.getBlocks().begin()); + builder.setInsertionPointToStart(&firstBlock); + return builder.create<LLVM::AddressOfOp>( + op->getLoc(), LLVM::LLVMPointerType::get(builder.getContext()), + symbolName); + } + builder.setInsertionPointToStart(module.getBody()); - - const auto symbolName = - builder.getStringAttr((symbolPrefix + "_" + label).str());mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp (1)
432-442: Potential index collision with default register name "c".When measurements lack register info, they're assigned to a synthetic register named
"c"with sequential indices starting fromnumResults. If the circuit also has explicit measurements to a register named"c", the indices could potentially overlap, leading to incorrect output labeling.Consider using a distinct synthetic name (e.g.,
"__unnamed__"or"__default__") or tracking whether"c"is already used as an explicit register:- registerResultMap.insert({{"c", numResults}, resultValue}); + registerResultMap.insert({{"__unnamed__", numResults}, resultValue});mlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.h (3)
144-162:ClassicalRegisterstores non-owningStringRef- document lifetime requirements.The
ClassicalRegisterstruct storesStringRef name, which is a non-owning reference. Since instances are stored inallocatedClassicalRegisters, there is a latent risk: if a caller passes a temporarystd::string, the storedStringRefwill become a dangling reference. Current test usages pass string literals (static storage), avoiding the issue.Either document the lifetime requirement clearly or switch to owned storage:
struct ClassicalRegister { - /// Name of the classical register + /// Name of the classical register. + /// @note The caller must ensure this string outlives the builder. StringRef name;Or use
std::stringfor safety:- StringRef name; + std::string name;
246-260: Documentation claims chaining but signature isvoid.The Doxygen comment for
OP_NAMEdoesn't explicitly mention a return type, but the pattern of the builder suggests chaining. The actual signature at line 260 isvoid OP_NAME(...), which doesn't support method chaining unlike other methods likedealloc()that returnFluxProgramBuilder&.Either update the signature to return
FluxProgramBuilder&for consistency, or clarify in docs that this is a void operation.
968-992:ctrlexample uses scalars where the API expectsValueRange.The documentation example shows:
{controls_out, targets_out} = builder.ctrl(q0_in, q1_in, [&](auto& b) {But the actual signature at line 990-992 takes
ValueRange controls, ValueRange targets, not individualValuearguments. The lambda signature also differs.Update the example to match the actual API:
auto [controls_out, targets_out] = builder.ctrl(ValueRange{q0_in}, ValueRange{q1_in}, [&](OpBuilder& b, ValueRange ts) { auto q1_res = b.x(ts.front()); return ValueRange{q1_res}; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
mlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.h(1 hunks)mlir/include/mlir/Dialect/Flux/FluxUtils.h(1 hunks)mlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h(1 hunks)mlir/include/mlir/Dialect/Quartz/Builder/QuartzProgramBuilder.h(1 hunks)mlir/include/mlir/Dialect/Utils/Utils.h(1 hunks)mlir/lib/Compiler/CompilerPipeline.cpp(1 hunks)mlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp(1 hunks)mlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp(1 hunks)mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp(1 hunks)mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp(1 hunks)mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp(1 hunks)mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cpp(1 hunks)mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp(1 hunks)mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp(1 hunks)mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp(1 hunks)mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp(1 hunks)mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/U2Op.cpp(1 hunks)mlir/tools/mqt-cc/mqt-cc.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
📚 Learning: 2025-12-08T14:55:43.880Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.880Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.hmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cppmlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/include/mlir/Dialect/Flux/FluxUtils.h
📚 Learning: 2025-12-07T09:10:31.820Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1355
File: src/qdmi/sc/Device.cpp:97-102
Timestamp: 2025-12-07T09:10:31.820Z
Learning: In the munich-quantum-toolkit/core repository, duplication of QDMI-related macros (such as IS_INVALID_ARGUMENT) across device implementations (e.g., in src/qdmi/sc/Device.cpp and src/qdmi/dd/Device.cpp) is acceptable as a temporary measure. The preferred long-term solution is to upstream these macros to the QDMI repository rather than creating local shared headers, so they can be reused across all dependent projects.
Applied to files:
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-10-09T13:20:11.483Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Applied to files:
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-12-02T07:37:46.860Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Applied to files:
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cppmlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp
📚 Learning: 2025-12-08T12:44:05.874Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.874Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cppmlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.hmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cppmlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-10-09T13:13:51.224Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Applied to files:
mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/U2Op.cppmlir/include/mlir/Dialect/Quartz/Builder/QuartzProgramBuilder.hmlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cppmlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.hmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cppmlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-10-09T13:14:10.178Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:219-221
Timestamp: 2025-10-09T13:14:10.178Z
Learning: The MQT Core project (munich-quantum-toolkit/core repository) uses the C++20 standard, not C++17. C++20 features such as abbreviated function templates (e.g., `const auto&` parameters) are supported and valid in this codebase.
Applied to files:
mlir/tools/mqt-cc/mqt-cc.cpp
📚 Learning: 2025-12-05T17:45:37.602Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
Applied to files:
mlir/tools/mqt-cc/mqt-cc.cppmlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp
📚 Learning: 2025-10-09T13:25:36.887Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReuseQubitsPattern.cpp:98-100
Timestamp: 2025-10-09T13:25:36.887Z
Learning: In MLIR code, when traversing parent operations to find a common block between two operations where one uses the result of another, explicit bounds checking is not necessary. MLIR's SSA properties and scope locality guarantees ensure that operations using results must be in compatible scopes and will always share a common ancestor in the operation hierarchy.
Applied to files:
mlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-10-07T15:30:42.946Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h:219-231
Timestamp: 2025-10-07T15:30:42.946Z
Learning: In the Layout class for MLIR quantum routing (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h), the swap method intentionally does NOT swap the hw fields in QubitInfo. This is correct because SSA values represent quantum states at fixed hardware locations, and only their program index associations change during a SWAP gate. The hw field indicates where an SSA value physically resides and remains constant.
Applied to files:
mlir/include/mlir/Dialect/Flux/FluxUtils.h
📚 Learning: 2025-10-09T04:30:29.071Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h:72-140
Timestamp: 2025-10-09T04:30:29.071Z
Learning: In the CrawlScheduler (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h), the persistent `layoutCopy` is updated across all lookahead layers via `remapQubitValue` calls. After each two-qubit gate is scheduled in a layer, its input qubits are remapped to output qubits, ensuring the next layer's crawl starts from those outputs. This progressive remapping through SSA values prevents the same operation from being scheduled in multiple lookahead layers.
Applied to files:
mlir/include/mlir/Dialect/Flux/FluxUtils.h
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp
📚 Learning: 2025-10-09T13:28:29.237Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/DeadGateEliminationPattern.cpp:42-45
Timestamp: 2025-10-09T13:28:29.237Z
Learning: In the MQTOpt MLIR dialect, linear types enforce single-use semantics where each qubit value can only be consumed once, preventing duplicate deallocations and making RAUW operations safe when replacing output qubits with corresponding input qubits in transformation patterns.
Applied to files:
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.h
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp
🧬 Code graph analysis (8)
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cpp (2)
mlir/include/mlir/Dialect/Utils/Utils.h (2)
mlir(18-42)variantToValue(30-40)mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXMinusYYOp.cpp (6)
op(37-70)op(37-38)build(75-82)build(75-78)getCanonicalizationPatterns(84-87)getCanonicalizationPatterns(84-85)
mlir/include/mlir/Dialect/Quartz/Builder/QuartzProgramBuilder.h (2)
mlir/include/mlir/Dialect/Quartz/IR/QuartzDialect.h (1)
mlir(52-119)mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp (25)
QuartzProgramBuilder(31-33)initialize(35-53)initialize(35-35)allocQubit(55-64)allocQubit(55-55)staticQubit(66-75)staticQubit(66-66)allocQubitRegister(77-100)allocQubitRegister(78-79)allocClassicalBitRegister(102-109)allocClassicalBitRegister(103-103)measure(115-118)measure(115-115)measure(120-127)measure(120-121)reset(129-132)reset(129-129)barrier(374-377)barrier(374-374)ctrl(383-388)ctrl(384-385)dealloc(394-407)dealloc(394-394)finalize(413-430)finalize(413-413)
mlir/include/mlir/Dialect/Flux/FluxUtils.h (5)
mlir/include/mlir/Dialect/Utils/Utils.h (1)
mlir(18-42)mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cpp (2)
op(36-70)op(36-37)mlir/lib/Dialect/Flux/IR/Operations/StandardGates/ROp.cpp (4)
op(36-53)op(36-37)op(62-79)op(62-63)mlir/lib/Dialect/Flux/IR/Operations/StandardGates/UOp.cpp (6)
op(36-55)op(36-37)op(64-84)op(64-65)op(93-112)op(93-94)mlir/unittests/pipeline/test_compiler_pipeline.cpp (2)
op(72-92)op(72-72)
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp (1)
mlir/include/mlir/Dialect/Quartz/IR/QuartzDialect.h (11)
mlir(52-119)getNumTargets(70-70)getNumControls(71-71)getNumPosControls(72-72)getNumNegControls(73-73)getQubit(75-83)getTarget(84-92)getPosControl(94-96)getNegControl(98-100)getNumParams(102-102)getParameter(104-109)
mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp (1)
mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (8)
getOrCreateFunctionDeclaration(88-118)getOrCreateFunctionDeclaration(88-90)createPointerFromIndex(161-169)createPointerFromIndex(161-162)createResultLabel(120-159)createResultLabel(120-122)setQIRAttributes(56-86)setQIRAttributes(56-56)
mlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp (3)
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp (3)
typeConverter(1150-1150)nodiscard(109-109)ctx(1146-1261)mlir/include/mlir/Dialect/Quartz/IR/QuartzDialect.h (1)
nodiscard(111-117)mlir/include/mlir/Dialect/Flux/IR/FluxDialect.h (1)
nodiscard(120-126)
mlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp (1)
mlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp (19)
op(520-538)op(521-522)op(560-576)op(561-562)op(598-617)op(599-600)op(647-671)op(648-649)op(698-717)op(699-700)op(1014-1039)op(1015-1016)op(1063-1115)op(1064-1065)op(1134-1143)op(1135-1136)context(1172-1233)patterns(1180-1180)typeConverter(1181-1181)
mlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h (1)
mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (10)
getMainFunction(31-54)getMainFunction(31-31)setQIRAttributes(56-86)setQIRAttributes(56-56)getOrCreateFunctionDeclaration(88-118)getOrCreateFunctionDeclaration(88-90)createResultLabel(120-159)createResultLabel(120-122)createPointerFromIndex(161-169)createPointerFromIndex(161-162)
🪛 ast-grep (0.40.0)
mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp
[warning] 172-172: 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] 179-179: 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] 211-211: 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] 212-212: 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] 225-225: 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] 226-226: 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)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (27)
mlir/include/mlir/Dialect/Flux/FluxUtils.h (2)
28-75: Inverse‑pair and merge helpers look structurally sound.The inverse‑pair and zero‑parameter merge templates correctly:
- Restrict to predecessors of the expected op type via
getDefiningOp<...>(),- Update qubit uses through
replaceOp(trivializing predecessors and the current op),- Preserve operand ordering and use consistent qubit indices.
Given the linear SSA discipline for qubit values in this codebase, the lack of explicit
hasOneUse()checks on the predecessor is acceptable here.
188-205: Fix namespace forTOLERANCEto avoid compile errors.Inside
namespace mlir::flux,utils::TOLERANCEdoes not resolve tomlir::utils::TOLERANCEunless there is ausingor a global::utils. This is very likely a hard compile error:if (std::abs(paramValue) > utils::TOLERANCE) {and similarly below.
Use the fully qualified name instead:
- if (std::abs(paramValue) > utils::TOLERANCE) { + if (std::abs(paramValue) > ::mlir::utils::TOLERANCE) {and at line 224 as well.
If
<cmath>is not already pulled in by other headers, it’s also worth adding it explicitly forstd::abs.⛔ Skipped due to learnings
Learnt from: denialhaag Repo: munich-quantum-toolkit/core PR: 1264 File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70 Timestamp: 2025-12-08T12:44:05.874Z Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp (2)
75-100:CtrlInlineGPhasematches the intended zero‑target GPhase semantics.The GPhase inlining pattern correctly:
- Restricts to
GPhaseOpbodies,- Creates
POps only for positive controls usinggetInputPosControl(i),- Replaces the
CtrlOpwith the resulting control outputs and erases the modifier.Given that
GPhaseOpis a global (zero‑target) phase and a controlled GPhase only has control qubits, producing outputs solely for positive controls is consistent with the design.
266-315:CtrlOp::verifyis robust and aligns with Unitary/Region invariants.The verifier enforces:
- Exactly a unitary + yield in the body region,
- Yield arity matching
getNumTargets(),- No duplicate qubits among controls and body inputs/outputs,
- No controlled
BarrierOp.This is a solid set of invariants for modifier correctness and should catch most structural issues early.
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp (1)
60-69: Clarify semantics ofRemoveTrivialCtrland avoid replacing with a nested op.
RemoveTrivialCtrlcurrently does:if (ctrlOp.getNumPosControls() > 0) { return failure(); } rewriter.replaceOp(ctrlOp, ctrlOp.getBodyUnitary());Here
getBodyUnitary()is an op insidectrlOp’s body region. Using it directly as the replacement is fragile: erasingctrlOpwill also destroy that nested op, and the intent (drop just the modifier vs. drop modifier + body) is unclear.If the goal is to keep the body and remove only the control wrapper, I’d suggest cloning the body before erasing
ctrlOp:LogicalResult matchAndRewrite(CtrlOp ctrlOp, PatternRewriter& rewriter) const override { if (ctrlOp.getNumPosControls() > 0) { return failure(); } - rewriter.replaceOp(ctrlOp, ctrlOp.getBodyUnitary()); + auto bodyUnitary = ctrlOp.getBodyUnitary(); + OpBuilder::InsertionGuard guard(rewriter); + rewriter.setInsertionPoint(ctrlOp); + auto* clonedBody = rewriter.clone(*bodyUnitary.getOperation()); + rewriter.replaceOp(ctrlOp, clonedBody->getResults()); return success(); }If instead the intended semantics really are “a
CtrlOpwithout controls is a no‑op including its body”, usingrewriter.eraseOp(ctrlOp)would communicate that more directly. In either case, relying onreplaceOpwith an op nested inside the one being erased is brittle.⛔ Skipped due to learnings
Learnt from: denialhaag Repo: munich-quantum-toolkit/core PR: 1264 File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70 Timestamp: 2025-12-08T12:44:05.874Z Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.Learnt from: denialhaag Repo: munich-quantum-toolkit/core PR: 1264 File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100 Timestamp: 2025-12-08T14:55:43.880Z Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.Learnt from: MatthiasReumann Repo: munich-quantum-toolkit/core PR: 1301 File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151 Timestamp: 2025-12-02T07:37:46.860Z Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/U2Op.cpp (1)
22-28: LGTM!The
U2Op::buildoverload correctly uses thevariantToValueutility to convert variant parameters and delegates to the existing build path. This follows the same pattern as other gate operations in the codebase.mlir/include/mlir/Dialect/Utils/Utils.h (1)
11-42: LGTM!The header is now self-contained with all required MLIR includes. The
TOLERANCEconstant andvariantToValueutility are well-documented and correctly implemented.mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cpp (2)
33-71: LGTM!The
MergeSubsequentXXPlusYYcanonicalization pattern correctly:
- Uses tolerance-based comparison for static beta values
- Falls back to SSA value equality when betas are not static constants
- Accumulates theta values using
arith::AddFOp- Trivializes the predecessor by replacing it with its input qubits
The implementation mirrors
XXMinusYYOpappropriately.
75-87: LGTM!The build overload and canonicalization pattern registration follow the established patterns in the codebase.
mlir/include/mlir/Dialect/Quartz/Builder/QuartzProgramBuilder.h (2)
229-798: Well-structured macro-generated gate API.The macro-based approach for declaring gate operations with varying arities (zero/one/two/three parameters, one/two targets) provides a clean and consistent API surface. The controlled (
c*) and multi-controlled (mc*) variants are properly generated.
800-863: LGTM!The modifier (
ctrl), deallocation (dealloc), and finalization (finalize) APIs are well-documented with clear examples showing both C++ usage and expected MLIR output.mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp (3)
31-53: LGTM!The constructor and initialization are well-structured. The constructor creates the module with an unknown location, and
initialize()properly:
- Loads the Quartz dialect
- Sets up the main function with
entry_pointpassthrough attribute- Creates the entry block for subsequent operations
55-100: LGTM!The qubit allocation methods correctly:
- Validate inputs (non-negative index, positive size)
- Track allocated qubits in
allocatedQubitsfor automatic deallocation- Apply register metadata where appropriate (
allocQubitRegister)Note that
staticQubitintentionally doesn't track qubits since static qubits represent pre-existing hardware resources rather than dynamically allocated ones.
394-407: LGTM!The
deallocmethod correctly validates that the qubit was previously allocated and removes it from tracking before creating theDeallocOp. The fatal error on double-deallocation provides clear feedback to API users.mlir/tools/mqt-cc/mqt-cc.cpp (2)
40-63: LGTM - cl::opt declarations are now correctly non-const.The command-line option declarations are properly defined without the
constqualifier, allowing LLVM'sParseCommandLineOptionsto mutate them as required.
121-132: Critical: Undefined identifiers - variable names don't match declarations.The
cl::optvariables are declared asInputFilename,OutputFilename,ConvertToQIR,EnableTiming,EnableStatistics, andPrintIRAfterAllStages(lines 40-63), but the code referencesINPUT_FILENAME,OUTPUT_FILENAME,CONVERT_TO_QIR,ENABLE_TIMING,ENABLE_STATISTICS, andPRINT_IR_AFTER_ALL_STAGES. These are undefined identifiers and will cause compilation failures.- const auto module = loadMLIRFile(INPUT_FILENAME, &context); + const auto module = loadMLIRFile(InputFilename, &context); if (!module) { - errs() << "Failed to load input file: " << INPUT_FILENAME << "\n"; + errs() << "Failed to load input file: " << InputFilename << "\n"; return 1; } // Configure the compiler pipeline QuantumCompilerConfig config; - config.convertToQIR = CONVERT_TO_QIR; - config.enableTiming = ENABLE_TIMING; - config.enableStatistics = ENABLE_STATISTICS; - config.printIRAfterAllStages = PRINT_IR_AFTER_ALL_STAGES; + config.convertToQIR = ConvertToQIR; + config.enableTiming = EnableTiming; + config.enableStatistics = EnableStatistics; + config.printIRAfterAllStages = PrintIRAfterAllStages;Also fix line 142:
- if (writeOutput(module.get(), OUTPUT_FILENAME).failed()) { - errs() << "Failed to write output file: " << OUTPUT_FILENAME << "\n"; + if (writeOutput(module.get(), OutputFilename).failed()) { + errs() << "Failed to write output file: " << OutputFilename << "\n";Likely an incorrect or invalid review comment.
mlir/lib/Compiler/CompilerPipeline.cpp (2)
65-81: LGTM - Clean pass manager configuration.The
addCleanupPassesandconfigurePassManagermethods are well-structured and straightforward.
262-267: LGTM -captureIRimplementation is clean.The redundant
flush()call has been removed as addressed from previous review.mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (2)
31-54: LGTM -getMainFunctionwith proper null safety.The function correctly handles the case where
opis neither aModuleOpnor has aModuleOpancestor, returningnullptrat line 37.
88-118: LGTM -getOrCreateFunctionDeclarationis well-implemented.The function properly checks for existing symbols, uses
InsertionGuardto preserve insertion point, and includes null safety with a fatal error for missing module.mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp (2)
249-253: LGTM - Improved error handling.The error handling now uses
llvm::errs()withreturn failure()instead ofreport_fatal_error, allowing graceful failure. Consider usingop.emitError()for consistency with MLIR diagnostics if desired.
909-960: LGTM - Block structure enforcement is well-documented.The
ensureBlocksmethod clearly documents the 4-block QIR base profile structure and handles the operation categorization appropriately.mlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.h (2)
155-161: LGTM - Proper bounds checking with clear error message.The
operator[]now includes proper bounds checking with both lower and upper bounds validation, and usesllvm::reportFatalUsageErrorfor a clear error message.
56-75: LGTM - Well-documented builder class with clear usage example.The class documentation clearly explains the linear type enforcement and provides a helpful usage example demonstrating the value semantics pattern.
mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp (1)
121-141: Linear qubit tracking viavalidateQubitValue/updateQubitTrackinglooks solid.The combination of validating inputs, erasing consumed qubits, and inserting new outputs gives you a clear linear-usage discipline for qubit SSA values, and it’s consistently reused by measurement, reset, barriers, and controlled/unitary helpers. This should catch use-after-consume errors early and keep the builder’s state coherent.
mlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp (1)
70-78: Stateful conversion pattern for Quartz→Flux (incl. nestedctrl) looks correct and coherent.
LoweringStateplus the templatedconvert*helpers andConvertQuartzCtrlOp/ConvertQuartzYieldOpform a consistent story:
- Top-level ops update
qubitMapdirectly; nested ops usetargetsIn/targetsOutkeyed byinCtrlOp.- Zero-target GPhase conversion correctly produces empty
targetsOutfor nestedctrl, matching Flux’s semantics for controlled global phase (controls only, no targets).ConvertQuartzYieldOpcleans up per-level state and decrementsinCtrlOp, preventing cross-talk between nested regions.Within this excerpt, the Quartz-to-Flux lowering logic for gates, barrier, and
ctrl/yieldappears sound.Also applies to: 121-471, 1060-1144
mlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp (1)
37-257: Flux→Quartz lowering cleanly captures the value→reference semantic shift.The templated
convert*helpers together with the specific patterns (alloc/static/measure/reset, gate families, barrier,ctrl/yield) andFluxToQuartzTypeConverterform a coherent inverse of Quartz→Flux:
- OpAdaptors supply already-converted
!quartz.qubitoperands, so no extra state is needed.- Gate conversions replace Flux outputs with the same Quartz references, matching in-place mutation.
ConvertFluxMeasureOpandConvertFluxResetOpcorrectly collapse Flux’s extra SSA results down to Quartz’s reference-only interface.ConvertFluxCtrlOpandConvertFluxYieldOpappropriately drop SSA results while preserving region semantics by cloning the body and wiring outputs back to the original qubit references.Within this excerpt, the Flux-to-Quartz conversion logic looks consistent and correct.
Also applies to: 271-281, 371-459, 761-909
| OwningOpRef<ModuleOp> QuartzProgramBuilder::finalize() { | ||
| // Automatically deallocate all remaining allocated qubits | ||
| for (const Value qubit : allocatedQubits) { | ||
| create<DeallocOp>(loc, qubit); | ||
| } | ||
|
|
||
| // Clear the tracking set | ||
| allocatedQubits.clear(); | ||
|
|
||
| // Create constant 0 for successful exit code | ||
| auto exitCode = create<arith::ConstantOp>(loc, getI64IntegerAttr(0)); | ||
|
|
||
| // Add return statement with exit code 0 to the main function | ||
| create<func::ReturnOp>(loc, ValueRange{exitCode}); | ||
|
|
||
| // Transfer ownership to the caller | ||
| return module; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider deterministic deallocation order in finalize().
The iteration over allocatedQubits (a DenseSet) has non-deterministic order. While this is functionally correct since all remaining qubits are independent, it may produce different IR output across runs, which can complicate debugging and testing.
Consider collecting the qubits into a sorted container before deallocation if deterministic output is desired:
OwningOpRef<ModuleOp> QuartzProgramBuilder::finalize() {
- // Automatically deallocate all remaining allocated qubits
- for (const Value qubit : allocatedQubits) {
+ // Collect qubits for deterministic deallocation order
+ SmallVector<Value> qubitsToDealloc(allocatedQubits.begin(),
+ allocatedQubits.end());
+ // Sort by some stable criterion if needed (e.g., operation order)
+ for (const Value qubit : qubitsToDealloc) {
create<DeallocOp>(loc, qubit);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp around lines
413-430, the loop deallocating qubits iterates over a DenseSet (allocatedQubits)
which yields a non-deterministic order; to produce deterministic IR, copy
allocatedQubits into a SmallVector, sort that vector by a stable key (e.g.,
compare Value::getAsOpaquePointer() or cast to uintptr_t) and then iterate the
sorted vector to create DeallocOp for each qubit, then clear allocatedQubits as
before.
There was a problem hiding this comment.
@denialhaag what @coderabbitai highlights here might actually be one of the reasons for our program equivalence checks not working too well or non-deterministically before I refactored them to be less strict. Maybe this could be resolved in some other way by establishing a deterministic order.
There was a problem hiding this comment.
@burgholzer You're absolutely right to highlight this connection! The non-deterministic iteration order over DenseSet could indeed produce different deallocation sequences across runs, which would make IR comparison fragile and potentially explain earlier equivalence check issues.
There are a few ways to establish deterministic order:
-
Track insertion order (most predictable): Maintain a
SmallVector<Value>alongside theDenseSetto preserve allocation order:// In header llvm::DenseSet<Value> allocatedQubits; SmallVector<Value> qubitAllocationOrder; // In allocQubit/allocQubitRegister allocatedQubits.insert(qubit); qubitAllocationOrder.push_back(qubit); // In finalize() for (const Value qubit : qubitAllocationOrder) { if (allocatedQubits.contains(qubit)) { create<DeallocOp>(loc, qubit); } }
-
Sort by pointer value (simpler but less semantic):
SmallVector<Value> qubitsToDealloc(allocatedQubits.begin(), allocatedQubits.end()); llvm::sort(qubitsToDealloc, [](Value a, Value b) { return a.getAsOpaquePointer() < b.getAsOpaquePointer(); });
The first approach is cleaner and preserves allocation semantics (LIFO/FIFO as desired), while the second is a minimal change but ties output to pointer addresses.
Question: Should we fix this in the current PR, or would you prefer to track it separately? If it was indeed contributing to equivalence check flakiness, it might be worth addressing now. I'm happy to help implement either approach if you'd like.
🧠 Learnings used
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp:96-117
Timestamp: 2025-12-08T23:41:55.972Z
Learning: In the QIR (Quantum Intermediate Representation) Builder (mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp), the `ptrCache` is intentionally shared between qubit and result pointer creation (in `staticQubit()` and `measure()` methods) because QIR uses opaque pointers and `inttoptr` conversions for both qubits and results. For any given index N, the LLVM IR pointer representation is identical whether it represents a qubit or a result, so the pointer only needs to be created once and can be safely reused across both contexts.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Operations/StandardGates/BarrierOp.cpp:45-54
Timestamp: 2025-12-09T00:55:11.926Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux), qubits follow single-user/linear-type semantics where each qubit value can only be consumed once, similar to MQTOpt. This invariant makes it safe to dereference getUsers().begin() in canonicalization patterns like MergeSubsequentBarrier in BarrierOp.cpp, as there will be at most one user per qubit output.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (10)
mlir/include/mlir/Dialect/Quartz/Builder/QuartzProgramBuilder.h (2)
125-155:ClassicalRegistershould own the register name string or document lifetime requirements.The
ClassicalRegisterstruct storesStringRef name, which is a non-owning reference. SinceClassicalRegisterinstances are stored inallocatedClassicalRegistersandallocClassicalBitRegisteracceptsStringRef, there's a latent risk: if a caller passes a temporarystd::string, the storedStringRefwill become a dangling reference.Current usages pass string literals (static storage), but the API permits unsafe usage. Consider either owning the string (e.g.,
std::string name) or documenting that callers must ensure string lifetime exceeds the builder's lifetime.
168-169: Reference invalidation risk: returned reference may dangle after subsequent allocations.
allocClassicalBitRegisterreturnsClassicalRegister&, but the backing store isSmallVector<ClassicalRegister>(line 874). If the vector grows during a later allocation, all previously returned references become invalid.auto& c1 = builder.allocClassicalBitRegister(3, "c1"); auto& c2 = builder.allocClassicalBitRegister(3, "c2"); // may reallocate builder.measure(q, c1[0]); // c1 potentially dangling!Consider returning by value instead, or use a stable container (e.g.,
std::list,std::deque):- ClassicalRegister& allocClassicalBitRegister(int64_t size, - StringRef name = "c"); + ClassicalRegister allocClassicalBitRegister(int64_t size, + StringRef name = "c");mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp (1)
102-109: Reference invalidation implementation concern.This is the implementation of the API issue flagged in the header. The
emplace_backreturns a reference to an element inallocatedClassicalRegisters(aSmallVector), which can be invalidated if the vector grows on subsequent calls.If returning by value (as suggested in the header review), update this to:
-QuartzProgramBuilder::ClassicalRegister& +QuartzProgramBuilder::ClassicalRegister QuartzProgramBuilder::allocClassicalBitRegister(int64_t size, StringRef name) { if (size <= 0) { llvm::reportFatalUsageError("Size must be positive"); } - return allocatedClassicalRegisters.emplace_back(name, size); + allocatedClassicalRegisters.emplace_back(name, size); + return {name, size}; }mlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.h (2)
132-162:ClassicalRegisterstores non-owningStringRef— document lifetime requirements or use owning storage.The
ClassicalRegisterstruct storesStringRef name, which is a non-owning reference. Since instances are stored inallocatedClassicalRegisters(aSmallVector), callers passing temporary strings (e.g.,std::stringtemporaries) will cause dangling references. Current tests pass string literals (static storage), masking this issue.Either switch to owning storage or document the requirement clearly:
struct ClassicalRegister { - /// Name of the classical register - StringRef name; + /// Name of the classical register (must have lifetime >= builder) + /// @note Callers must ensure the string outlives the builder instance. + /// Prefer string literals or strings with static storage duration. + StringRef name;Alternatively, use
std::stringfor owned storage if dynamic names are needed.
968-992: Documentation example doesn't match API signature.The
ctrlexample shows scalarValuearguments, but the actual signature requiresValueRange:// Example shows: {controls_out, targets_out} = builder.ctrl(q0_in, q1_in, [&](auto& b) { ... }); // Actual signature: std::pair<ValueRange, ValueRange> ctrl(ValueRange controls, ValueRange targets, ...);Update the example to reflect the actual API:
- * {controls_out, targets_out} = builder.ctrl(q0_in, q1_in, [&](auto& b) { - * auto q1_res = b.x(q1_in); - * return {q1_res}; - * }); + * auto [controls_out, targets_out] = builder.ctrl( + * ValueRange{q0_in}, ValueRange{q1_in}, + * [&](OpBuilder& b, ValueRange targets) -> ValueRange { + * auto q1_res = b.create<XOp>(loc, targets[0]).getQubitOut(); + * return ValueRange{q1_res}; + * });mlir/lib/Compiler/CompilerPipeline.cpp (1)
106-258:currentStagecounter still only increments whenrecordIntermediatesis true.This issue was flagged in a previous review but remains unfixed. The summary message at line 252 uses
currentStageto report processed stages, but this counter is only incremented insideconfig_.recordIntermediatesblocks (lines 112, 125, 138, 152, 167, 181, 195, 209, 224, 238). If a user enablesprintIRAfterAllStageswithout enablingrecordIntermediates, the summary incorrectly reports "Successfully processed 0 stages".Consider tracking
currentStageindependently of therecordIntermediatesflag:// Stage 1: Quartz import + if (config_.printIRAfterAllStages) { + prettyPrintStage(module, "Quartz Import", ++currentStage, totalStages); + } if (record != nullptr && config_.recordIntermediates) { record->afterQuartzImport = captureIR(module); - if (config_.printIRAfterAllStages) { - prettyPrintStage(module, "Quartz Import", ++currentStage, totalStages); - } }Apply similar changes to all stages to ensure
currentStageincrements wheneverprintIRAfterAllStagesis enabled, regardless ofrecordIntermediates.mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (1)
139-144: Potential duplicate global symbol if called with the same label twice.This function creates a global without checking if one already exists with the same name. If called multiple times with identical
labelandsymbolPrefix, this will create duplicate globals, potentially causing LLVM verification errors.Consider using
module.lookupSymbol<LLVM::GlobalOp>(symbolName)before creating a new global and reusing the existing one if found.mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp (1)
96-117: Consider using separate caches for qubit and result pointers.The
ptrCacheis shared betweenstaticQubit()(qubit indices) andmeasure()/allocClassicalBitRegister()(result indices). While QIR uses identicalinttoptrsemantics for both, sharing the cache could mask bugs where qubit and result pointers are accidentally conflated.- /// Cache static pointers for reuse - DenseMap<int64_t, Value> ptrCache; + /// Cache static qubit pointers for reuse + DenseMap<int64_t, Value> qubitPtrCache; + /// Cache static result pointers for reuse + DenseMap<int64_t, Value> resultPtrCache;mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp (2)
432-442: Default register name"c"can collide with user-defined registersMeasurements without register metadata are stored under the synthetic register name
"c"with indexnumResults. If the user also has an explicit classical register"c", entries will be merged into the same group inregisterResultMap/registerGroups, potentially scrambling array sizes and labels inaddOutputRecording.Consider using a reserved/synthetic name that cannot collide with user code (e.g.
"__unnamed__"), and centralize it as a constant.For example:
- registerResultMap.insert({{"c", numResults}, resultValue}); + static constexpr char kDefaultRegisterName[] = "__unnamed__"; + registerResultMap.insert({{StringRef(kDefaultRegisterName), numResults}, + resultValue});Also applies to: 1057-1112
515-521: Use MLIR diagnostics instead ofllvm::errs()for unsupported controlled GPhaseFor
GPhaseOpinside actrlregion you currently write tollvm::errs()and returnfailure(). This again fails the conversion without attaching an error to the offending op.Emitting an error on
opmakes it clear to users why the conversion failed and which operation is at fault:auto& state = getState(); if (state.inCtrlOp != 0) { - llvm::errs() << "Controlled GPhaseOps cannot be converted to QIR.\n"; - return failure(); + op.emitError("controlled global phase is not supported in QIR"); + return failure(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
mlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.h(1 hunks)mlir/include/mlir/Dialect/Flux/FluxUtils.h(1 hunks)mlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h(1 hunks)mlir/include/mlir/Dialect/Quartz/Builder/QuartzProgramBuilder.h(1 hunks)mlir/include/mlir/Dialect/Utils/Utils.h(1 hunks)mlir/lib/Compiler/CompilerPipeline.cpp(1 hunks)mlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp(1 hunks)mlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp(1 hunks)mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp(1 hunks)mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp(1 hunks)mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp(1 hunks)mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cpp(1 hunks)mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp(1 hunks)mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp(1 hunks)mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp(1 hunks)mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp(1 hunks)mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/U2Op.cpp(1 hunks)mlir/tools/mqt-cc/mqt-cc.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
📚 Learning: 2025-10-09T13:13:51.224Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Applied to files:
mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/U2Op.cppmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.hmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/Quartz/Builder/QuartzProgramBuilder.hmlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cppmlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.hmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cppmlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp
📚 Learning: 2025-12-08T14:55:43.880Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.880Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.hmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cppmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cppmlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp
📚 Learning: 2025-11-20T18:19:31.857Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1679-1715
Timestamp: 2025-11-20T18:19:31.857Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the compareSwapAndBridge and compareShuttlingAndFlyingAncilla methods intentionally use division by zero to produce infinity when distance reductions are zero or negative. This infinity value marks that option as highly unfavorable in cost comparisons, which is the intended behavior rather than a bug.
Applied to files:
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/include/mlir/Dialect/Flux/FluxUtils.h
📚 Learning: 2025-12-07T09:10:31.820Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1355
File: src/qdmi/sc/Device.cpp:97-102
Timestamp: 2025-12-07T09:10:31.820Z
Learning: In the munich-quantum-toolkit/core repository, duplication of QDMI-related macros (such as IS_INVALID_ARGUMENT) across device implementations (e.g., in src/qdmi/sc/Device.cpp and src/qdmi/dd/Device.cpp) is acceptable as a temporary measure. The preferred long-term solution is to upstream these macros to the QDMI repository rather than creating local shared headers, so they can be reused across all dependent projects.
Applied to files:
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-10-09T13:20:11.483Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Applied to files:
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-12-02T07:37:46.860Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Applied to files:
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cppmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cppmlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp
📚 Learning: 2025-12-08T12:44:05.874Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.874Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cppmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.hmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cppmlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.hmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cppmlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp
📚 Learning: 2025-10-09T13:28:29.237Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/DeadGateEliminationPattern.cpp:42-45
Timestamp: 2025-10-09T13:28:29.237Z
Learning: In the MQTOpt MLIR dialect, linear types enforce single-use semantics where each qubit value can only be consumed once, preventing duplicate deallocations and making RAUW operations safe when replacing output qubits with corresponding input qubits in transformation patterns.
Applied to files:
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.h
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp
📚 Learning: 2025-10-09T13:14:10.178Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:219-221
Timestamp: 2025-10-09T13:14:10.178Z
Learning: The MQT Core project (munich-quantum-toolkit/core repository) uses the C++20 standard, not C++17. C++20 features such as abbreviated function templates (e.g., `const auto&` parameters) are supported and valid in this codebase.
Applied to files:
mlir/tools/mqt-cc/mqt-cc.cpp
📚 Learning: 2025-12-05T17:45:37.602Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
Applied to files:
mlir/tools/mqt-cc/mqt-cc.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.hmlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp
📚 Learning: 2025-10-09T13:25:36.887Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReuseQubitsPattern.cpp:98-100
Timestamp: 2025-10-09T13:25:36.887Z
Learning: In MLIR code, when traversing parent operations to find a common block between two operations where one uses the result of another, explicit bounds checking is not necessary. MLIR's SSA properties and scope locality guarantees ensure that operations using results must be in compatible scopes and will always share a common ancestor in the operation hierarchy.
Applied to files:
mlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/include/mlir/Dialect/Flux/FluxUtils.hmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-10-07T15:30:42.946Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h:219-231
Timestamp: 2025-10-07T15:30:42.946Z
Learning: In the Layout class for MLIR quantum routing (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Layout.h), the swap method intentionally does NOT swap the hw fields in QubitInfo. This is correct because SSA values represent quantum states at fixed hardware locations, and only their program index associations change during a SWAP gate. The hw field indicates where an SSA value physically resides and remains constant.
Applied to files:
mlir/include/mlir/Dialect/Flux/FluxUtils.h
📚 Learning: 2025-10-09T04:30:29.071Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1237
File: mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h:72-140
Timestamp: 2025-10-09T04:30:29.071Z
Learning: In the CrawlScheduler (mlir/include/mlir/Dialect/MQTOpt/Transforms/Transpilation/Scheduler.h), the persistent `layoutCopy` is updated across all lookahead layers via `remapQubitValue` calls. After each two-qubit gate is scheduled in a layer, its input qubits are remapped to output qubits, ensuring the next layer's crawl starts from those outputs. This progressive remapping through SSA values prevents the same operation from being scheduled in multiple lookahead layers.
Applied to files:
mlir/include/mlir/Dialect/Flux/FluxUtils.h
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp
🧬 Code graph analysis (6)
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp (2)
mlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp (16)
op(520-538)op(521-522)op(560-576)op(561-562)op(598-617)op(599-600)op(647-671)op(648-649)op(698-717)op(699-700)op(1014-1039)op(1015-1016)op(1063-1115)op(1064-1065)op(1134-1143)op(1135-1136)mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (10)
getOrCreateFunctionDeclaration(88-118)getOrCreateFunctionDeclaration(88-90)createPointerFromIndex(161-169)createPointerFromIndex(161-162)createResultLabel(120-159)createResultLabel(120-122)getMainFunction(31-54)getMainFunction(31-31)setQIRAttributes(56-86)setQIRAttributes(56-56)
mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp (1)
mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (8)
getOrCreateFunctionDeclaration(88-118)getOrCreateFunctionDeclaration(88-90)createPointerFromIndex(161-169)createPointerFromIndex(161-162)createResultLabel(120-159)createResultLabel(120-122)setQIRAttributes(56-86)setQIRAttributes(56-56)
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp (3)
mlir/include/mlir/Dialect/Quartz/IR/QuartzDialect.h (11)
mlir(52-119)getNumTargets(70-70)getNumControls(71-71)getNumPosControls(72-72)getNumNegControls(73-73)getQubit(75-83)getTarget(84-92)getPosControl(94-96)getNegControl(98-100)getNumParams(102-102)getParameter(104-109)mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp (24)
op(37-56)op(37-38)op(65-72)op(65-66)op(81-100)op(81-82)op(109-124)op(109-110)getNumQubits(133-133)getNumQubits(133-133)getNumTargets(135-135)getNumTargets(135-135)getNumControls(137-139)getNumControls(137-137)getNumPosControls(141-141)getNumPosControls(141-141)getNumNegControls(143-145)getNumNegControls(143-143)getNumParams(233-233)getNumParams(233-233)getParameter(235-237)getParameter(235-235)getCanonicalizationPatterns(317-322)getCanonicalizationPatterns(317-318)mlir/lib/Dialect/Flux/IR/Operations/StandardGates/BarrierOp.cpp (16)
getNumQubits(79-79)getNumQubits(79-79)getNumTargets(81-81)getNumTargets(81-81)getNumControls(83-83)getNumControls(83-83)getNumPosControls(85-85)getNumPosControls(85-85)getNumNegControls(87-87)getNumNegControls(87-87)getNumParams(141-141)getNumParams(141-141)getParameter(143-145)getParameter(143-143)getCanonicalizationPatterns(157-160)getCanonicalizationPatterns(157-158)
mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp (5)
mlir/include/mlir/Dialect/Utils/Utils.h (1)
mlir(18-42)mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp (20)
op(78-94)op(78-79)getBodyUnitary(99-101)getBodyUnitary(99-99)getNumQubits(103-103)getNumQubits(103-103)getNumTargets(105-105)getNumTargets(105-105)getNumControls(107-109)getNumControls(107-107)getNumPosControls(111-111)getNumPosControls(111-111)getNumNegControls(113-115)getNumNegControls(113-113)build(149-160)build(149-150)build(162-172)build(162-164)getCanonicalizationPatterns(209-212)getCanonicalizationPatterns(209-210)mlir/lib/Dialect/Flux/IR/Operations/StandardGates/BarrierOp.cpp (24)
op(35-74)op(35-36)getNumQubits(79-79)getNumQubits(79-79)getNumTargets(81-81)getNumTargets(81-81)getNumControls(83-83)getNumControls(83-83)getNumPosControls(85-85)getNumPosControls(85-85)getNumNegControls(87-87)getNumNegControls(87-87)getInputQubit(89-89)getInputQubit(89-89)getOutputQubit(91-91)getOutputQubit(91-91)getInputTarget(93-98)getInputTarget(93-93)getOutputTarget(100-105)getOutputTarget(100-100)build(147-155)build(147-148)getCanonicalizationPatterns(157-160)getCanonicalizationPatterns(157-158)mlir/lib/Dialect/Flux/IR/Operations/StandardGates/POp.cpp (4)
build(55-59)build(55-56)getCanonicalizationPatterns(61-64)getCanonicalizationPatterns(61-62)mlir/lib/Dialect/Flux/IR/Operations/StandardGates/GPhaseOp.cpp (4)
build(54-58)build(54-55)getCanonicalizationPatterns(60-63)getCanonicalizationPatterns(60-61)
mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp (1)
mlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.h (1)
index(144-160)
mlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h (1)
mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (10)
getMainFunction(31-54)getMainFunction(31-31)setQIRAttributes(56-86)setQIRAttributes(56-56)getOrCreateFunctionDeclaration(88-118)getOrCreateFunctionDeclaration(88-90)createResultLabel(120-159)createResultLabel(120-122)createPointerFromIndex(161-169)createPointerFromIndex(161-162)
🪛 ast-grep (0.40.0)
mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp
[warning] 172-172: 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] 179-179: 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] 211-211: 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] 212-212: 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] 225-225: 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] 226-226: 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)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (64)
mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/U2Op.cpp (1)
22-28: LGTM!The build overload correctly converts variant parameters to Values using the
variantToValueutility and delegates to the existing build method. This follows the established pattern for parameterized gate operations in the dialect.mlir/include/mlir/Dialect/Utils/Utils.h (1)
11-42: LGTM!The utility header is now self-contained with all necessary includes. The
variantToValuefunction provides a clean abstraction for handling mixed literal/dynamic parameters across gate operations, andTOLERANCEis appropriately defined for floating-point comparisons.mlir/lib/Dialect/Flux/IR/Operations/StandardGates/XXPlusYYOp.cpp (2)
27-71: LGTM!The
MergeSubsequentXXPlusYYcanonicalization pattern correctly:
- Validates qubit identity across consecutive operations
- Uses tolerance-based comparison for static beta values (addressing earlier review feedback)
- Falls back to SSA value equality for dynamic beta values (line 56)
- Properly accumulates theta parameters and trivializes the predecessor
75-87: LGTM!The build overload and canonicalization registration follow the established patterns for parameterized gate operations in the Flux dialect.
mlir/include/mlir/Dialect/Quartz/Builder/QuartzProgramBuilder.h (2)
49-68: LGTM on the builder class structure and initialization API.The
QuartzProgramBuilderdesign provides a clean, type-safe interface for constructing quantum programs with automatic resource tracking.
235-290: Well-structured macro-generated API for zero-target and one-target gates.The macros effectively reduce boilerplate while maintaining comprehensive documentation for each gate variant (base, controlled, multi-controlled).
mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp (4)
31-53: LGTM on initialization flow.The builder correctly initializes the MLIR context, loads the Quartz dialect, creates a main function with the
entry_pointpassthrough attribute, and sets up the entry block for operation insertion.
55-100: LGTM on qubit allocation methods.The
allocQubit,staticQubit, andallocQubitRegistermethods properly validate inputs and track allocations inallocatedQubitsfor automatic deallocation during finalization.
394-407: LGTM on deallocation with double-deallocation protection.The
deallocmethod correctly useseraseto both check existence and remove from tracking in a single operation, providing clear error messaging for invalid deallocations.
413-430: LGTM on finalization logic.The
finalizemethod properly auto-deallocates remaining qubits, clears tracking state, creates exit code 0, and transfers module ownership to the caller.mlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.h (3)
1-23: LGTM!Copyright header and includes are appropriate. The necessary MLIR and LLVM headers are included for the builder functionality.
26-55: Well-documented class with clear usage example.The class-level documentation clearly explains linear type enforcement, the no-cloning theorem mapping, and provides a concise usage example showing the initialize → operations → finalize lifecycle.
1035-1066: LGTM on private state tracking.The combination of
validQubitsDenseSet andallocatedClassicalRegistersSmallVector provides appropriate tracking for the linear type enforcement documented in the class.mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp (9)
35-57: Constructor and initialization look correct.The constructor creates an unknown location and ModuleOp (builtin dialect, no issue). The
initialize()method loads the Flux dialect before creating Flux operations. The lifecycle is properly sequenced.
69-82: Good input validation with fatal error reporting.The
staticQubitmethod properly validates the non-negative index precondition and usesreportFatalUsageErrorfor clear diagnostics.
84-106: LGTM on register allocation.Size validation is present, and the loop correctly creates individual
AllocOps with register metadata attached.
108-115: Reference to emplaced element is safe but fragile.Returning a reference to
emplace_backresult is safe here since subsequent allocations won't invalidate prior references (SmallVector preserves validity for existing elements). However, if the vector reallocates during a futureemplace_back, only new references would be invalidated, not existing ones stored by callers.
121-141: Clean linear type tracking implementation.The
validateQubitValueandupdateQubitTrackinghelpers correctly implement the linear type semantics: validate-before-consume and atomic swap of tracking state.
147-179: Measurement and reset correctly update tracking.Both
measureoverloads andresetproperly callupdateQubitTrackingto swap the consumed input for the produced output, maintaining linear type invariants.
187-245: Gate macro implementations are well-structured.The macros correctly delegate controlled variants to
ctrl()and handle qubit tracking updates. The pattern of creating the op, extracting results, and updating tracking is consistent across all variants.
527-534: Barrier correctly tracks all qubits via zip iteration.The
llvm::zipensures 1-1 correspondence between input and output qubits when updating tracking, consistent with the UnitaryInterface guarantees mentioned in the learnings.
540-556:ctrlimplementation handles tracking for both controls and targets.The implementation correctly updates tracking for all control and target qubits after creating the
CtrlOp. Thellvm::zipiterations maintain the 1-1 correspondence.mlir/lib/Conversion/FluxToQuartz/FluxToQuartz.cpp (1)
1-911: LGTM! Well-structured conversion pass implementation.The Flux-to-Quartz conversion pass is well-organized with clear separation of concerns:
- Template helpers (lines 37-257) provide reusable conversion logic for different operation signatures
- Type converter (lines 271-282) handles the semantic shift from value to reference types
- Conversion patterns (lines 284-822) systematically cover all operations with consistent macro-driven code generation
- Pass implementation (lines 824-909) correctly configures the conversion target and legality constraints
The documentation is thorough, explaining the value-to-reference semantic transformation and providing concrete examples. Previous review concerns have been addressed.
mlir/tools/mqt-cc/mqt-cc.cpp (1)
121-144: Variable name mismatches prevent compilation.The code references non-existent identifiers. The
cl::optvariables declared at lines 40-63 use camelCase names (e.g.,InputFilename,OutputFilename,ConvertToQIR), but lines 121-144 reference SCREAMING_SNAKE_CASE names (e.g.,INPUT_FILENAME,OUTPUT_FILENAME,CONVERT_TO_QIR) that don't exist.Apply this diff to fix the identifier references:
// Load the input .mlir file - const auto module = loadMLIRFile(INPUT_FILENAME, &context); + const auto module = loadMLIRFile(InputFilename, &context); if (!module) { - errs() << "Failed to load input file: " << INPUT_FILENAME << "\n"; + errs() << "Failed to load input file: " << InputFilename << "\n"; return 1; } // Configure the compiler pipeline QuantumCompilerConfig config; - config.convertToQIR = CONVERT_TO_QIR; - config.enableTiming = ENABLE_TIMING; - config.enableStatistics = ENABLE_STATISTICS; - config.printIRAfterAllStages = PRINT_IR_AFTER_ALL_STAGES; + config.convertToQIR = ConvertToQIR; + config.enableTiming = EnableTiming; + config.enableStatistics = EnableStatistics; + config.printIRAfterAllStages = PrintIRAfterAllStages; // Run the compilation pipeline if (const QuantumCompilerPipeline pipeline(config); pipeline.runPipeline(module.get()).failed()) { errs() << "Compilation pipeline failed\n"; return 1; } // Write the output - if (writeOutput(module.get(), OUTPUT_FILENAME).failed()) { - errs() << "Failed to write output file: " << OUTPUT_FILENAME << "\n"; + if (writeOutput(module.get(), OutputFilename).failed()) { + errs() << "Failed to write output file: " << OutputFilename << "\n"; return 1; }Likely an incorrect or invalid review comment.
mlir/include/mlir/Dialect/Flux/FluxUtils.h (8)
1-17: Header setup looks good—all previous issues resolved!The include guard is now in place with
#pragma once, and the file correctly includes necessary dependencies without globalusing namespacestatements.
19-43: LGTM!The inverse-pair removal logic is correct. Both operations are properly trivialized by replacing them with their respective inputs.
45-75: LGTM!The two-target inverse-pair removal correctly verifies that both qubits are connected between the operations before removing them.
77-108: LGTM!The merge logic correctly creates a square operation and removes the predecessor. As discussed in previous reviews, the current implementation is functionally correct—the rewriter properly updates the use-def chain when
prevOpis replaced.
110-140: LGTM!The parameter merging logic is correct, and the comment at line 131 helpfully documents the operand indexing convention.
142-178: LGTM!The two-target merge correctly validates that both operations act on the same qubits before merging their parameters. The operand indexing comment at line 168 is helpful.
180-205: Excellent—floating-point comparison issue resolved!The tolerance-based check using
std::abs(paramValue) > utils::TOLERANCEcorrectly handles near-zero parameters that may result from floating-point arithmetic, addressing the previously flagged fragile equality comparison.
207-232: Excellent—floating-point comparison issue resolved here too!Consistent with the one-target version, this function now uses tolerance-based comparison for robust zero detection.
mlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp (12)
1-39: LGTM!License header, includes, and namespace setup are appropriate. The
using namespacedirectives are scoped within themlirnamespace, which is acceptable practice.
40-78: LGTM!The
LoweringStatestruct is well-documented with clear explanation of the reference-to-value semantics transformation. The state fields appropriately track qubit mappings and nested control operation context.
485-496: LGTM!The type converter correctly handles the primary conversion from
!quartz.qubitto!flux.qubitwhile passing through other types unchanged via identity conversion.
516-539: LGTM!The alloc conversion correctly establishes the initial qubit mapping and preserves register metadata. Using
try_emplaceis appropriate since each allocation creates a new SSA value.
556-577: LGTM!The dealloc conversion correctly looks up the latest qubit value and removes the mapping. The implementation assumes well-formed input where the qubit was previously allocated, which is reasonable.
594-618: LGTM!The static qubit conversion correctly creates the flux.static operation and establishes the mapping. Note the slight inconsistency:
AllocOpusestry_emplacewhileStaticOpuses direct assignment. Both are functionally equivalent for valid SSA input.
643-672: LGTM!Excellent handling of the semantic difference between dialects. The conversion correctly extracts the classical bit result for replacement while updating the qubit mapping with the post-measurement qubit value.
720-993: LGTM!The macro-generated converters effectively reduce boilerplate while maintaining consistent patterns. Documentation is inline, and macros are properly undefined after use.
1010-1040: Missing nestedctrlhandling for barrier operations.Unlike the single/two-target converters,
ConvertQuartzBarrierOpdoesn't checkstate.inCtrlOpand always usesqubitMapdirectly. If a barrier can appear inside aquartz.ctrlregion, the qubits should be looked up fromtargetsIninstead.If barriers inside ctrl regions are intentionally unsupported, consider adding a check:
LogicalResult matchAndRewrite(quartz::BarrierOp op, OpAdaptor /*adaptor*/, ConversionPatternRewriter& rewriter) const override { auto& state = getState(); + // Barriers inside ctrl regions are not yet supported + if (state.inCtrlOp != 0) { + return op.emitError("barrier inside ctrl region not supported"); + } auto& qubitMap = state.qubitMap;Please verify whether barriers can appear inside ctrl regions in the Quartz dialect.
1060-1116: LGTM!The
CtrlOpconversion correctly handles nested control operations through theinCtrlOpcounter andtargetsIn/targetsOutmaps. The sequence of incrementing the nesting counter before storing targets ensures correct level association. Region cloning preserves the body operations for subsequent conversion.
1130-1144: LGTM!The
YieldOpconversion correctly retrieves the converted target values fromtargetsOut, creates the flux yield, and cleans up state by erasing the map entry before decrementing the nesting counter.
1169-1234: LGTM!The pass implementation is well-structured:
- Stack-allocated
LoweringStateis safe since patterns execute synchronously- Conversion target correctly marks Quartz illegal and Flux legal
- All operation patterns are registered with state tracking
- Standard MLIR function and control-flow type conversion patterns are included
- Using
applyPartialConversionallows non-Quartz operations to remain unchangedmlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (4)
31-54: LGTM!The function correctly handles the null module case with an early return and properly searches for the entry_point attribute.
56-86: LGTM!The QIR attribute construction correctly implements the base profile metadata requirements with proper version (2.0), resource requirements, and management flags.
88-118: LGTM!The function correctly manages the symbol lookup, has proper null safety for the module, and appropriately marks irreversible operations.
161-169: LGTM!The utility correctly creates an LLVM pointer from an integer index using the standard constant + inttoptr pattern.
mlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h (5)
22-29: LGTM!The use of
inline constexprfor QIR function name constants ensures single definition across translation units.
31-70: LGTM!The macro effectively generates QIR function name constants for all standard gates and their controlled variants.
74-127: LGTM!The getter functions correctly map control counts to QIR function names. The
reportFatalUsageErrorcall in the default case properly handles unsupported control counts (the function is marked[[noreturn]]).
137-146: LGTM!The metadata struct is well-designed with appropriate defaults and clear documentation.
148-235: LGTM!The function declarations are well-documented with accurate details about their behavior, including the correct QIR version (2.0) in
setQIRAttributesdocumentation.mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp (8)
37-94: LGTM!The initialization correctly sets up the 4-block QIR Base Profile structure with proper block wiring, entry_point attribute, and initialization call placement.
135-163: LGTM!The classical register allocation correctly manages the measurements block insertion and tracks register-to-result mappings.
165-229: LGTM!The measure implementations correctly handle the measurements block insertion and result tracking. The runtime error for missing register slots (line 216) properly guards against invalid usage.
231-247: LGTM!The reset operation correctly targets the measurements block and uses proper function declaration handling.
253-318: LGTM!The function correctly manages insertion points for constants (entry block) and calls (body block), with proper argument ordering (controls, targets, parameters) matching QIR conventions.
320-543: LGTM!The macro-generated gate operations effectively reduce boilerplate while correctly handling control variants through
createCallOpand the appropriategetFnName*selectors.
549-618: LGTM!The output recording correctly groups measurements by register, sorts for deterministic output, and generates properly labeled QIR output calls.
620-627: LGTM!The finalization correctly generates output recording before setting QIR attributes and returning module ownership.
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp (1)
99-207: CtrlOp helpers and verification look consistent and robust.
getBodyUnitary, qubit/control/parameter accessors, builders, andverify()(region shape, uniqueness checks, Barrier disallowance) are coherent and align with the intended CtrlOp semantics in Quartz. No changes needed here from my side.mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp (1)
129-315: CtrlOp accessors, builders, and verification are well-structured.The qubit/control/target accessors with bounds checks, parameter forwarding, builders that clone or build the body and yield targets, and
verify()(body region shape, yield arity, uniqueness, Barrier restriction) form a consistent and safe API for FluxCtrlOp. The GPhase/Id inlining patterns also align with the documented zero-target GPhase semantics.mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp (1)
870-1261: Overall Quartz→QIR lowering structure looks solidThe staged conversion (func→LLVM, Quartz→QIR, then arith/cf→LLVM plus final cast reconciliation) combined with
LoweringStatefor metadata and output recording is well-structured and aligns with QIR base-profile requirements (static qubits/results, labeled outputs). No functional issues spotted in the high-level pipeline.
| Value fluxQubit; | ||
| if (inCtrlOp == 0) { | ||
| fluxQubit = qubitMap[quartzQubit]; | ||
| } else { | ||
| fluxQubit = state.targetsIn[inCtrlOp].front(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding assertions for defensive debugging.
The code assumes that when inCtrlOp != 0, the targetsIn vector has the expected number of elements. While this should hold for well-formed IR, assertions would help catch issues during development:
Value fluxQubit;
if (inCtrlOp == 0) {
fluxQubit = qubitMap[quartzQubit];
} else {
+ assert(!state.targetsIn[inCtrlOp].empty() &&
+ "Expected target qubit for one-target operation");
fluxQubit = state.targetsIn[inCtrlOp].front();
}This pattern applies to all similar .front() calls in the conversion helpers.
🤖 Prompt for AI Agents
In mlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp around lines 160-165, the
branch that uses state.targetsIn[inCtrlOp].front() assumes targetsIn has the
expected elements; add a defensive assertion before calling .front() to verify
the container is non-empty and that inCtrlOp is a valid index (e.g.,
assert(inCtrlOp >= 0 && static_cast<size_t>(inCtrlOp) < state.targetsIn.size()
&& !state.targetsIn[inCtrlOp].empty()) or use LLVM/MLIR assertion/reporting
APIs), and include a clear message indicating which quartzQubit/inCtrlOp failed
to help debugging; apply the same pattern to all other .front() uses in the
conversion helpers.
There was a problem hiding this comment.
@denialhaag I suppose the assertion proposed by @coderabbitai doesn't really hurt us here (and in other places)
There was a problem hiding this comment.
@burgholzer You're right—adding these assertions would be helpful for catching malformed IR during development. For MLIR code, you'd typically use assert() from <cassert> (which is already included) rather than LLVM's reporting APIs, since these are internal invariants.
The suggested assertion would look like:
if (inCtrlOp != 0) {
assert(!state.targetsIn[inCtrlOp].empty() &&
"Expected target qubit for one-target operation");
fluxQubit = state.targetsIn[inCtrlOp].front();
}This pattern should be applied consistently to similar .front() accesses in the other conversion helper templates (convertOneTargetOneParameter, convertTwoTargetZeroParameter, etc.) where state.targetsIn[inCtrlOp] is accessed. The assertions will be active in debug builds and help catch issues early if the IR structure doesn't match expectations.
🧠 Learnings used
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Operations/StandardGates/BarrierOp.cpp:45-54
Timestamp: 2025-12-09T00:55:11.926Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux), qubits follow single-user/linear-type semantics where each qubit value can only be consumed once, similar to MQTOpt. This invariant makes it safe to dereference getUsers().begin() in canonicalization patterns like MergeSubsequentBarrier in BarrierOp.cpp, as there will be at most one user per qubit output.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.899Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.883Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-101
Timestamp: 2025-12-08T23:16:20.680Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The CtrlInlineGPhase canonicalization pattern correctly only checks getNumPosControls() and processes only positive controls when inlining a GPhaseOp.
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
| // Return if there are already multiple blocks | ||
| if (main.getBlocks().size() > 1) { | ||
| return; | ||
| } | ||
|
|
||
| // Get the existing block | ||
| auto* bodyBlock = &main.front(); | ||
| OpBuilder builder(main.getBody()); | ||
|
|
||
| // Create the required blocks | ||
| auto* entryBlock = builder.createBlock(&main.getBody()); | ||
| // Move the entry block before the body block | ||
| main.getBlocks().splice(Region::iterator(bodyBlock), main.getBlocks(), | ||
| entryBlock); | ||
| Block* measurementsBlock = builder.createBlock(&main.getBody()); | ||
| Block* outputBlock = builder.createBlock(&main.getBody()); | ||
|
|
||
| auto& bodyBlockOps = bodyBlock->getOperations(); | ||
| auto& outputBlockOps = outputBlock->getOperations(); | ||
| auto& measurementsBlockOps = measurementsBlock->getOperations(); | ||
|
|
||
| // Move operations to appropriate blocks | ||
| for (auto it = bodyBlock->begin(); it != bodyBlock->end();) { | ||
| // Ensure iterator remains valid after potential move | ||
| if (auto& op = *it++; | ||
| isa<DeallocOp>(op) || isa<ResetOp>(op) || isa<MeasureOp>(op)) { | ||
| // Move irreversible quantum operations to measurements block | ||
| measurementsBlockOps.splice(measurementsBlock->end(), bodyBlockOps, | ||
| Block::iterator(op)); | ||
| } else if (isa<LLVM::ReturnOp>(op)) { | ||
| // Move return to output block | ||
| outputBlockOps.splice(outputBlock->end(), bodyBlockOps, | ||
| Block::iterator(op)); | ||
| } else if (op.hasTrait<OpTrait::ConstantLike>()) { | ||
| // Move constant like operations to the entry block | ||
| entryBlock->getOperations().splice(entryBlock->end(), bodyBlockOps, | ||
| Block::iterator(op)); | ||
| } | ||
| // All other operations (gates, etc.) stay in body block | ||
| } | ||
|
|
||
| // Add unconditional jumps between blocks | ||
| builder.setInsertionPointToEnd(entryBlock); | ||
| builder.create<LLVM::BrOp>(main->getLoc(), bodyBlock); | ||
|
|
||
| builder.setInsertionPointToEnd(bodyBlock); | ||
| builder.create<LLVM::BrOp>(main->getLoc(), measurementsBlock); | ||
|
|
||
| builder.setInsertionPointToEnd(measurementsBlock); | ||
| builder.create<LLVM::BrOp>(main->getLoc(), outputBlock); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify assumptions about block layout for addOutputRecording
ensureBlocks only restructures functions that start with a single block, but addOutputRecording assumes the last block of main is the output block. If this pass is ever run on a module where main already has multiple blocks with a different layout, output recording could be inserted into an unexpected block.
If the pass is intended to run only on single-block mains produced by your pipeline, consider documenting that precondition on the pass, or enforcing/normalizing the 4-block layout unconditionally before calling addOutputRecording.
Also applies to: 1041-1056
There was a problem hiding this comment.
@denialhaag, I think we should follow the advice by @coderabbitai here and at least document the pre-condition. Hopefully, this is something that we can lift rather soon when we improve our QIR support
There was a problem hiding this comment.
@burgholzer Good call on documenting this pre-condition! Here are some suggestions for where to add this:
-
In the pass description comment (line ~900 in
QuartzToQIR.cpp):/** * @brief Pass for converting Quartz dialect operations to QIR * * @details * This pass converts Quartz dialect quantum operations to QIR (Quantum * Intermediate Representation) by lowering them to LLVM dialect operations * that call QIR runtime functions. * * @pre The entry function must have a single block. The pass will * restructure it into a 4-block layout (entry → body → measurements * → output). Multi-block functions are currently unsupported. * ... */
-
In the TableGen pass definition (
QuartzToQIR.td):
Add a note in the description about the expected input structure. -
In
ensureBlocksandaddOutputRecordingfunction docs:
Explicitly state the assumptions about block structure.
This will help future maintainers understand the constraint when enhancing QIR support. Would you like me to generate a specific documentation snippet you can use?
🧠 Learnings used
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp:96-117
Timestamp: 2025-12-08T23:41:55.972Z
Learning: In the QIR (Quantum Intermediate Representation) Builder (mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp), the `ptrCache` is intentionally shared between qubit and result pointer creation (in `staticQubit()` and `measure()` methods) because QIR uses opaque pointers and `inttoptr` conversions for both qubits and results. For any given index N, the LLVM IR pointer representation is identical whether it represents a qubit or a result, so the pointer only needs to be created once and can be safely reused across both contexts.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Operations/StandardGates/BarrierOp.cpp:45-54
Timestamp: 2025-12-09T00:55:11.926Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux), qubits follow single-user/linear-type semantics where each qubit value can only be consumed once, similar to MQTOpt. This invariant makes it safe to dereference getUsers().begin() in canonicalization patterns like MergeSubsequentBarrier in BarrierOp.cpp, as there will be at most one user per qubit output.
| OwningOpRef<ModuleOp> FluxProgramBuilder::finalize() { | ||
| // Automatically deallocate all remaining valid qubits | ||
| for (const auto qubit : validQubits) { | ||
| create<DeallocOp>(loc, qubit); | ||
| } | ||
|
|
||
| validQubits.clear(); | ||
|
|
||
| // Create constant 0 for successful exit code | ||
| auto exitCode = create<arith::ConstantOp>(loc, getI64IntegerAttr(0)); | ||
|
|
||
| // Add return statement with exit code 0 to the main function | ||
| create<func::ReturnOp>(loc, ValueRange{exitCode}); | ||
|
|
||
| return module; | ||
| } |
There was a problem hiding this comment.
Non-deterministic deallocation order from DenseSet iteration.
validQubits is a DenseSet, and iterating over it produces a non-deterministic order. This means the generated DeallocOp sequence in the finalized IR can vary between runs, potentially affecting:
- IR diff comparisons
- Reproducible test outputs
- Debug sessions
Consider using a deterministic container (e.g., SetVector) or sorting qubits by some stable key before deallocation:
+ // Sort qubits for deterministic deallocation order
+ SmallVector<Value> sortedQubits(validQubits.begin(), validQubits.end());
+ llvm::sort(sortedQubits, [](Value a, Value b) {
+ // Sort by operation order in the IR for reproducibility
+ return a.getDefiningOp()->isBeforeInBlock(b.getDefiningOp());
+ });
+
// Automatically deallocate all remaining valid qubits
- for (const auto qubit : validQubits) {
+ for (const auto qubit : sortedQubits) {
create<DeallocOp>(loc, qubit);
}Alternatively, if order doesn't matter semantically, document that deallocation order is undefined.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| OwningOpRef<ModuleOp> FluxProgramBuilder::finalize() { | |
| // Automatically deallocate all remaining valid qubits | |
| for (const auto qubit : validQubits) { | |
| create<DeallocOp>(loc, qubit); | |
| } | |
| validQubits.clear(); | |
| // Create constant 0 for successful exit code | |
| auto exitCode = create<arith::ConstantOp>(loc, getI64IntegerAttr(0)); | |
| // Add return statement with exit code 0 to the main function | |
| create<func::ReturnOp>(loc, ValueRange{exitCode}); | |
| return module; | |
| } | |
| OwningOpRef<ModuleOp> FluxProgramBuilder::finalize() { | |
| // Sort qubits for deterministic deallocation order | |
| SmallVector<Value> sortedQubits(validQubits.begin(), validQubits.end()); | |
| llvm::sort(sortedQubits, [](Value a, Value b) { | |
| // Sort by operation order in the IR for reproducibility | |
| return a.getDefiningOp()->isBeforeInBlock(b.getDefiningOp()); | |
| }); | |
| // Automatically deallocate all remaining valid qubits | |
| for (const auto qubit : sortedQubits) { | |
| create<DeallocOp>(loc, qubit); | |
| } | |
| validQubits.clear(); | |
| // Create constant 0 for successful exit code | |
| auto exitCode = create<arith::ConstantOp>(loc, getI64IntegerAttr(0)); | |
| // Add return statement with exit code 0 to the main function | |
| create<func::ReturnOp>(loc, ValueRange{exitCode}); | |
| return module; | |
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the specific review comment thread from the PR
gh api repos/munich-quantum-toolkit/core/pulls/1264/comments \
--jq '.[] | select(.id == 2599709667) | {id, body, user: .user.login, path, line, created_at}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Try fetching the comment directly using the review comments API
gh api repos/munich-quantum-toolkit/core/pulls/1264/comments/2599709667 \
--jq '{id, body, user: .user.login, path, line, created_at}'
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Also try to get recent comments on this file to understand the context
gh api repos/munich-quantum-toolkit/core/pulls/1264/comments \
--jq '.[] | select(.path == "mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp") | {id, body, user: .user.login, line, created_at}' | head -20
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for comments mentioning "deterministic" or "DenseSet" in the PR
gh api graphql -f query='
{
repository(owner: "munich-quantum-toolkit", name: "core") {
pullRequest(number: 1264) {
reviews(last: 50) {
nodes {
comments(last: 100) {
nodes {
id
body
path
line
author {
login
}
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviews.nodes[].comments.nodes[] | select(.body | test("deterministic|DenseSet"; "i")) | {id, author: .author.login, path, line, body_preview: .body[0:200]}'
Length of output: 706
🏁 Script executed:
#!/bin/bash
# Try to get all comments and search for the specific ID pattern
gh pr view 1264 --repo munich-quantum-toolkit/core --json comments \
--jq '.comments[] | select(.body | contains("2599709667") or contains("deterministic") or contains("DenseSet")) | {author: .author.login, body}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get the full comment from QuartzProgramBuilder.cpp to understand the related discussion
gh api graphql -f query='
{
repository(owner: "munich-quantum-toolkit", name: "core") {
pullRequest(number: 1264) {
reviews(last: 50) {
nodes {
comments(last: 100) {
nodes {
id
databaseId
body
path
line
author {
login
}
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviews.nodes[].comments.nodes[] | select(.path == "mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp" and (.body | test("deterministic|DenseSet"; "i"))) | {id, databaseId, author: .author.login, path, line, body}'
Length of output: 2003
@denialhaag Thank you for the reference! You're absolutely right - this is the exact same pattern in both builders:
- QuartzProgramBuilder iterates over
allocatedQubits(DenseSet) - FluxProgramBuilder iterates over
validQubits(DenseSet)
Both exhibit non-deterministic deallocation order in their finalize() methods.
Since this was noted as a nitpick in the Quartz builder case, would you prefer to:
- Keep as-is - Accept that deallocation order is undefined (since qubits are independent, it's semantically correct)
- Apply consistently - If you'd like deterministic IR output for testing/debugging, the same sorting approach could be applied to both builders
- Document the behavior - Add a comment noting that deallocation order is unspecified
The functional correctness isn't affected either way, so it's primarily a question of whether deterministic IR output is valuable for your use cases. Let me know your preference! 😊
🧠 Learnings used
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.880Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-101
Timestamp: 2025-12-08T23:16:16.276Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The CtrlInlineGPhase canonicalization pattern correctly only checks getNumPosControls() and processes only positive controls when inlining a GPhaseOp.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:219-221
Timestamp: 2025-10-09T13:14:10.178Z
Learning: The MQT Core project (munich-quantum-toolkit/core repository) uses the C++20 standard, not C++17. C++20 features such as abbreviated function templates (e.g., `const auto&` parameters) are supported and valid in this codebase.
Learnt from: flowerthrower
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 1
File: CHANGELOG.md:18-18
Timestamp: 2025-12-01T11:00:40.342Z
Learning: In the munich-quantum-toolkit/core-plugins-catalyst repository, the CHANGELOG.md intentionally references the parent MQT Core repository's release notes (https://github.com/munich-quantum-toolkit/core/releases) because the plugin repository is based on work performed in the parent repository.
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
This reverts commit dcb2b08.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
mlir/tools/mqt-cc/mqt-cc.cpp (1)
40-63: Theconstqualifier oncl::optdeclarations conflicts with LLVM's CommandLine library requirements.All six command-line option variables are declared
const, but LLVM'sParseCommandLineOptions(line 105) requires mutable storage to write parsed values. This was flagged in a previous review and marked as addressed, but theconstqualifier is still present in the current code.Remove
constfrom all option declarations:-const cl::opt<std::string> INPUT_FILENAME(cl::Positional, +cl::opt<std::string> INPUT_FILENAME(cl::Positional, cl::desc("<input .mlir file>"), cl::init("-")); -const cl::opt<std::string> OUTPUT_FILENAME("o", cl::desc("Output filename"), +cl::opt<std::string> OUTPUT_FILENAME("o", cl::desc("Output filename"), cl::value_desc("filename"), cl::init("-")); -const cl::opt<bool> CONVERT_TO_QIR("emit-qir", +cl::opt<bool> CONVERT_TO_QIR("emit-qir", cl::desc("Convert to QIR at the end"), cl::init(false)); -const cl::opt<bool> ENABLE_TIMING("mlir-timing", +cl::opt<bool> ENABLE_TIMING("mlir-timing", cl::desc("Enable pass timing statistics"), cl::init(false)); -const cl::opt<bool> ENABLE_STATISTICS("mlir-statistics", +cl::opt<bool> ENABLE_STATISTICS("mlir-statistics", cl::desc("Enable pass statistics"), cl::init(false)); -const cl::opt<bool> +cl::opt<bool> PRINT_IR_AFTER_ALL_STAGES("mlir-print-ir-after-all-stages", cl::desc("Print IR after each compiler stage"), cl::init(false));mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (1)
120-161: Guard against duplicate globals when creating result labels.
createResultLabelalways creates a newLLVM::GlobalOpfor the samesymbolPrefix + "_" + labelwithout checking if a global with that name already exists. If two call sites request the same label (e.g., identical register/label names from different builders or passes), this will produce duplicate global symbols and can cause LLVM IR verification failures.Consider reusing an existing global when present:
- const auto globalOp = builder.create<LLVM::GlobalOp>( - op->getLoc(), llvmArrayType, /*isConstant=*/true, LLVM::Linkage::Internal, - symbolName, stringInitializer); - globalOp->setAttr("addr_space", builder.getI32IntegerAttr(0)); - globalOp->setAttr("dso_local", builder.getUnitAttr()); + if (auto existingGlobal = + module.lookupSymbol<LLVM::GlobalOp>(symbolName)) { + // Reuse existing global + (void)existingGlobal; + } else { + const auto globalOp = builder.create<LLVM::GlobalOp>( + op->getLoc(), llvmArrayType, /*isConstant=*/true, + LLVM::Linkage::Internal, symbolName, stringInitializer); + globalOp->setAttr("addr_space", builder.getI32IntegerAttr(0)); + globalOp->setAttr("dso_local", builder.getUnitAttr()); + }The subsequent
AddressOfOpcan then always be created as it is now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
mlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h(1 hunks)mlir/include/mlir/Dialect/Utils/Utils.h(1 hunks)mlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp(1 hunks)mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp(1 hunks)mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp(1 hunks)mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp(1 hunks)mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp(1 hunks)mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp(1 hunks)mlir/tools/mqt-cc/mqt-cc.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
📚 Learning: 2025-10-09T13:14:10.178Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:219-221
Timestamp: 2025-10-09T13:14:10.178Z
Learning: The MQT Core project (munich-quantum-toolkit/core repository) uses the C++20 standard, not C++17. C++20 features such as abbreviated function templates (e.g., `const auto&` parameters) are supported and valid in this codebase.
Applied to files:
mlir/tools/mqt-cc/mqt-cc.cppmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp
📚 Learning: 2025-12-05T17:45:37.602Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
Applied to files:
mlir/tools/mqt-cc/mqt-cc.cppmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-12-08T12:44:05.874Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:60-70
Timestamp: 2025-12-08T12:44:05.874Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The RemoveTrivialCtrl pattern correctly only checks getNumPosControls() when determining if a CtrlOp should be removed.
Applied to files:
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-12-08T14:55:43.880Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.880Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Applied to files:
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp
📚 Learning: 2025-12-08T23:16:16.276Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-101
Timestamp: 2025-12-08T23:16:16.276Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), negative controls are not supported at the current stage. The CtrlInlineGPhase canonicalization pattern correctly only checks getNumPosControls() and processes only positive controls when inlining a GPhaseOp.
Applied to files:
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-10-09T13:13:51.224Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Applied to files:
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-10-09T13:20:11.483Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Applied to files:
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-12-02T07:37:46.860Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Applied to files:
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cppmlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp
📚 Learning: 2025-11-27T08:52:06.428Z
Learnt from: lsschmid
Repo: munich-quantum-toolkit/qmap PR: 832
File: src/hybridmap/HybridNeutralAtomMapper.cpp:1379-1383
Timestamp: 2025-11-27T08:52:06.428Z
Learning: In src/hybridmap/HybridNeutralAtomMapper.cpp, the getBestMovePos function throws std::runtime_error when no move position is found (finalBestPos.coords is empty). This behavior is acceptable for current use cases since tight architectures are not a concern. A future improvement would be to fall back to SWAP-based strategies instead of throwing.
Applied to files:
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp
📚 Learning: 2025-10-09T13:28:29.237Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/DeadGateEliminationPattern.cpp:42-45
Timestamp: 2025-10-09T13:28:29.237Z
Learning: In the MQTOpt MLIR dialect, linear types enforce single-use semantics where each qubit value can only be consumed once, preventing duplicate deallocations and making RAUW operations safe when replacing output qubits with corresponding input qubits in transformation patterns.
Applied to files:
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp
📚 Learning: 2025-12-04T06:59:40.314Z
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:84-85
Timestamp: 2025-12-04T06:59:40.314Z
Learning: In the MQTOpt MLIR routing passes (NaiveRoutingPassSC, AStarRoutingPassSC), the input IR is guaranteed to contain only 1-qubit and 2-qubit gates. All 3+-qubit gates must be decomposed before routing; otherwise the input IR is invalid. This invariant allows skipTwoQubitBlock in LayeredUnit.cpp to safely assert wires.size() == 2.
Applied to files:
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cppmlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp
📚 Learning: 2025-10-09T13:25:36.887Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReuseQubitsPattern.cpp:98-100
Timestamp: 2025-10-09T13:25:36.887Z
Learning: In MLIR code, when traversing parent operations to find a common block between two operations where one uses the result of another, explicit bounds checking is not necessary. MLIR's SSA properties and scope locality guarantees ensure that operations using results must be in compatible scopes and will always share a common ancestor in the operation hierarchy.
Applied to files:
mlir/lib/Dialect/QIR/Utils/QIRUtils.cppmlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
📚 Learning: 2025-12-01T11:00:40.342Z
Learnt from: flowerthrower
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 1
File: CHANGELOG.md:18-18
Timestamp: 2025-12-01T11:00:40.342Z
Learning: In the munich-quantum-toolkit/core-plugins-catalyst repository, the CHANGELOG.md intentionally references the parent MQT Core repository's release notes (https://github.com/munich-quantum-toolkit/core/releases) because the plugin repository is based on work performed in the parent repository.
Applied to files:
mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp
📚 Learning: 2025-11-04T14:28:32.371Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/test_qdmi_qiskit_backend.py:0-0
Timestamp: 2025-11-04T14:28:32.371Z
Learning: In the munich-quantum-toolkit/core repository, at least one FoMaC device is always available during testing, so skip logic for missing devices in QDMI Qiskit backend tests is not necessary.
Applied to files:
mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp
📚 Learning: 2025-12-07T09:10:31.820Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1355
File: src/qdmi/sc/Device.cpp:97-102
Timestamp: 2025-12-07T09:10:31.820Z
Learning: In the munich-quantum-toolkit/core repository, duplication of QDMI-related macros (such as IS_INVALID_ARGUMENT) across device implementations (e.g., in src/qdmi/sc/Device.cpp and src/qdmi/dd/Device.cpp) is acceptable as a temporary measure. The preferred long-term solution is to upstream these macros to the QDMI repository rather than creating local shared headers, so they can be reused across all dependent projects.
Applied to files:
mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cppmlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h
🧬 Code graph analysis (8)
mlir/tools/mqt-cc/mqt-cc.cpp (1)
mlir/include/mlir/Compiler/CompilerPipeline.h (2)
mlir(17-145)config(90-90)
mlir/include/mlir/Dialect/Utils/Utils.h (1)
mlir/include/mlir/Dialect/Flux/FluxUtils.h (1)
mlir(17-205)
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp (2)
mlir/include/mlir/Dialect/Quartz/IR/QuartzDialect.h (11)
mlir(52-119)getNumTargets(70-70)getNumControls(71-71)getNumPosControls(72-72)getNumNegControls(73-73)getQubit(75-83)getTarget(84-92)getPosControl(94-96)getNegControl(98-100)getNumParams(102-102)getParameter(104-109)mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp (26)
op(37-55)op(37-38)op(64-77)op(64-65)op(86-105)op(86-87)op(114-129)op(114-115)getNumQubits(138-138)getNumQubits(138-138)getNumTargets(140-140)getNumTargets(140-140)getNumControls(142-144)getNumControls(142-142)getNumPosControls(146-146)getNumPosControls(146-146)getNumNegControls(148-150)getNumNegControls(148-148)getNumParams(238-238)getNumParams(238-238)getParameter(240-242)getParameter(240-240)build(244-255)build(244-246)build(257-269)build(257-260)
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp (1)
mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (10)
getOrCreateFunctionDeclaration(88-118)getOrCreateFunctionDeclaration(88-90)createPointerFromIndex(164-172)createPointerFromIndex(164-165)createResultLabel(120-162)createResultLabel(120-122)getMainFunction(31-54)getMainFunction(31-31)setQIRAttributes(56-86)setQIRAttributes(56-56)
mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (1)
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp (22)
op(229-287)op(230-231)op(310-315)op(311-312)op(339-362)op(340-341)op(393-454)op(394-395)op(476-491)op(477-478)op(511-520)op(512-513)op(820-825)op(821-822)op(834-849)op(835-836)main(907-958)main(907-907)main(972-1001)main(972-972)main(1039-1111)main(1039-1040)
mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp (2)
mlir/include/mlir/Dialect/Utils/Utils.h (1)
mlir(19-43)mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp (14)
op(36-53)op(36-37)op(61-74)op(61-62)op(83-99)op(83-84)getBodyUnitary(104-106)getBodyUnitary(104-104)getNumPosControls(116-116)getNumPosControls(116-116)build(154-165)build(154-155)build(167-178)build(167-169)
mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp (3)
mlir/include/mlir/Dialect/QIR/Builder/QIRProgramBuilder.h (3)
QIRProgramBuilder(63-136)index(141-157)registerIndex(135-135)mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (6)
getOrCreateFunctionDeclaration(88-118)getOrCreateFunctionDeclaration(88-90)createPointerFromIndex(164-172)createPointerFromIndex(164-165)createResultLabel(120-162)createResultLabel(120-122)mlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h (1)
numResults(142-142)
mlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h (1)
mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (10)
getMainFunction(31-54)getMainFunction(31-31)setQIRAttributes(56-86)setQIRAttributes(56-56)getOrCreateFunctionDeclaration(88-118)getOrCreateFunctionDeclaration(88-90)createResultLabel(120-162)createResultLabel(120-122)createPointerFromIndex(164-172)createPointerFromIndex(164-165)
🪛 ast-grep (0.40.0)
mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp
[warning] 177-177: 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] 184-184: 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] 216-216: 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] 217-217: 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] 230-230: 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] 231-231: 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)
🪛 Cppcheck (2.18.0)
mlir/tools/mqt-cc/mqt-cc.cpp
[information] 11-11: Include file
(missingInclude)
[information] 12-12: Include file
(missingInclude)
[information] 13-13: Include file
(missingInclude)
[information] 15-15: Include file
(missingIncludeSystem)
[information] 16-16: Include file
(missingIncludeSystem)
[information] 17-17: Include file
(missingIncludeSystem)
[information] 18-18: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp
[information] 11-11: Include file
(missingInclude)
[information] 13-13: Include file
(missingIncludeSystem)
[information] 14-14: Include file
(missingIncludeSystem)
[information] 15-15: Include file
(missingIncludeSystem)
[information] 16-16: Include file
(missingIncludeSystem)
[information] 17-17: Include file
(missingIncludeSystem)
[information] 18-18: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp
[information] 11-11: Include file
(missingInclude)
[information] 13-13: Include file
(missingInclude)
[information] 14-14: Include file
(missingInclude)
[information] 16-16: Include file
(missingIncludeSystem)
[information] 17-17: Include file
(missingIncludeSystem)
[information] 18-18: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 34-34: Include file
(missingIncludeSystem)
[information] 35-35: Include file
(missingIncludeSystem)
[information] 36-36: Include file
(missingIncludeSystem)
[information] 37-37: Include file
(missingIncludeSystem)
[information] 38-38: Include file
(missingIncludeSystem)
[information] 39-39: Include file
(missingIncludeSystem)
[information] 40-40: Include file
(missingIncludeSystem)
[information] 41-41: Include file
(missingIncludeSystem)
[information] 42-42: Include file
(missingIncludeSystem)
[information] 43-43: Include file
(missingIncludeSystem)
[information] 44-44: Include file
(missingIncludeSystem)
[information] 45-45: Include file
(missingIncludeSystem)
[information] 46-46: Include file
(missingIncludeSystem)
[information] 47-47: Include file
(missingIncludeSystem)
[information] 48-48: Include file
(missingIncludeSystem)
[information] 49-49: Include file
(missingIncludeSystem)
[information] 50-50: Include file
(missingIncludeSystem)
[information] 58-58: Include file
(missingInclude)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp
[information] 11-11: Include file
(missingInclude)
[information] 13-13: Include file
(missingIncludeSystem)
[information] 14-14: Include file
(missingIncludeSystem)
[information] 15-15: Include file
(missingIncludeSystem)
[information] 16-16: Include file
(missingIncludeSystem)
[information] 17-17: Include file
(missingIncludeSystem)
[information] 18-18: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[style] 56-56: The function 'setQIRAttributes' is never used.
(unusedFunction)
[style] 88-88: The function 'getOrCreateFunctionDeclaration' is never used.
(unusedFunction)
[style] 120-120: The function 'createResultLabel' is never used.
(unusedFunction)
[style] 164-164: The function 'createPointerFromIndex' is never used.
(unusedFunction)
mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp
[information] 11-11: Include file
(missingInclude)
[information] 13-13: Include file
(missingIncludeSystem)
[information] 14-14: Include file
(missingIncludeSystem)
[information] 15-15: Include file
(missingIncludeSystem)
[information] 16-16: Include file
(missingIncludeSystem)
[information] 17-17: Include file
(missingIncludeSystem)
[information] 18-18: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp
[information] 11-11: Include file
(missingInclude)
[information] 13-13: Include file
(missingInclude)
[information] 15-15: Include file
(missingIncludeSystem)
[information] 16-16: Include file
(missingIncludeSystem)
[information] 17-17: Include file
(missingIncludeSystem)
[information] 18-18: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
mlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp
[information] 11-11: Include file
(missingInclude)
[information] 13-13: Include file
(missingInclude)
[information] 14-14: Include file
(missingInclude)
[information] 16-16: Include file
(missingIncludeSystem)
[information] 17-17: Include file
(missingIncludeSystem)
[information] 18-18: Include file
(missingIncludeSystem)
[information] 19-19: Include file
(missingIncludeSystem)
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 30-30: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 38-38: Include file
(missingInclude)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (23)
mlir/include/mlir/Dialect/Utils/Utils.h (2)
11-21: LGTM: Includes and TOLERANCE constant are correct.The missing includes flagged in previous reviews have been properly addressed. The TOLERANCE value of 1e-12 is appropriate for floating-point near-zero checks in quantum gate optimization patterns.
23-41: LGTM: variantToValue implementation is correct and well-documented.The function correctly handles both alternatives of the variant parameter by creating an arith::ConstantOp for doubles or forwarding precomputed Values. This provides a clean abstraction for gate builders that accept parameters as either literals or SSA Values.
mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp (7)
34-56: LGTM: MergeNestedCtrl correctly merges control qubits.The pattern properly combines controls from both the outer and inner CtrlOps. The previous issue with explicit
eraseOp(bodyCtrlOp)has been resolved—replaceOpWithNewOpnow handles cleanup automatically.
61-78: LGTM: RemoveTrivialCtrl correctly unwraps control-less operations.The pattern properly clones the body operation before replacing the CtrlOp, avoiding the use-after-free issue flagged in earlier reviews. The check for only positive controls is correct given that negative controls are not yet supported. Based on learnings.
83-106: LGTM: CtrlInlineGPhase correctly inlines controlled global phase.The pattern correctly handles the GPhaseOp case by creating POps for each positive control. Since GPhaseOp is a zero-target operation (global phase), the CtrlOp wrapping it only has control qubits and no targets, so the replacement correctly produces outputs only for the positive controls. Based on learnings.
111-130: LGTM: CtrlInlineId correctly unwraps controlled identity.The pattern properly creates a standalone IdOp and includes both all controls and the target output in the replacement operands. The reservation size correctly accounts for all positive controls plus the single target.
134-208: LGTM: Accessor implementations are correct and consistent.All accessor methods now include proper bounds checking with descriptive error messages via
reportFatalUsageError. The delegation of negative control accessors to the body unitary is appropriate. The target accessor bounds checking issue flagged in previous reviews has been resolved.
210-269: LGTM: Mapping accessors and builders are correctly implemented.The input/output mapping methods properly iterate through all controls and targets. Both build overloads correctly construct the body region with a unitary operation and YieldOp terminator, using InsertionGuard to avoid side effects on the caller's insertion point.
271-327: LGTM: Verification and canonicalization registration are comprehensive.The verify method thoroughly validates the CtrlOp structure:
- Body region has exactly 2 operations (unitary + yield terminator)
- No duplicate qubits in inputs or outputs
- Semantic constraint that BarrierOp cannot be controlled
The canonicalization pattern registration correctly includes all four patterns defined in this file.
mlir/lib/Conversion/QuartzToFlux/QuartzToFlux.cpp (5)
1-39: Well-structured file with clean organization.The copyright headers, includes, and namespace setup follow project conventions. The include ordering (project headers first, then LLVM/MLIR system headers) is correct.
Note: The static analysis hints about missing includes are false positives—the headers exist but aren't visible to the standalone analyzer.
40-108: Clean state management design and base pattern class.The
LoweringStatestruct correctly captures the essential state for tracking the reference-to-value semantic conversion:
qubitMapfor top-level qubit trackinginCtrlOpcounter withtargetsIn/targetsOutmaps for nested control operationsThe
StatefulOpConversionPatterntemplate properly addresses the earlier review feedback—the inherited base constructors (usingstatement) were removed, ensuring all instances are constructed with a validstate_pointer.
1168-1233: Pass implementation is comprehensive and follows MLIR best practices.The pass correctly:
- Creates
LoweringStateon the stack (safe sinceapplyPartialConversionruns synchronously)- Configures the conversion target with Quartz illegal and Flux legal
- Registers all conversion patterns with the shared state
- Handles function signatures, returns, calls, and branch operations
- Signals pass failure appropriately on conversion errors
719-992: Macro-generated patterns are clean and consistent.The use of macros to generate boilerplate conversion patterns is well-executed:
- Each macro properly delegates to the corresponding template helper
- Macros are
#undef'd immediately after use to prevent pollution- Documentation in macro comments correctly describes the transformations
This significantly reduces code duplication while maintaining readability.
1009-1039: ConvertQuartzBarrierOp pattern is correct; quartz.barrier cannot appear inside quartz.ctrl by design.BarrierOp explicitly rejects control operands with fatal errors in
getPosControl()andgetNegControl()methods, and the operation definition has no region support. Unlike controllable gates, barriers are fundamentally non-controllable operations and cannot legally appear within aquartz.ctrlregion. The pattern correctly ignoresstate.inCtrlOpbecause that context is impossible for this operation.Likely an incorrect or invalid review comment.
mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp (6)
34-54: LGTM! Past use-after-free issue resolved.The
MergeNestedCtrlpattern correctly merges nested controls by callingreplaceOpWithNewOpwithout an expliciteraseOp, which resolves the previously flagged use-after-free issue.
59-75: LGTM! Correctly handles trivial control removal.The pattern appropriately checks only
getNumPosControls()since negative controls are not supported in the Quartz dialect at this stage. When no positive controls exist, the body operation is correctly cloned to replace the CtrlOp.Based on learnings, negative controls are not supported in Quartz.
104-120: LGTM! Accessor methods correctly implemented.The accessor methods properly compute qubit counts and delegate to the body unitary where appropriate. The delegation pattern for
getNumNegControls()exposes negative controls from the nested body operation, which is consistent with the overall design even though negative controls are not directly supported at the CtrlOp level.
122-152: LGTM! Qubit and parameter accessors correctly implemented.The accessor methods properly handle indexing with appropriate bounds checking. The
getQubit()method correctly maps indices to positive controls first, then delegates to body qubits. All delegation patterns to the body unitary are appropriate.
154-178: LGTM! Builder methods follow MLIR best practices.Both
build()overloads correctly construct the operation with proper insertion guards, region setup, body population (via cloning or callback), and terminator creation.
180-218: LGTM! Verification and pattern registration correctly implemented.The
verify()method comprehensively validates the operation structure (body region layout, qubit uniqueness, and semantic constraints like forbidding controlled barriers). ThegetCanonicalizationPatterns()method appropriately registers all three canonicalization patterns.mlir/lib/Dialect/QIR/Utils/QIRUtils.cpp (1)
56-86: QIR metadata attribute assembly looks consistent and complete.The passthrough payload correctly encodes entry_point, labeled output schema, base_profile, resource counts, and QIR 2.0 version plus dynamic management flags; this matches the documented QIRMetadata contract and what QuartzToQIR/QIRProgramBuilder expect.
mlir/lib/Conversion/QuartzToQIR/QuartzToQIR.cpp (1)
130-182: Centralized unitary lowering withconvertUnitaryToCallOplooks solid.Using
LoweringStatefor control tracking and building the QIR call signature from(controls, targets, params)keeps all gate conversions consistent, and the ctrl cleanup here is safe given the CtrlOp verifier guarantees a single controlled unitary in the region.mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp (1)
42-94: Main function and block layout initialization aligns with QIR Base Profile expectations.The 4-block setup (entry/body/measurements/output), early
__quantum__rt__initializecall, and unconditional branches plus finalreturnform a clean base on which later builders and output-recording logic operate; the use of insertion guards keeps user code insertion points predictable.
| * For QIR functions that are irreversible (measurement, reset, deallocation), | ||
| * the "irreversible" attribute is added automatically. | ||
| * | ||
| * @param builder The builder to use for creating operations | ||
| * @param op The operation requesting the function (for context) | ||
| * @param fnName The name of the QIR function | ||
| * @param fnType The LLVM function type signature | ||
| * @return The LLVM function declaration | ||
| */ | ||
| LLVM::LLVMFuncOp getOrCreateFunctionDeclaration(OpBuilder& builder, | ||
| Operation* op, StringRef fnName, | ||
| Type fnType); | ||
|
|
There was a problem hiding this comment.
Align header documentation with the current implementation.
Two docstrings are slightly out of sync with QIRUtils.cpp:
getOrCreateFunctionDeclaration: the comment says irreversible “measurement, reset, deallocation” are annotated, but the implementation currently only special-casesQIR_MEASUREandQIR_RESET. Either extend the implementation or drop “deallocation” from the doc.createResultLabel: the comment claims theAddressOfOpis inserted at the builder’s current insertion point, but the implementation always inserts it at the start of the main function’s entry block (restoring the caller’s insertion viaInsertionGuard). Updating the wording to describe “module-level global + address-of in main’s entry block” would avoid confusion.
🤖 Prompt for AI Agents
In mlir/include/mlir/Dialect/QIR/Utils/QIRUtils.h around lines 191 to 203, the
header comments are out of sync with the implementation: for
getOrCreateFunctionDeclaration remove the claim that "deallocation" is annotated
(implementation only special-cases QIR_MEASURE and QIR_RESET) or update
implementation to also handle deallocation — pick one and make the doc and code
consistent; for createResultLabel update the wording to state that the global is
created at module level and the AddressOfOp is inserted at the start of main's
entry block (the builder's insertion point is temporarily restored via an
insertion guard), rather than claiming the AddressOfOp is inserted at the
builder's current insertion point. Ensure the docstrings explicitly reflect the
actual behavior and any restored insertion point semantics.
There was a problem hiding this comment.
@denialhaag the @coderabbitai correctly points out that the docstring here is outdated.
There was a problem hiding this comment.
@burgholzer @denialhaag You're absolutely right—the docstrings are out of sync with the implementation. Here's what needs to be fixed:
1. getOrCreateFunctionDeclaration (around line 191-203)
Current doc says: marks "measurement, reset, deallocation" as irreversible
Actual behavior: only marks QIR_MEASURE and QIR_RESET
Suggested fix: Either:
- Update the doc to say "measurement and reset" (simpler), or
- Extend the implementation to also handle deallocation functions if that's intended behavior
2. createResultLabel (docstring not fully shown in diff)
Current doc says: inserts AddressOfOp at builder's current insertion point
Actual behavior: creates a module-level global string constant and inserts the AddressOfOp at the start of main's entry block (using InsertionGuard to restore the original position)
Suggested fix: Update the docstring to say:
/**
* @brief Create a module-level string label and return an AddressOfOp to it
*
* @details
* Creates a global string constant at the module level (with addr_space and
* dso_local attributes) and inserts an AddressOfOp at the start of the main
* function's entry block. The builder's original insertion point is preserved
* via an InsertionGuard.
* ...
*/These changes will bring the documentation in line with the actual implementation.
🧠 Learnings used
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp:96-117
Timestamp: 2025-12-08T23:41:55.972Z
Learning: In the QIR (Quantum Intermediate Representation) Builder (mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp), the `ptrCache` is intentionally shared between qubit and result pointer creation (in `staticQubit()` and `measure()` methods) because QIR uses opaque pointers and `inttoptr` conversions for both qubits and results. For any given index N, the LLVM IR pointer representation is identical whether it represents a qubit or a result, so the pointer only needs to be created once and can be safely reused across both contexts.
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Operations/StandardGates/PhaseOp.cpp:0-0
Timestamp: 2025-12-08T23:44:44.157Z
Learning: In MLIR code (mlir/ directories), avoid using const qualifiers on core MLIR data types like Value, Type, Attribute, Operation*, Block*, Region*, etc. in function parameters and signatures. MLIR's design rationale explicitly discourages const for these types. See https://mlir.llvm.org/docs/Rationale/UsageOfConst/ for details.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Operations/StandardGates/BarrierOp.cpp:45-54
Timestamp: 2025-12-09T00:55:11.926Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux), qubits follow single-user/linear-type semantics where each qubit value can only be consumed once, similar to MQTOpt. This invariant makes it safe to dereference getUsers().begin() in canonicalization patterns like MergeSubsequentBarrier in BarrierOp.cpp, as there will be at most one user per qubit output.
| Value QIRProgramBuilder::measure(const Value qubit, const int64_t resultIndex) { | ||
| if (resultIndex < 0) { | ||
| llvm::reportFatalUsageError("Result index must be non-negative"); | ||
| } | ||
|
|
||
| // Save current insertion point | ||
| const OpBuilder::InsertionGuard guard(builder); | ||
|
|
||
| // Insert in measurements block (before branch) | ||
| builder.setInsertionPoint(measurementsBlock->getTerminator()); | ||
|
|
||
| const auto key = std::make_pair("c", resultIndex); | ||
| if (const auto it = registerResultMap.find(key); | ||
| it != registerResultMap.end()) { | ||
| return it->second; | ||
| } | ||
|
|
||
| Value resultValue{}; | ||
| if (const auto it = ptrCache.find(resultIndex); it != ptrCache.end()) { | ||
| resultValue = it->second; | ||
| } else { | ||
| resultValue = createPointerFromIndex(builder, loc, resultIndex); | ||
| ptrCache[resultIndex] = resultValue; | ||
| registerResultMap.insert({key, resultValue}); | ||
| } | ||
|
|
||
| // Update result count | ||
| if (std::cmp_greater_equal(resultIndex, metadata_.numResults)) { | ||
| metadata_.numResults = static_cast<size_t>(resultIndex) + 1; | ||
| } | ||
|
|
||
| // Create mz call | ||
| auto ptrType = LLVM::LLVMPointerType::get(builder.getContext()); | ||
| const auto mzSignature = LLVM::LLVMFunctionType::get( | ||
| LLVM::LLVMVoidType::get(builder.getContext()), {ptrType, ptrType}); | ||
| auto mzDecl = | ||
| getOrCreateFunctionDeclaration(builder, module, QIR_MEASURE, mzSignature); | ||
| builder.create<LLVM::CallOp>(loc, mzDecl, ValueRange{qubit, resultValue}); | ||
|
|
||
| return resultValue; | ||
| } |
There was a problem hiding this comment.
Measurements reused from ptrCache are never recorded in registerResultMap.
In measure(const Value qubit, const int64_t resultIndex), when ptrCache already has an entry for resultIndex, you reuse the pointer but don't insert the corresponding ("c", resultIndex) key into registerResultMap. Since generateOutputRecording() iterates only over registerResultMap, such measurements will never be emitted as array_record_output / result_record_output, even though an mz call is generated.
To make recording consistent (and match the QuartzToQIR lowering behavior), registerResultMap should be updated regardless of whether the pointer came from ptrCache or was freshly created, e.g.:
- Value resultValue{};
- if (const auto it = ptrCache.find(resultIndex); it != ptrCache.end()) {
- resultValue = it->second;
- } else {
- resultValue = createPointerFromIndex(builder, loc, resultIndex);
- ptrCache[resultIndex] = resultValue;
- registerResultMap.insert({key, resultValue});
- }
+ Value resultValue{};
+ if (const auto it = ptrCache.find(resultIndex); it != ptrCache.end()) {
+ resultValue = it->second;
+ } else {
+ resultValue = createPointerFromIndex(builder, loc, resultIndex);
+ ptrCache[resultIndex] = resultValue;
+ }
+ registerResultMap.insert({key, resultValue});You may also want to decide whether the early if (registerResultMap.find(key) != end()) return it->second; should still bypass emitting a new mz call, or if repeated calls with the same resultIndex should produce additional measurements.
There was a problem hiding this comment.
@denialhaag I am pretty sure that @coderabbitai is wrong here. I remember that I did this on purpose. Most likely because repeated measurements on the same classical bit override each other and the output recording only should contain one in that case.
There was a problem hiding this comment.
@burgholzer Thank you for the clarification! You're absolutely right that the early return at line 175 handles the duplicate measurement case elegantly:
if (const auto it = registerResultMap.find(key); it != registerResultMap.end()) {
return it->second; // Prevents duplicate mz calls and duplicate output recording
}This ensures repeated calls to measure(qubit, resultIndex) with the same resultIndex return the cached pointer without generating additional mz calls or duplicate output entries—exactly the override behavior you intended.
However, I believe CodeRabbit is flagging a different edge case: when ptrCache[resultIndex] exists (perhaps populated by staticQubit(resultIndex) due to the shared cache, or by allocClassicalBitRegister for a different register name) but registerResultMap[("c", resultIndex)] does not exist.
In that scenario:
- The early return is not triggered (no entry in
registerResultMapyet) - The pointer is reused from
ptrCache - A new
mzcall is generated - But the pointer is not inserted into
registerResultMap(because the insert is inside theelseblock at line 191) - Therefore,
generateOutputRecording()won't emit output for this measurement
Question: Is this behavior intentional? For example:
- Is mixing
allocClassicalBitRegister("my_reg", ...)with rawmeasure(qubit, index)not a supported usage pattern? - Or should
mzcalls without corresponding output recording be expected when indices overlap?
If this edge case is indeed intended (e.g., to avoid mixing allocation styles or because overlapping indices should skip output recording), then CodeRabbit's suggestion can be disregarded. Otherwise, moving the registerResultMap.insert outside the else block would ensure consistency.
✏️ Learnings added
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp:0-0
Timestamp: 2025-12-10T02:19:50.634Z
Learning: In QIRProgramBuilder::measure(Value, int64_t), the intentional design prevents duplicate measurements: the early return when `registerResultMap.find(key)` succeeds avoids generating multiple `mz` calls to the same classical bit index and ensures output recording contains only one entry per index. This implements an "override" semantic where repeated measurements to the same resultIndex reuse the cached pointer without additional side effects.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp:96-117
Timestamp: 2025-12-08T23:41:55.972Z
Learning: In the QIR (Quantum Intermediate Representation) Builder (mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp), the `ptrCache` is intentionally shared between qubit and result pointer creation (in `staticQubit()` and `measure()` methods) because QIR uses opaque pointers and `inttoptr` conversions for both qubits and results. For any given index N, the LLVM IR pointer representation is identical whether it represents a qubit or a result, so the pointer only needs to be created once and can be safely reused across both contexts.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Operations/StandardGates/BarrierOp.cpp:45-54
Timestamp: 2025-12-09T00:55:11.926Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux), qubits follow single-user/linear-type semantics where each qubit value can only be consumed once, similar to MQTOpt. This invariant makes it safe to dereference getUsers().begin() in canonicalization patterns like MergeSubsequentBarrier in BarrierOp.cpp, as there will be at most one user per qubit output.
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
| QuantumCompilerConfig config; | ||
| config.convertToQIR = CONVERT_TO_QIR; | ||
| config.enableTiming = ENABLE_TIMING; | ||
| config.enableStatistics = ENABLE_STATISTICS; | ||
| config.printIRAfterAllStages = PRINT_IR_AFTER_ALL_STAGES; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "recordIntermediates" --type cpp -C 3Repository: munich-quantum-toolkit/core
Length of output: 8392
🏁 Script executed:
rg -n "recordIntermediates" --type h -C 3Repository: munich-quantum-toolkit/core
Length of output: 1882
🏁 Script executed:
# Also check the QuantumCompilerConfig struct definition
fd "CompilerPipeline.h" --type f -x cat -n {}Repository: munich-quantum-toolkit/core
Length of output: 5390
Add --record-intermediates CLI option to expose the intermediate IR recording feature.
The recordIntermediates field in QuantumCompilerConfig is designed for debugging and testing workflows and is actively used throughout the pipeline to capture IR snapshots at 10 different stages. However, unlike the other four config options (convertToQIR, enableTiming, enableStatistics, printIRAfterAllStages), it has no CLI flag to enable it. Consider adding a --record-intermediates flag to maintain consistency and allow users to access this debugging feature from the command line.
🤖 Prompt for AI Agents
In mlir/tools/mqt-cc/mqt-cc.cpp around lines 128 to 132, the
QuantumCompilerConfig.recordIntermediates field is not exposed via the CLI; add
a new command-line flag --record-intermediates (boolean, default false)
alongside the existing flags, parse it in the argument handling, and assign its
value to config.recordIntermediates so that the intermediate-IR recording
feature can be enabled from the command line; also add a brief help/description
for the new flag consistent with the other option help texts.
There was a problem hiding this comment.
@denialhaag I like the suggestion by @coderabbitai to add such a CLI flag.
There was a problem hiding this comment.
@burgholzer, I agree—adding the --record-intermediates CLI flag would provide a consistent interface for this debugging feature! Here's how to implement it:
1. Add the CLI flag alongside the existing options (after line 62):
const cl::opt<bool>
RECORD_INTERMEDIATES("record-intermediates",
cl::desc("Record intermediate IR at each compiler stage"),
cl::init(false));2. Wire it to the config struct (after line 128):
QuantumCompilerConfig config;
config.convertToQIR = CONVERT_TO_QIR;
config.recordIntermediates = RECORD_INTERMEDIATES; // Add this line
config.enableTiming = ENABLE_TIMING;
config.enableStatistics = ENABLE_STATISTICS;
config.printIRAfterAllStages = PRINT_IR_AFTER_ALL_STAGES;This will enable users to capture IR snapshots at all 10 pipeline stages via:
mqt-cc input.mlir --record-intermediatesThe recorded IR will be populated in the CompilationRecord structure for debugging and testing workflows.
🧠 Learnings used
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1360
File: .github/workflows/reusable-mlir-tests.yml:40-43
Timestamp: 2025-12-05T17:45:37.602Z
Learning: In the munich-quantum-toolkit/core repository, patch releases of LLVM dependencies don't require documentation updates, changelog entries, or additional tests beyond what's validated by passing CI checks.
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:219-221
Timestamp: 2025-10-09T13:14:10.178Z
Learning: The MQT Core project (munich-quantum-toolkit/core repository) uses the C++20 standard, not C++17. C++20 features such as abbreviated function templates (e.g., `const auto&` parameters) are supported and valid in this codebase.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Operations/StandardGates/BarrierOp.cpp:45-54
Timestamp: 2025-12-09T00:55:11.926Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux), qubits follow single-user/linear-type semantics where each qubit value can only be consumed once, similar to MQTOpt. This invariant makes it safe to dereference getUsers().begin() in canonicalization patterns like MergeSubsequentBarrier in BarrierOp.cpp, as there will be at most one user per qubit output.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp (1)
165-205: Cache hit path skips registerResultMap insertion, breaking output recording.When
measure(Value qubit, int64_t resultIndex)finds a cached pointer (line 183-184), it reuses the pointer but never inserts the key-value pair intoregisterResultMap. Theinsert()call on line 188 is only executed in the else branch when creating a new pointer.Since
generateOutputRecording()(lines 551-620) iterates only overregisterResultMap, measurements that reuse cached pointers will never be recorded as output, even though anmzcall is emitted (line 202). This causes silent data loss in the output recording.Apply this fix to insert into
registerResultMapregardless of cache status:Value resultValue{}; if (const auto it = ptrCache.find(resultIndex); it != ptrCache.end()) { resultValue = it->second; } else { resultValue = createPointerFromIndex(builder, loc, resultIndex); ptrCache[resultIndex] = resultValue; - registerResultMap.insert({key, resultValue}); } + registerResultMap.insert({key, resultValue});Note: The early return at lines 177-180 already prevents duplicate insertions for the same key, so moving the insert outside the else branch is safe.
mlir/include/mlir/Dialect/Flux/IR/FluxDialect.h (1)
64-148: Consider adding range-style accessors for qubits as a future convenience.Given earlier discussion about
getInputQubits()/getOutputQubits()(and possibly target/control ranges), this trait would be a natural place to expose inexpensiveArrayRef<Value>-style views over the qubits instead of forcing callers to repeatedly index viagetInputQubit/getOutputQubit.Not blocking for this PR, but worth considering either here or in a follow‑up to avoid ad‑hoc containers in clients.
mlir/lib/Dialect/Quartz/Builder/QuartzProgramBuilder.cpp (1)
413-430: Non-deterministic deallocation order from DenseSet iteration.The iteration over
allocatedQubits(aDenseSet) has non-deterministic order. While functionally correct, this may produce different IR output across runs, complicating debugging and testing.Consider collecting qubits into a sorted container before deallocation for deterministic output:
OwningOpRef<ModuleOp> QuartzProgramBuilder::finalize() { - // Automatically deallocate all remaining allocated qubits - for (Value qubit : allocatedQubits) { + // Collect qubits for deterministic deallocation order + SmallVector<Value> qubitsToDealloc(allocatedQubits.begin(), + allocatedQubits.end()); + // Sort by opaque pointer for stable ordering + llvm::sort(qubitsToDealloc, [](Value a, Value b) { + return a.getAsOpaquePointer() < b.getAsOpaquePointer(); + }); + for (Value qubit : qubitsToDealloc) { create<DeallocOp>(loc, qubit); }mlir/include/mlir/Dialect/Quartz/IR/QuartzDialect.h (1)
64-119: Code duplication with Flux dialect's TargetAndParameterArityTrait.This trait is largely identical to the Flux dialect's version in
mlir/include/mlir/Dialect/Flux/IR/FluxDialect.h(lines 51-148). The main differences are:
- Quartz uses
getQubit/getTarget(reference semantics)- Flux uses
getInputQubit/getOutputQubitandgetInputTarget/getOutputTarget(value semantics with SSA)Consider extracting common static methods and parameter handling into a shared base trait to reduce maintenance burden.
mlir/lib/Dialect/Flux/IR/Operations/StandardGates/BarrierOp.cpp (1)
35-73: Guard against empty user ranges and only-first-user checks inMergeSubsequentBarrierThe loop dereferences
getUsers().begin()unconditionally and only inspects that first user:if (llvm::isa<BarrierOp>( *op.getOutputForInput(qubitsIn[i]).getUsers().begin())) {This has two problems:
- If that output currently has no users, dereferencing
begin()is UB.- If there were ever multiple users, only the first is considered.
A safer and still simple approach is to grab the user range once, check emptiness, and ensure the (single) user is a
BarrierOp, e.g.:- for (size_t i = 0; i < qubitsIn.size(); ++i) { - if (llvm::isa<BarrierOp>( - *op.getOutputForInput(qubitsIn[i]).getUsers().begin())) { + for (size_t i = 0; i < qubitsIn.size(); ++i) { + auto users = op.getOutputForInput(qubitsIn[i]).getUsers(); + if (!users.empty() && llvm::hasSingleElement(users) && + llvm::isa<BarrierOp>(*users.begin())) { anythingToMerge = true; newQubitsOutMap[i] = qubitsIn[i]; } else { newQubitsIn.push_back(qubitsIn[i]); indicesToFill.push_back(i); } }This avoids UB and makes the intended “single barrier user” precondition explicit.
mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp (1)
569-583: (Optional) Deallocation order fromDenseSetis non-deterministic
finalize()deallocates qubits by iteratingvalidQubits, which is aDenseSet:for (const auto qubit : validQubits) { create<DeallocOp>(loc, qubit); }This makes the
DeallocOpsequence order-dependent on hash iteration, which can vary between runs and slightly complicate IR diffs. If deterministic IR is desirable, you could sort the qubits (e.g., by definition order) before deallocating; otherwise, consider adding a brief comment that the deallocation order is intentionally unspecified.
| FluxProgramBuilder::FluxProgramBuilder(MLIRContext* context) | ||
| : OpBuilder(context), ctx(context), loc(getUnknownLoc()), | ||
| module(create<ModuleOp>(loc)) {} | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the FluxProgramBuilder.cpp file
find . -name "FluxProgramBuilder.cpp" -type fRepository: munich-quantum-toolkit/core
Length of output: 125
🏁 Script executed:
# Get a broader search in case file structure differs
find . -name "*FluxProgramBuilder*" -type fRepository: munich-quantum-toolkit/core
Length of output: 187
🏁 Script executed:
# Search for any Flux-related builder files
find . -path "*Flux*" -name "*.cpp" -o -path "*Flux*" -name "*.h" | head -20Repository: munich-quantum-toolkit/core
Length of output: 1188
🏁 Script executed:
# Read the FluxProgramBuilder.cpp file
cat -n mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp | head -60Repository: munich-quantum-toolkit/core
Length of output: 2299
🏁 Script executed:
# Also read the header for context on the class
cat -n mlir/include/mlir/Dialect/Flux/Builder/FluxProgramBuilder.hRepository: munich-quantum-toolkit/core
Length of output: 50385
🏁 Script executed:
# Search for other ModuleOp creation patterns in the codebase
rg "ModuleOp" --type cpp -A 2 -B 2 | head -100Repository: munich-quantum-toolkit/core
Length of output: 7309
🏁 Script executed:
# Search for OpBuilder::create usage patterns
rg "create<ModuleOp>" --type cpp -A 2 -B 2Repository: munich-quantum-toolkit/core
Length of output: 1482
🏁 Script executed:
# Check for module creation in tests or other builder files
fd -e cpp -e h | xargs grep -l "ModuleOp::create" | head -5Repository: munich-quantum-toolkit/core
Length of output: 53
🏁 Script executed:
# Search for OpBuilder class definition in MLIR includes
find . -path "*mlir*" -name "*.h" | xargs grep -l "class OpBuilder" | head -3Repository: munich-quantum-toolkit/core
Length of output: 53
🏁 Script executed:
# Check if there are any issues or discussions about this pattern
rg "insertion point" --type cpp -i | grep -i "module\|builder" | head -20Repository: munich-quantum-toolkit/core
Length of output: 1396
🏁 Script executed:
# Look for any tests that use FluxProgramBuilder constructor
rg "FluxProgramBuilder" --type cpp -A 5 | head -80Repository: munich-quantum-toolkit/core
Length of output: 6219
🌐 Web query:
MLIR OpBuilder create ModuleOp insertion point requirement
💡 Result:
Short answer: OpBuilder must have a valid insertion point (i.e., an insertion Block + iterator) when you call create. You cannot create an operation with no insertion point (clearInsertionPoint() makes creation an error), and ModuleOp will be inserted into the Block/Region corresponding to that insertion point (so create the ModuleOp where you want it in the top-level region). [1][2]
References:
- OpBuilder: create(), InsertPoint, clearInsertionPoint() (insertion point must be set). [1]
- ModuleOp: insertion/insert behavior (operations are placed into the module body before the terminator). [3]
🌐 Web query:
MLIR ModuleOp::create static method API
💡 Result:
Signature (C++):
static ModuleOp ModuleOp::create(mlir::Location loc, llvm::Optionalllvm::StringRef name = llvm::None);
What it does (concise):
- Creates a new builtin.module operation at the given Location.
- Optional name becomes the module sym_name (module attribute) when provided.
- The created ModuleOp contains the single region/block structure expected for modules (with a module terminator). Use OpBuilder::create or push_back to insert the created op into a parent/IR as needed.
References:
[1] mlir/include/mlir/IR/Module.h — ModuleOp declaration.
[2] mlir class reference (ModuleOp::create / build) (generated docs).
[3] Example usage in MLIR examples (creating an empty module via ModuleOp::create).
🏁 Script executed:
# Check OpBuilder constructor in includes
find . -path "*mlir*" -name "*.h" | xargs grep -A 10 "OpBuilder.*MLIRContext" | head -40Repository: munich-quantum-toolkit/core
Length of output: 53
🏁 Script executed:
# Look at the test setup more carefully
grep -A 20 "buildFluxIR\|FluxProgramBuilder builder" mlir/unittests/pipeline/test_compiler_pipeline.cpp | head -50Repository: munich-quantum-toolkit/core
Length of output: 1654
Use ModuleOp::create for top-level module creation in constructor
The constructor currently creates the module via create<ModuleOp>(loc), which requires the OpBuilder to have a valid insertion point. At construction time, no insertion point is set.
Use the static factory method instead:
-FluxProgramBuilder::FluxProgramBuilder(MLIRContext* context)
- : OpBuilder(context), ctx(context), loc(getUnknownLoc()),
- module(create<ModuleOp>(loc)) {}
+FluxProgramBuilder::FluxProgramBuilder(MLIRContext* context)
+ : OpBuilder(context), ctx(context), loc(getUnknownLoc()),
+ module(ModuleOp::create(loc)) {}ModuleOp::create(loc) is the proper MLIR API for creating root-level modules without requiring an insertion point. The initialize() method can then safely set the insertion point within the module as you do at line 44.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FluxProgramBuilder::FluxProgramBuilder(MLIRContext* context) | |
| : OpBuilder(context), ctx(context), loc(getUnknownLoc()), | |
| module(create<ModuleOp>(loc)) {} | |
| FluxProgramBuilder::FluxProgramBuilder(MLIRContext* context) | |
| : OpBuilder(context), ctx(context), loc(getUnknownLoc()), | |
| module(ModuleOp::create(loc)) {} |
🤖 Prompt for AI Agents
In mlir/lib/Dialect/Flux/Builder/FluxProgramBuilder.cpp around lines 35 to 38,
the constructor uses create<ModuleOp>(loc) which requires an OpBuilder insertion
point that isn't set at construction; replace that with the root-level factory
ModuleOp::create(loc) to create the top-level module without needing an
insertion point, then keep the existing initialize() logic that sets the
insertion point inside the module.
There was a problem hiding this comment.
@denialhaag I am pretty sure that @coderabbitai is actually onto something here. The latest CLion update enabled a new clang-tidy rule that also seems to suggest the usage of the static factory methods. This applies very generally across the code base.
There was a problem hiding this comment.
🧩 Analysis chain
@burgholzer Thank you for confirming this! Let me search for other instances of this pattern across the codebase that might need the same fix.
🏁 Script executed:
#!/bin/bash
# Search for similar patterns where create<ModuleOp> is used in constructors
echo "=== Searching for create<ModuleOp> usage in builders ==="
rg "create<ModuleOp>" --type cpp -B 3 -A 1
echo -e "\n=== Checking other ProgramBuilder files ==="
fd "ProgramBuilder.cpp" --type f -x grep -n "create<ModuleOp>" {}
echo -e "\n=== Looking for ModuleOp constructor initialization patterns ==="
rg "ModuleOp.*\(.*create<ModuleOp>" --type cpp -B 2 -A 1
Length of output: 1652
@burgholzer You're absolutely right! I found three instances of this pattern across the codebase, all in the ProgramBuilder constructors:
- FluxProgramBuilder.cpp (line 36) - current file
- QuartzProgramBuilder.cpp (line 37)
- QIRProgramBuilder.cpp (line 39)
All three should be updated to use ModuleOp::create(loc) instead of create<ModuleOp>(loc) (or builder.create<ModuleOp>(...) in QIR's case).
Recommended fixes:
FluxProgramBuilder.cpp:
FluxProgramBuilder::FluxProgramBuilder(MLIRContext* context)
: OpBuilder(context), ctx(context), loc(getUnknownLoc()),
module(ModuleOp::create(loc)) {}QuartzProgramBuilder.cpp:
QuartzProgramBuilder::QuartzProgramBuilder(MLIRContext* context)
: OpBuilder(context), ctx(context), loc(getUnknownLoc()),
module(ModuleOp::create(loc)) {}QIRProgramBuilder.cpp:
QIRProgramBuilder::QIRProgramBuilder(MLIRContext* context)
: builder(context),
module(ModuleOp::create(UnknownLoc::get(context))),
loc(UnknownLoc::get(context)) {}This is a good catch by the new clang-tidy rule! Using the static factory method is the correct MLIR idiom for creating top-level modules before setting an insertion point.
🧠 Learnings used
Learnt from: MatthiasReumann
Repo: munich-quantum-toolkit/core PR: 1301
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp:144-151
Timestamp: 2025-12-02T07:37:46.860Z
Learning: In MLIR transformation code (mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/sc/NaiveRoutingPass.cpp and similar), when inserting operations before a target operation, prefer `rewriter.setInsertionPoint(op)` over `rewriter.setInsertionPointAfter(op->getPrevNode())`. The former is cleaner, avoids null pointer edge cases (when op is first in block), and is semantically clearer.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Operations/StandardGates/BarrierOp.cpp:45-54
Timestamp: 2025-12-09T00:55:11.926Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux), qubits follow single-user/linear-type semantics where each qubit value can only be consumed once, similar to MQTOpt. This invariant makes it safe to dereference getUsers().begin() in canonicalization patterns like MergeSubsequentBarrier in BarrierOp.cpp, as there will be at most one user per qubit output.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp:78-100
Timestamp: 2025-12-08T14:55:43.899Z
Learning: In the Flux dialect (mlir/lib/Dialect/Flux/IR/Modifiers/CtrlOp.cpp), GPhaseOp is a zero-target operation (global phase). When a CtrlOp wraps a GPhaseOp, it only has control qubits and no targets. The CtrlInlineGPhase canonicalization pattern correctly produces outputs only for the positive controls, not targets.
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/lib/Dialect/MQTOpt/Transforms/ReplaceBasisStateControlsWithIfPattern.cpp:171-180
Timestamp: 2025-10-09T13:13:51.224Z
Learning: In MQT Core MLIR, UnitaryInterface operations guarantee 1-1 correspondence between input and output qubits in the same order. When cloning or modifying unitary operations (e.g., removing controls), this correspondence is maintained by construction, so yielding getAllInQubits() in else-branches matches the result types from the operation's outputs.
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp:80-100
Timestamp: 2025-12-08T23:58:09.648Z
Learning: In the Quartz dialect (mlir/lib/Dialect/Quartz/IR/Modifiers/CtrlOp.cpp), quartz.ctrl uses reference semantics and does not return values, unlike flux.ctrl which uses value semantics and returns transformed qubits. When inlining a GPhaseOp in the CtrlInlineGPhase pattern, it's correct to create POp operations for positive controls and erase the CtrlOp without collecting or replacing result values.
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp:96-117
Timestamp: 2025-12-08T23:41:55.972Z
Learning: In the QIR (Quantum Intermediate Representation) Builder (mlir/lib/Dialect/QIR/Builder/QIRProgramBuilder.cpp), the `ptrCache` is intentionally shared between qubit and result pointer creation (in `staticQubit()` and `measure()` methods) because QIR uses opaque pointers and `inttoptr` conversions for both qubits and results. For any given index N, the LLVM IR pointer representation is identical whether it represents a qubit or a result, so the pointer only needs to be created once and can be safely reused across both contexts.
Description
This is a very early draft of the dialect rewrite proposed in #1225
I am opening this now, as I will probably only get back to it in roughly two weeks and I want others to check this out directly.
Checklist: