✨ Add unitary matrix to QCO dialect #1426
Draft
+658
−53
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
taminob
added
enhancement
8 tasks
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduces Eigen-based matrix utilities for quantum gates and parameterized unitary operation traits to the QCO dialect, with Eigen dependency management via CMake FetchContent and configuration updates to exclude Eigen headers from include-cleaner analysis. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @mlir/include/mlir/Dialect/QCO/IR/QCODialect.h:
- Around line 205-222: The permutate function currently computes undoPermutation
but never fills permutatedMatrix or uses swapMatrix; implement the permutation
by applying the provided permutation vector to reorder rows and/or columns of
inputMatrix (depending on intended semantics) into permutatedMatrix and compute
undoPermutation as already started; specifically, iterate over indices i from 0
to dim-1, set permutatedMatrix.row(i) = inputMatrix.row(permutation(i)) (or
column variant if columns should be permuted), keep
undoPermutation(permutation(i)) = i, and remove the unused swapMatrix or use
utils::getMatrixSWAP if a SWAP-based transformation is required; ensure
assertions remain and return {permutatedMatrix, undoPermutation} once populated.
- Around line 192-203: The helper tryGetParameterAsDouble currently creates a
DummyArityType to call
TargetAndParameterArityTrait::Impl<arith::ConstantOp>::getStaticParameter;
refactor by extracting getStaticParameter into a standalone utility function
(e.g., getStaticParameterForConstantOp or getStaticParameterFromAttr) and have
tryGetParameterAsDouble call that utility directly, removing the DummyArityType
alias; update the declaration/definition of the new utility in the same
header/namespace as TargetAndParameterArityTrait and replace usages of
DummyArityType::Impl<arith::ConstantOp>::getStaticParameter with the new
function.
- Line 35: The file currently includes an internal Eigen header path
("unsupported/Eigen/src/KroneckerProduct/KroneckerTensorProduct.h") which is
fragile; replace that internal src include with the public unsupported Eigen
header (for example use the public header under unsupported/Eigen, e.g.
<unsupported/Eigen/KroneckerProduct> or the corresponding public
KroneckerProduct header) so the include is stable across Eigen updates and
remove the direct reference to the src directory.
- Around line 230-236: The loop over region incorrectly returns early when a
cast to mlir::qco::UnitaryOpInterface succeeds: in the block containing the
dyn_cast to UnitaryOpInterface (variable unitaryOp) the code returns result
before calling unitaryOp.getUnitaryMatrix(), preventing any unitary processing;
fix by inverting the condition so you return only when the cast fails (i.e., if
(!unitaryOp) continue/return as intended) and then call
unitaryOp.getUnitaryMatrix() and handle/update result; adjust the loop around
the dyn_cast, getUnitaryMatrix(), and any result accumulation accordingly in the
code paths referencing UnitaryOpInterface and result.
In @mlir/include/mlir/Dialect/QCO/IR/QCOOps.td:
- Line 277: The pattern in operations like GPhaseOp (and RXOp, RYOp, etc.)
unsafely calls tryGetParameterAsDouble(op, 0).value(), which throws if the
parameter is not a compile-time constant; change getUnitaryMatrix generation to
first check the optional (e.g., using has_value()/operator bool or
tryGetParameterAsDouble return check) and handle the dynamic-parameter case
instead of unwrapping: either assert/emit a clear error when a static value is
required, document that getUnitaryMatrix only supports static parameters, or
make getUnitaryMatrix return an error/Optional/MaybeMatrix for non-static
parameters; update the usages that call utils::getMatrixGPhase(...) (and
equivalent getMatrix* helpers) to accept the guarded/optional path so no
.value() is called without a prior check.
In @mlir/include/mlir/Dialect/Utils/MatrixUtils.h:
- Around line 289-292: The current indexing for placing the target block uses
matrix(dim - targetDim - j, dim - targetDim - i) which writes the block reversed
and in the wrong position; update the placement to use matrix(dim - targetDim +
j, dim - targetDim + i) so the targetMatrix(j, i) maps to the bottom-right block
(rows/cols [dim-targetDim, dim)), or replace the loop with Eigen's block API by
assigning matrix.bottomRightCorner(targetDim, targetDim) = targetMatrix for a
clearer, less error-prone fix; locate the assignment inside the function that
builds the controlled gate matrix and update that code path accordingly.
- Line 13: MatrixUtils.h currently includes QCODialect.h, creating a circular
include with QCODialect.h; remove the direct include and either forward-declare
the minimal QCO symbols you need (e.g., declare namespace mlir::qco { class
QCODialect; } or forward-declare specific types used) inside MatrixUtils.h, or
move the dialect-dependent utility functions out of MatrixUtils (or into a new
file) so MatrixUtils.h stays dialect-agnostic; update any implementations that
relied on the QCODialect include to include QCODialect.h in their .cpp or in the
new dialect-specific file instead.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.clang-tidyCMakeLists.txtmlir/include/mlir/Dialect/QCO/IR/QCODialect.hmlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/MatrixUtils.hmlir/lib/Dialect/QCO/IR/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (27)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
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: 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/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.
📚 Learning: 2025-11-03T23:09:26.881Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1287
File: test/qdmi/dd/CMakeLists.txt:9-21
Timestamp: 2025-11-03T23:09:26.881Z
Learning: The CMake functions `generate_device_defs_executable` and `generate_prefixed_qdmi_headers` used in QDMI device test CMakeLists.txt files are provided by the external QDMI library (fetched via FetchContent from https://github.com/Munich-Quantum-Software-Stack/qdmi.git), specifically in the cmake/PrefixHandling.cmake module of the QDMI repository.
Applied to files:
CMakeLists.txtmlir/lib/Dialect/QCO/IR/CMakeLists.txt
📚 Learning: 2026-01-04T23:01:47.734Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1356
File: mlir/CMakeLists.txt:13-15
Timestamp: 2026-01-04T23:01:47.734Z
Learning: In the munich-quantum-toolkit/core repository's MLIR directory (mlir/CMakeLists.txt), the MSVC compile definition `_SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING` is necessary to suppress warnings triggered by MLIR/LLVM internal headers, not because the project code uses non-floating complex types. This suppression is needed to avoid warnings from third-party MLIR/LLVM header includes.
Applied to files:
CMakeLists.txtmlir/lib/Dialect/QCO/IR/CMakeLists.txt
📚 Learning: 2026-01-04T21:41:30.252Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1356
File: cmake/SetupMLIR.cmake:32-33
Timestamp: 2026-01-04T21:41:30.252Z
Learning: In the munich-quantum-toolkit/core repository, the project provides its own pre-built LLVM/MLIR distributions that are compiled with LLVM_ENABLE_RTTI=ON and LLVM_ENABLE_EH=ON. Users are directed to these distributions via the upgrade guide and installation instructions to avoid ABI compatibility issues. The mid-term goal is to eliminate RTTI and exception handling dependencies entirely.
Applied to files:
CMakeLists.txtmlir/lib/Dialect/QCO/IR/CMakeLists.txt
📚 Learning: 2025-12-14T16:51:52.504Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 23
File: .readthedocs.yaml:13-18
Timestamp: 2025-12-14T16:51:52.504Z
Learning: In the munich-quantum-toolkit/core-plugins-catalyst repository, LLVM and MLIR toolchains are required for the documentation build because `uv run` includes a full build of the package, which compiles C++/MLIR extensions using scikit-build-core.
Applied to files:
CMakeLists.txt
📚 Learning: 2025-12-05T15:54:56.385Z
Learnt from: flowerthrower
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 3
File: lib/Conversion/CatalystQuantumToMQTOpt/CMakeLists.txt:19-20
Timestamp: 2025-12-05T15:54:56.385Z
Learning: The munich-quantum-toolkit/core-plugins-catalyst project does not support Windows builds, so MSVC-specific compiler options or cross-platform compatibility with MSVC is not required.
Applied to files:
CMakeLists.txt
📚 Learning: 2026-01-04T23:54:33.540Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1356
File: src/qdmi/na/CMakeLists.txt:179-184
Timestamp: 2026-01-04T23:54:33.540Z
Learning: In the munich-quantum-toolkit/core repository, MSVC link-time code generation (LTCG) must be disabled for the NA device dynamic library target (mqt-core-qdmi-na-device-dyn) on Windows to avoid link errors and missing symbols. This issue affects both release-only builds and debug/release mixed builds, not just debug/release mixing scenarios.
Applied to files:
CMakeLists.txt
📚 Learning: 2025-12-05T15:57:39.701Z
Learnt from: flowerthrower
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 3
File: lib/Conversion/CatalystQuantumToMQTOpt/CMakeLists.txt:22-25
Timestamp: 2025-12-05T15:57:39.701Z
Learning: The munich-quantum-toolkit projects (core and core-plugins-catalyst) use `file(GLOB_RECURSE ...)` patterns in CMakeLists.txt files to collect header files, following an established convention in the parent repository for consistency across the codebase.
Applied to files:
CMakeLists.txtmlir/lib/Dialect/QCO/IR/CMakeLists.txt
📚 Learning: 2025-12-28T17:14:53.890Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1403
File: src/qdmi/na/CMakeLists.txt:31-38
Timestamp: 2025-12-28T17:14:53.890Z
Learning: In the munich-quantum-toolkit/core repository, the NA device generator target (mqt-core-qdmi-na-device-gen) is intentionally propagated to MQT_CORE_TARGETS via list(APPEND) because it's publicly linked to the NA device library (the NA device uses a public function from the generator). The SC device generator is not propagated because it has no such dependency with the SC device library.
Applied to files:
CMakeLists.txt
📚 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/include/mlir/Dialect/QCO/IR/QCOInterfaces.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/MatrixUtils.hmlir/include/mlir/Dialect/QCO/IR/QCODialect.h
📚 Learning: 2026-01-07T12:29:16.380Z
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1070-1085
Timestamp: 2026-01-07T12:29:16.380Z
Learning: In the QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp), the ConvertQCOFuncFuncOp pattern assumes that when a func.func operation is matched for conversion, all of its arguments are qco.qubit types (never mixed qubit/classical). The pattern unconditionally converts all arguments to qc::QubitType based on this assumption.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QCO/IR/QCODialect.h
📚 Learning: 2026-01-07T12:29:02.062Z
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1040-1050
Timestamp: 2026-01-07T12:29:02.062Z
Learning: In the QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp), ConvertQCOFuncCallOp assumes that if a func::CallOp has qubit results, then all arguments and results are qubits (no mixed classical/quantum types). The conversion is scoped to handle all-qubit function calls only.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/MatrixUtils.hmlir/include/mlir/Dialect/QCO/IR/QCODialect.h
📚 Learning: 2025-12-17T17:44:31.349Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/include/mlir/Dialect/QCO/IR/QCOOps.td:259-259
Timestamp: 2025-12-17T17:44:31.349Z
Learning: In the QCO dialect (mlir/include/mlir/Dialect/QCO/IR/QCOOps.td), GPhaseOp intentionally uses `MemoryEffects<[MemWrite]>` instead of `Pure` to prevent the remove-dead-values pass from eliminating it. Since GPhaseOp is a zero-target operation with no result values, it would otherwise be removed by DCE, even though it has a meaningful effect on the global quantum state.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.tdmlir/include/mlir/Dialect/QCO/IR/QCODialect.h
📚 Learning: 2025-12-08T23:58:09.648Z
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.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/MatrixUtils.hmlir/include/mlir/Dialect/QCO/IR/QCODialect.h
📚 Learning: 2025-12-09T00:55:11.926Z
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.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.tdmlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/MatrixUtils.hmlir/include/mlir/Dialect/QCO/IR/QCODialect.h
📚 Learning: 2025-12-08T23:41:55.972Z
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.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.tdmlir/include/mlir/Dialect/QCO/IR/QCODialect.h
📚 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/lib/Dialect/QCO/IR/CMakeLists.txt
📚 Learning: 2025-12-17T11:32:53.683Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/lib/Dialect/MQTOpt/Transforms/Transpilation/LayeredUnit.cpp:83-86
Timestamp: 2025-12-17T11:32:53.683Z
Learning: In the mlir directory of the munich-quantum-toolkit/core repository, the clang-tidy configuration (mlir/.clang-tidy) prefers marking free functions as `static` over placing them in anonymous namespaces. The configuration enables `llvm-prefer-static-over-anonymous-namespace` and disables `misc-use-anonymous-namespace`. Types/classes should still use anonymous namespaces, but free functions should use explicit `static` linkage.
Applied to files:
.clang-tidy
📚 Learning: 2025-12-22T01:25:21.609Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/ir/register_permutation.cpp:153-171
Timestamp: 2025-12-22T01:25:21.609Z
Learning: In the munich-quantum-toolkit/core repository, when using nanobind iterator factory functions like `make_key_iterator` and `make_iterator`, the unqualified form (without explicit `nb::` prefix) is preferred. The clang-tidy configuration suggests removal of explicit namespace qualification, relying on ADL (Argument-Dependent Lookup) to resolve these functions correctly.
Applied to files:
.clang-tidy
📚 Learning: 2025-12-17T17:44:21.624Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1264
File: mlir/include/mlir/Dialect/QCO/IR/QCOOps.td:259-259
Timestamp: 2025-12-17T17:44:21.624Z
Learning: In mlir/include/mlir/Dialect/QCO/IR/QCOOps.td (QCO dialect), ensure GPhaseOp declares MemoryEffects<[MemWrite]> rather than Pure. This op has no results and is a zero-target operation, so using Pure would lead to its removal by dead-code elimination. By marking it with MemWrite, review ensures DCE preserves the operation because it has a meaningful effect on the global quantum state. This guidance applies when reviewing or updating QCO ops in this file (and broadly to similar zero-target ops with side effects in QCO).
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOOps.td
📚 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/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/MatrixUtils.hmlir/include/mlir/Dialect/QCO/IR/QCODialect.h
📚 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/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/QCO/IR/QCODialect.h
📚 Learning: 2025-12-08T14:55:43.899Z
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.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/MatrixUtils.hmlir/include/mlir/Dialect/QCO/IR/QCODialect.h
📚 Learning: 2025-11-01T15:57:31.153Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1283
File: src/qir/runtime/QIR.cpp:196-201
Timestamp: 2025-11-01T15:57:31.153Z
Learning: In the QIR runtime (src/qir/runtime/QIR.cpp), the PRX gate (__quantum__qis__prx__body) is an alias for the R gate (Phased X-Rotation) and should call runtime.apply<qc::R>(theta, phi, qubit), not runtime.apply<qc::RX>() which is a single-parameter rotation gate.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOOps.td
📚 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/QCO/IR/QCOOps.td
📚 Learning: 2025-12-08T12:44:05.883Z
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.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/MatrixUtils.hmlir/include/mlir/Dialect/QCO/IR/QCODialect.h
📚 Learning: 2025-12-08T23:16:20.680Z
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.
Applied to files:
mlir/include/mlir/Dialect/QCO/IR/QCOOps.tdmlir/include/mlir/Dialect/Utils/MatrixUtils.h
🧬 Code graph analysis (1)
mlir/include/mlir/Dialect/Utils/MatrixUtils.h (1)
mlir/include/mlir/Dialect/QCO/IR/QCODialect.h (2)
mlir(58-179)mlir(190-222)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (windows-2025) / 🐍 windows-2025
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🐍 Test (macos-15) / 🐍 macos-15
- GitHub Check: 🇨 Test 🏁 (windows-2025, msvc, Debug) / 🏁 windows-2025 msvc Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Test 🏁 (windows-2025, msvc, Release) / 🏁 windows-2025 msvc Release
- GitHub Check: 🇨 Test 🍎 (macos-15, clang, Release) / 🍎 macos-15 clang Release
- GitHub Check: 🇨 Test 🍎 (macos-15, clang, Debug) / 🍎 macos-15 clang Debug
- GitHub Check: 🇨 Coverage / 📈 Coverage
- GitHub Check: 🐍 Lint / 🚨 Lint
🔇 Additional comments (11)
.clang-tidy (1)
77-78: LGTM!Appropriate configuration to exclude Eigen headers from
misc-include-cleaneranalysis. This prevents clang-tidy noise from third-party header-only library includes.CMakeLists.txt (2)
107-110: Verify target architecture detection for cross-compilation.The condition uses
CMAKE_HOST_SYSTEM_PROCESSORwhich detects the build machine's architecture, not the target architecture. For cross-compilation scenarios (e.g., building ARM64 binaries on x64), this should useCMAKE_SYSTEM_PROCESSORinstead.🔧 Suggested fix
- if(WIN32 AND ${CMAKE_HOST_SYSTEM_PROCESSOR} STREQUAL ARM64) + if(WIN32 AND CMAKE_SYSTEM_PROCESSOR STREQUAL "ARM64")
101-106: LGTM - Eigen dependency setup.Fetching Eigen 3.4.1 via FetchContent with shallow clone is appropriate. Version 3.4.1 is the latest stable patch in the 3.x series with no known security advisories.
mlir/lib/Dialect/QCO/IR/CMakeLists.txt (1)
24-28: LGTM!Adding
Eigen3::Eigenas a PUBLIC dependency is correct since the QCO dialect headers expose Eigen types (e.g.,Eigen::MatrixXcdin the interface).mlir/include/mlir/Dialect/QCO/IR/QCOInterfaces.td (3)
93-103: LGTM!Updated convenience methods correctly use the
$_opproxy for accessing operation properties.
113-123: Verify error message string construction.The error message concatenation
"Operation '" + $_op.getBaseSymbol() + "' has no..."may not compile correctly sincegetBaseSymbol()returnsStringRef, which doesn't directly supportoperator+withconst char*. Consider usingllvm::Twineorstd::stringconversion.🔧 Suggested fix
[{ if constexpr (requires { $_op.getUnitaryMatrixDefinition(); }) { return $_op.getUnitaryMatrixDefinition(); } else { - llvm::reportFatalUsageError("Operation '" + $_op.getBaseSymbol() + "' has no unitary matrix definition!"); + llvm::reportFatalUsageError(llvm::Twine("Operation '") + $_op.getBaseSymbol() + "' has no unitary matrix definition!"); } }]
126-131: Verify whether$_opis substituted inextraTraitClassDeclaration.In MLIR's TableGen,
$_opsubstitution is standard inInterfaceMethodbodies, but its availability inextraTraitClassDeclarationis unclear. This context receives C++ code that gets injected into the trait class, where methods would typically access the operation viathis. If$_opis not substituted here, the code will fail to compile. Consider usingstatic_cast<ConcreteType*>(this)->getUnitaryMatrixDefinition()or verifying that TableGen performs substitution in trait class declarations.mlir/include/mlir/Dialect/QCO/IR/QCOOps.td (2)
241-250: LGTM - Well-designed TableGen infrastructure for matrix definitions.The
MatrixDefinitionLambdaandFixedSizeMatrixDefinitionLambdaclasses provide a clean abstraction for generating type-safe matrix definition lambdas. The conditional code generation (line 243) correctly handles operations without matrix definitions.
1057-1066: Verify CtrlOp matrix generation correctness.The matrix definition relies on
getBlockMatrixandgetCtrlBody, both of which have implementation issues flagged inQCODialect.h. Ensure those are resolved for this to work correctly.mlir/include/mlir/Dialect/Utils/MatrixUtils.h (2)
26-163: LGTM - Single-qubit gate matrices.The matrix definitions follow standard quantum computing conventions and use proper complex number representations with
std::complex_literals.
165-272: LGTM - Two-qubit gate matrices.The two-qubit gate matrix implementations (SWAP, iSWAP, DCX, ECR, and parameterized gates) are correctly defined.
| #include <mlir/Interfaces/SideEffectInterfaces.h> | ||
| #include <optional> | ||
| #include <string> | ||
| #include <unsupported/Eigen/src/KroneckerProduct/KroneckerTensorProduct.h> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer the public Eigen header over internal path.
Including the internal path unsupported/Eigen/src/KroneckerProduct/KroneckerTensorProduct.h is fragile and may break with Eigen updates. Use the public include path instead.
♻️ Suggested fix
-#include <unsupported/Eigen/src/KroneckerProduct/KroneckerTensorProduct.h>
+#include <unsupported/Eigen/KroneckerProduct>📝 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.
Suggested change
| #include <unsupported/Eigen/src/KroneckerProduct/KroneckerTensorProduct.h> | |
| #include <unsupported/Eigen/KroneckerProduct> |
🤖 Prompt for AI Agents
In @mlir/include/mlir/Dialect/QCO/IR/QCODialect.h at line 35, The file currently
includes an internal Eigen header path
("unsupported/Eigen/src/KroneckerProduct/KroneckerTensorProduct.h") which is
fragile; replace that internal src include with the public unsupported Eigen
header (for example use the public header under unsupported/Eigen, e.g.
<unsupported/Eigen/KroneckerProduct> or the corresponding public
KroneckerProduct header) so the include is stable across Eigen updates and
remove the direct reference to the src directory.
Comment on lines
+192
to
+203
| [[nodiscard]] inline std::optional<double> | ||
| tryGetParameterAsDouble(UnitaryOpInterface op, size_t i) { | ||
| using DummyArityType = | ||
| TargetAndParameterArityTrait<0, 0, Eigen::MatrixXcd, nullptr>; | ||
| const auto param = op.getParameter(i); | ||
| const auto floatAttr = | ||
| DummyArityType::Impl<arith::ConstantOp>::getStaticParameter(param); | ||
| if (!floatAttr) { | ||
| return std::nullopt; | ||
| } | ||
| return floatAttr.getValueAsDouble(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unusual pattern: using trait as utility class.
DummyArityType instantiates TargetAndParameterArityTrait solely to access the static getStaticParameter method. Consider extracting getStaticParameter as a standalone utility function to avoid this indirection.
🤖 Prompt for AI Agents
In @mlir/include/mlir/Dialect/QCO/IR/QCODialect.h around lines 192 - 203, The
helper tryGetParameterAsDouble currently creates a DummyArityType to call
TargetAndParameterArityTrait::Impl<arith::ConstantOp>::getStaticParameter;
refactor by extracting getStaticParameter into a standalone utility function
(e.g., getStaticParameterForConstantOp or getStaticParameterFromAttr) and have
tryGetParameterAsDouble call that utility directly, removing the DummyArityType
alias; update the declaration/definition of the new utility in the same
header/namespace as TargetAndParameterArityTrait and replace usages of
DummyArityType::Impl<arith::ConstantOp>::getStaticParameter with the new
function.
Comment on lines
+205
to
+222
| [[nodiscard]] inline std::pair<Eigen::MatrixXcd, Eigen::VectorXi> | ||
| permutate(const Eigen::MatrixXcd& inputMatrix, | ||
| const Eigen::VectorXi& permutation) { | ||
| const auto swapMatrix = utils::getMatrixSWAP(); | ||
|
|
||
| auto dim = inputMatrix.cols(); | ||
| assert(inputMatrix.cols() == inputMatrix.rows()); | ||
| assert(dim == permutation.size()); | ||
|
|
||
| Eigen::MatrixXcd permutatedMatrix(dim, dim); | ||
| Eigen::VectorXi undoPermutation(permutation.size()); | ||
| for (int i = 0; i < permutation.size(); ++i) { | ||
| undoPermutation(permutation(i)) = i; | ||
| // TODO | ||
| } | ||
|
|
||
| return {permutatedMatrix, undoPermutation}; | ||
| } |
Contributor
There was a problem hiding this comment.
Incomplete implementation: permutate function doesn't perform permutation.
The function declares permutatedMatrix but never populates it - only undoPermutation is partially filled. The swapMatrix is fetched but unused. Consider either completing the implementation or marking this as explicitly unimplemented.
Would you like me to help implement the permutation logic, or should this function be marked with a more explicit "not implemented" error?
🤖 Prompt for AI Agents
In @mlir/include/mlir/Dialect/QCO/IR/QCODialect.h around lines 205 - 222, The
permutate function currently computes undoPermutation but never fills
permutatedMatrix or uses swapMatrix; implement the permutation by applying the
provided permutation vector to reorder rows and/or columns of inputMatrix
(depending on intended semantics) into permutatedMatrix and compute
undoPermutation as already started; specifically, iterate over indices i from 0
to dim-1, set permutatedMatrix.row(i) = inputMatrix.row(permutation(i)) (or
column variant if columns should be permuted), keep
undoPermutation(permutation(i)) = i, and remove the unused swapMatrix or use
utils::getMatrixSWAP if a SWAP-based transformation is required; ensure
assertions remain and return {permutatedMatrix, undoPermutation} once populated.
Comment on lines
230
to
236
| for (auto&& block : region) { | ||
| for (auto&& op : block) { | ||
| auto unitaryOp = llvm::dyn_cast<mlir::qco::UnitaryOpInterface>(op); | ||
| if (unitaryOp) { | ||
| return result; | ||
| } | ||
| auto matrix = unitaryOp.getUnitaryMatrix(); |
Contributor
There was a problem hiding this comment.
Critical logic error: early return prevents matrix computation.
The condition on line 233 checks if the cast succeeded, but line 234 returns result immediately inside the success branch, before using unitaryOp.getUnitaryMatrix() on line 236. This means the function never actually processes any unitary operations.
🐛 Proposed fix
for (auto&& op : block) {
auto unitaryOp = llvm::dyn_cast<mlir::qco::UnitaryOpInterface>(op);
- if (unitaryOp) {
- return result;
- }
- auto matrix = unitaryOp.getUnitaryMatrix();
+ if (!unitaryOp) {
+ continue;
+ }
+ auto matrix = unitaryOp.getUnitaryMatrix();📝 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.
Suggested change
| for (auto&& block : region) { | |
| for (auto&& op : block) { | |
| auto unitaryOp = llvm::dyn_cast<mlir::qco::UnitaryOpInterface>(op); | |
| if (unitaryOp) { | |
| return result; | |
| } | |
| auto matrix = unitaryOp.getUnitaryMatrix(); | |
| for (auto&& block : region) { | |
| for (auto&& op : block) { | |
| auto unitaryOp = llvm::dyn_cast<mlir::qco::UnitaryOpInterface>(op); | |
| if (!unitaryOp) { | |
| continue; | |
| } | |
| auto matrix = unitaryOp.getUnitaryMatrix(); |
🤖 Prompt for AI Agents
In @mlir/include/mlir/Dialect/QCO/IR/QCODialect.h around lines 230 - 236, The
loop over region incorrectly returns early when a cast to
mlir::qco::UnitaryOpInterface succeeds: in the block containing the dyn_cast to
UnitaryOpInterface (variable unitaryOp) the code returns result before calling
unitaryOp.getUnitaryMatrix(), preventing any unitary processing; fix by
inverting the condition so you return only when the cast fails (i.e., if
(!unitaryOp) continue/return as intended) and then call
unitaryOp.getUnitaryMatrix() and handle/update result; adjust the loop around
the dyn_cast, getUnitaryMatrix(), and any result accumulation accordingly in the
code paths referencing UnitaryOpInterface and result.
| //===----------------------------------------------------------------------===// | ||
|
|
||
| def GPhaseOp : QCOOp<"gphase", traits = [UnitaryOpInterface, ZeroTargetOneParameter, MemoryEffects<[MemWrite]>]> { | ||
| def GPhaseOp : QCOOp<"gphase", traits = [UnitaryOpInterface, ZeroTargetOneParameterUnitaryMatrix<[{ return utils::getMatrixGPhase(tryGetParameterAsDouble(op, 0).value()); }]>, MemoryEffects<[MemWrite]>]> { |
Contributor
There was a problem hiding this comment.
Unchecked .value() call may throw for non-static parameters.
tryGetParameterAsDouble(op, 0).value() will throw std::bad_optional_access if the parameter is not a compile-time constant. This affects all parameterized gates (GPhaseOp, RXOp, RYOp, etc.). Consider either:
- Adding a guard/assertion before accessing
.value() - Documenting that
getUnitaryMatrix()is only valid for static parameters - Returning an error rather than a matrix for dynamic parameters
Example affected operations
- Line 277: GPhaseOp
- Line 544: RXOp
- Line 571: RYOp
- Line 598: RZOp
- Line 625: POp
- Line 652: ROp
- Line 680: U2Op
- Line 708: UOp
- And all two-qubit parameterized gates...
🤖 Prompt for AI Agents
In @mlir/include/mlir/Dialect/QCO/IR/QCOOps.td at line 277, The pattern in
operations like GPhaseOp (and RXOp, RYOp, etc.) unsafely calls
tryGetParameterAsDouble(op, 0).value(), which throws if the parameter is not a
compile-time constant; change getUnitaryMatrix generation to first check the
optional (e.g., using has_value()/operator bool or tryGetParameterAsDouble
return check) and handle the dynamic-parameter case instead of unwrapping:
either assert/emit a clear error when a static value is required, document that
getUnitaryMatrix only supports static parameters, or make getUnitaryMatrix
return an error/Optional/MaybeMatrix for non-static parameters; update the
usages that call utils::getMatrixGPhase(...) (and equivalent getMatrix* helpers)
to accept the guarded/optional path so no .value() is called without a prior
check.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR implements the feature of the unitary matrix definition for
UnitaryOpInterfacewhich has been removed from the original dialect rewrite PR in favor of this follow-up.It aims to allow access to the matrix definition of the standard gates via a fast, stack-allocated matrix returned by the function
getUnitaryMatrixDefinition()in the trait.A dynamic, heap-based matrix (size
Eigen::Dynamic) is returned by theUnitaryOpInterface. This requires a copy at the moment and further evaluations are required if all matrices should be moved to the heap or if there is a way to make use of the draft functiongetFastUnitaryMatrix()which uses a template parameter, but is currently also only accessible via a trait and thus needs a cast to the correct type first.The changes require the newly introduced
Eigenlibrary for its linear algebra type.For now, it uses the unsupported kronecker product feature which needs to be discussed before merging. "Unsupported" features in Eigen are community-contributed and are not an indication of how mature the code is. However, the maintainers do not provide any API guarantees.
Checklist: