Catalyst Quantum -> QC Conversion#87
Draft
NiklasKostrzewa wants to merge 9 commits intomunich-quantum-toolkit:mainfrom
Draft
Catalyst Quantum -> QC Conversion#87NiklasKostrzewa wants to merge 9 commits intomunich-quantum-toolkit:mainfrom
NiklasKostrzewa wants to merge 9 commits intomunich-quantum-toolkit:mainfrom
Conversation
cmake/ExternalDependencies.cmake MQT_CORE_REV -> "main" include/mlir/Conversion Filename CatalystQuantumToQC -> Changed in CMakeLists.txt include/mlir/Conversion/CatalystQuantumToQC Filenames to QC include/mlir/Conversion/CatalystQuantumToQC/CMakeLists.txt MQTOpt replaced by QC include/mlir/Conversion/CatalystQuantumToQC/.h MQTOpt replaced by QC include/mlir/Conversion/CatalystQuantumToQC/.td MQTOpt replaced by QC namespace dependent dialects: ::mlir::qc lib/CMakelists.txt MQTOpt replaced by QC lib/mqt-plugin.cpp #includes exclude QCToCatalystQuantum, MQTOpt replaced by QC change register namespaces to ::mlir:: lib/Conversion Filename CatalystQuantumToQC -> changed also in CMakeLists.txt lib/Conversion/CatalystQuantumToQC/CMakeLists.txt MLIRQCDialect in add_mlir_library MQTOpt replaced by QC lib/Conversion/CatalystQuantumToQC/.cpp MQTOpt replaced by QC
…sed on quantum -> MQTOpt conversion)
…tes (based of catalyst -> mqtopt conversions)
…es based on mqtopt -> quantum conversion
…ugin to test quantum -> qc conversion (commented out qc -> quantum conversion
Author
|
For discussion: |
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
1 similar comment
Contributor
✅ Actions performedFull review triggered. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request replaces MQTOpt-based conversion passes and build wiring with QC-based equivalents: deletes CatalystQuantum↔MQTOpt and MQTOpt↔CatalystQuantum passes, adds CatalystQuantum↔QC and QC↔CatalystQuantum passes and TableGen/CMake entries, updates plugin and tests, and tweaks an external dependency cache variable. Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Catalyst Quantum IR
participant Conv1 as CatalystQuantumToQC Pass
participant QC as QC Dialect
participant Conv2 as QCToCatalystQuantum Pass
participant Target as Catalyst Quantum IR
Source->>Conv1: submit IR for conversion
Conv1->>Conv1: apply type converter (Qubit/Qureg)
Conv1->>Conv1: apply OpConversionPatterns (Alloc/Measure/Custom/etc.)
Conv1->>QC: emit QC operations
QC->>Conv2: submit QC IR for conversion
Conv2->>Conv2: apply type converter (QC Qubit/MemRef)
Conv2->>Conv2: apply OpConversionPatterns (Load/Store/Measure/etc.)
Conv2->>Target: emit Catalyst Quantum operations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can get early access to new features in CodeRabbit.Enable the |
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/test_plugin.py (1)
65-78:⚠️ Potential issue | 🟠 MajorDon't delete every top-level
*.mlirfile.This autouse fixture runs from
Path.cwd()and unlinks all top-level.mlirfiles before and after each test. When pytest runs in the repo root, that will remove checked-in MLIR fixtures in addition to the generated intermediates.🧹 Limit cleanup to this test module's generated files
def _cleanup_mlir_artifacts() -> None: """Clean up MLIR intermediate files and directories created by Catalyst. @@ # Remove all module_N directories for module_dir in mlir_dir.glob("module_*"): if module_dir.is_dir(): shutil.rmtree(module_dir) - # Remove any loose .mlir files - for mlir_file in mlir_dir.glob("*.mlir"): - mlir_file.unlink() + # Remove only the loose files this test module creates. + for name in (MLIR_FILE_INIT, MLIR_FILE_TO_QC, MLIR_FILE_TO_CATALYST): + mlir_file = mlir_dir / name + if mlir_file.exists(): + mlir_file.unlink()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_plugin.py` around lines 65 - 78, The cleanup function _cleanup_mlir_artifacts currently uses Path.cwd() and unlinks all top-level "*.mlir" files which can delete checked-in fixtures; change it to only remove MLIR files that are part of generated module_N directories (e.g., iterate module_dir.glob("*.mlir") and unlink those, and remove module_dir via shutil.rmtree only when module_dir.is_dir()), and stop unlinking mlir files directly from Path.cwd() so files at the repo root are not touched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/ExternalDependencies.cmake`:
- Around line 18-21: Replace the temporary fork/branch references in the
external dependency variables: set MQT_CORE_REPO_OWNER back to the official
owner ("munich-quantum-toolkit") and change MQT_CORE_REV from the branch name
("test_Memref_explicit") to a stable pinned commit hash (replace the branch with
the specific commit hash you intend to use), and update/remove the inconsistent
inline comment referencing 7d1d7fb so the REV and comment match; these changes
are located where MQT_CORE_REV and MQT_CORE_REPO_OWNER are defined.
In `@include/mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.td`:
- Around line 11-22: The QCToCatalystQuantum pass (QCToCatalystQuantum) is
defined but not registered because registerQCToCatalystQuantumPasses() is
commented out in lib/mqt-plugin.cpp, so runtime lookups (e.g.
`@apply_pass`("mqt.qc-to-catalystquantum") and DEFAULT_PIPELINE referencing
"qc-to-catalystquantum") fail; to fix, uncomment and enable the call to
registerQCToCatalystQuantumPasses() in lib/mqt-plugin.cpp (or add conditional
registration gated by a readiness flag) so that the pass is registered with MLIR
when the implementation/back-conversion is complete and tests can find
mqt.qc-to-catalystquantum.
In `@lib/CMakeLists.txt`:
- Around line 25-26: The plugin module currently links only CatalystQuantumToQC
via the add_llvm_library(${TARGET_NAME} MODULE mqt-plugin.cpp LINK_LIBS
CatalystQuantumToQC) invocation; update that add_llvm_library call to also link
QCToCatalystQuantum so the back-conversion pass is exported by the plugin.
Locate the add_llvm_library line (with TARGET_NAME and mqt-plugin.cpp) and add
QCToCatalystQuantum to the LINK_LIBS list alongside CatalystQuantumToQC.
In `@lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp`:
- Line 319: The local variable qcOp is unused and should be removed: delete the
declaration "Operation* qcOp = nullptr;" and replace any assignment like "qcOp =
rewriter.create(...);" with just "rewriter.create(...);", relying on the side
effects of rewriter.create; ensure no other code references qcOp (search for
qcOp) and compile to verify the -Wunused-but-set-variable warning is resolved.
- Around line 446-515: The adjoint flag (op.getAdjoint()) is ignored for
parameterized gates (RX/RY/RZ, PhaseShift/P, CRX/CRY/CRZ,
ControlledPhaseShift/POp, IsingXY/IsingXX/IsingYY/IsingZZ and their lowering
macros CREATE_* like CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP and
CREATE_TWO_TARGET_ONE_PARAMETER_GATE_OP), so update the lowering to check
op.getAdjoint() and emit the adjointed operation: for single-parameter rotations
and phase gates negate the parameter (use -paramsValues[0]) when adjoint is
true; for controlled variants negate the same parameter passed into the inner
rewriter.create<qc::RXOp/RYOp/RZOp/POp>; for IsingXY negate the first parameter
(keep the added pi as-is) when adjoint is true; and for
CREATE_TWO_TARGET_ONE_PARAMETER_GATE_OP usages (RXX/RYY/RZZ) negate the
parameter when adjoint is set so the lowered qcOp preserves semantics. Ensure
you reference and modify the branches that inspect gateName (e.g., the RX/RY/RZ,
PhaseShift, CRX/CRY/CRZ, ControlledPhaseShift, Ising* branches) and use
op.getAdjoint() to decide whether to pass paramsValues[0] or -paramsValues[0].
In `@lib/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.cpp`:
- Around line 39-329: The file defines conversions
(QCToCatalystQuantumTypeConverter, ConvertQCAlloc, ConvertQCDealloc,
ConvertQCMeasure, ConvertQCLoad, ConvertQCStore, and helpers like
extractControlInfo/extractOperands) but never implements or registers the pass;
add a pass implementation using GEN_PASS_DEF_QCTOCATALYSTQUANTUM that defines
QCToCatalystQuantum::runOnOperation(), builds a ConversionTarget, instantiates
QCToCatalystQuantumTypeConverter, populates a RewritePatternSet with the
provided OpConversionPattern classes (ConvertQCAlloc, ConvertQCDealloc,
ConvertQCMeasure, ConvertQCLoad, ConvertQCStore, etc.), marks legal/illegal ops
appropriately, and call applyFullConversion/applyPartialConversion; finally
register the pass so the declared pass actually runs in tests.
- Around line 319-324: The InsertOp result is being dropped which loses the
write; instead capture the value returned by
rewriter.create<catalyst::quantum::InsertOp> (the new !quantum.reg) and thread
it to later users: replace subsequent uses of the original memref/register SSA
value (the value from adaptor.getMemref()) with the InsertOp result (or update
your value-mapping used during conversion) before calling rewriter.eraseOp(op),
so the write becomes the SSA value consumed by later ops (refer to
catalyst::quantum::InsertOp, adaptor.getMemref(), adaptor.getValue(), and the
current rewriter.eraseOp(op) call).
In `@lib/mqt-plugin.cpp`:
- Around line 46-47: Back-conversion registration
(mqt::ir::conversions::registerQCToCatalystQuantumPasses) is commented out which
breaks tests that expect the full roundtrip (DEFAULT_PIPELINE includes
"qc-to-catalystquantum"); add a clear TODO/FIXME above the commented line that
names registerQCToCatalystQuantumPasses and references DEFAULT_PIPELINE, and
either (a) briefly describe what remains to be implemented for back-conversion
or (b) add a short conditional/feature-flag note explaining why it is disabled
so future readers/tests can find and re-enable it.
- Around line 11-13: The build is failing because QCToCatalystQuantum.h depends
on a generated QCToCatalystQuantum.h.inc that isn't being produced or made
available; fix by making the TableGen-generated header available and/or
including the generated .inc explicitly: ensure your build (CMake) invokes the
tablegen step that produces QCToCatalystQuantum.h.inc and add the generated
include directory as an include path for the target that builds
lib/mqt-plugin.cpp (or update the target that defines the QCToCatalystQuantum
tablegen to export the generated header), and if the public header expects the
.inc, confirm or add the line to include
"mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.h.inc" from
QCToCatalystQuantum.h so the generated declarations are pulled in at compile
time.
In `@test/test_plugin.py`:
- Around line 552-565: The CHECK expectations are written for nested qc.ctrl
blocks but ConvertQuantumCustomOp emits a single qc.ctrl with a flattened
control list; update the check strings (e.g., the check_after_qc block and the
similar blocks at the other locations) to expect one qc.ctrl containing the full
control list rather than nested qc.ctrls, and remove the stray trailing comma in
the Toffoli pattern; locate occurrences by looking for the
check_after_qc/related CHECK string literals and adjust the qc.ctrl patterns to
a flattened form (single qc.ctrl with multiple %[[Q?_#:.*]] captures) for
RX/RY/RZ and Toffoli expectations.
---
Outside diff comments:
In `@test/test_plugin.py`:
- Around line 65-78: The cleanup function _cleanup_mlir_artifacts currently uses
Path.cwd() and unlinks all top-level "*.mlir" files which can delete checked-in
fixtures; change it to only remove MLIR files that are part of generated
module_N directories (e.g., iterate module_dir.glob("*.mlir") and unlink those,
and remove module_dir via shutil.rmtree only when module_dir.is_dir()), and stop
unlinking mlir files directly from Path.cwd() so files at the repo root are not
touched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 645359af-01be-4279-bbde-dc43b03e5cf0
📒 Files selected for processing (20)
cmake/ExternalDependencies.cmakeinclude/mlir/Conversion/CMakeLists.txtinclude/mlir/Conversion/CatalystQuantumToMQTOpt/CMakeLists.txtinclude/mlir/Conversion/CatalystQuantumToQC/CMakeLists.txtinclude/mlir/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.hinclude/mlir/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.tdinclude/mlir/Conversion/MQTOptToCatalystQuantum/CMakeLists.txtinclude/mlir/Conversion/QCToCatalystQuantum/CMakeLists.txtinclude/mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.hinclude/mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.tdlib/CMakeLists.txtlib/Conversion/CMakeLists.txtlib/Conversion/CatalystQuantumToMQTOpt/CatalystQuantumToMQTOpt.cpplib/Conversion/CatalystQuantumToQC/CMakeLists.txtlib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpplib/Conversion/MQTOptToCatalystQuantum/MQTOptToCatalystQuantum.cpplib/Conversion/QCToCatalystQuantum/CMakeLists.txtlib/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.cpplib/mqt-plugin.cpptest/test_plugin.py
💤 Files with no reviewable changes (4)
- include/mlir/Conversion/MQTOptToCatalystQuantum/CMakeLists.txt
- include/mlir/Conversion/CatalystQuantumToMQTOpt/CMakeLists.txt
- lib/Conversion/CatalystQuantumToMQTOpt/CatalystQuantumToMQTOpt.cpp
- lib/Conversion/MQTOptToCatalystQuantum/MQTOptToCatalystQuantum.cpp
Comment on lines
+18
to
21
| set(MQT_CORE_REV "test_Memref_explicit" # fix clang-tidy warnings 7d1d7fb | ||
| CACHE STRING "MQT Core identifier (tag, branch or commit hash)") | ||
| set(MQT_CORE_REPO_OWNER "munich-quantum-toolkit" | ||
| set(MQT_CORE_REPO_OWNER "NiklasKostrzewa" | ||
| CACHE STRING "MQT Core repository owner (change when using a fork)") |
Contributor
There was a problem hiding this comment.
Personal fork and branch reference must be reverted before merging.
The external dependency now points to a personal fork (NiklasKostrzewa/core) with a branch name (test_Memref_explicit) instead of the official munich-quantum-toolkit/core repository with a pinned commit hash.
Issues:
- Non-reproducible builds: Using a branch name instead of a commit hash means the build target can change over time.
- Personal fork dependency: Production code should not depend on personal forks.
- Inconsistent comment: The inline comment references commit
7d1d7fbbut the REV is set to a branch name.
This is acceptable for a draft PR during development, but ensure these values are reverted to the official repository with a stable commit hash before merging.
Suggested values for merge readiness
-set(MQT_CORE_REV "test_Memref_explicit" # fix clang-tidy warnings 7d1d7fb
+set(MQT_CORE_REV "<pinned-commit-hash-from-main>"
CACHE STRING "MQT Core identifier (tag, branch or commit hash)")
-set(MQT_CORE_REPO_OWNER "NiklasKostrzewa"
+set(MQT_CORE_REPO_OWNER "munich-quantum-toolkit"
CACHE STRING "MQT Core repository owner (change when using a fork)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmake/ExternalDependencies.cmake` around lines 18 - 21, Replace the temporary
fork/branch references in the external dependency variables: set
MQT_CORE_REPO_OWNER back to the official owner ("munich-quantum-toolkit") and
change MQT_CORE_REV from the branch name ("test_Memref_explicit") to a stable
pinned commit hash (replace the branch with the specific commit hash you intend
to use), and update/remove the inconsistent inline comment referencing 7d1d7fb
so the REV and comment match; these changes are located where MQT_CORE_REV and
MQT_CORE_REPO_OWNER are defined.
Comment on lines
+11
to
+22
| def QCToCatalystQuantum : Pass<"qc-to-catalystquantum"> { | ||
| let summary = "Convert MQT's `QC` to Catalyst's `Quantum` dialect."; | ||
|
|
||
| let description = [{ | ||
| This pass converts MQT's `MQTOpt` to Catalyst's `Quantum` dialect. | ||
| This pass converts MQT's `QC` to Catalyst's `Quantum` dialect. | ||
| }]; | ||
| let dependentDialects = [ | ||
| "::catalyst::quantum::QuantumDialect", | ||
| "::mlir::arith::ArithDialect", | ||
| "::mlir::func::FuncDialect", | ||
| "::mlir::memref::MemRefDialect", | ||
| "::mqt::ir::opt::MQTOptDialect" | ||
| "::mlir::qc::QCDialect" |
Contributor
There was a problem hiding this comment.
Pass defined but registration is commented out.
The qc-to-catalystquantum pass is correctly defined here, but registerQCToCatalystQuantumPasses() is commented out in lib/mqt-plugin.cpp (line 47). This means:
- The pass will not be available at runtime.
- Tests using
@apply_pass("mqt.qc-to-catalystquantum")or theDEFAULT_PIPELINE(which includes"qc-to-catalystquantum") will fail.
Per the PR description, this is intentional for the draft state since back-conversion is incomplete. Ensure the registration is uncommented when the implementation is ready.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.td` around
lines 11 - 22, The QCToCatalystQuantum pass (QCToCatalystQuantum) is defined but
not registered because registerQCToCatalystQuantumPasses() is commented out in
lib/mqt-plugin.cpp, so runtime lookups (e.g.
`@apply_pass`("mqt.qc-to-catalystquantum") and DEFAULT_PIPELINE referencing
"qc-to-catalystquantum") fail; to fix, uncomment and enable the call to
registerQCToCatalystQuantumPasses() in lib/mqt-plugin.cpp (or add conditional
registration gated by a readiness flag) so that the pass is registered with MLIR
when the implementation/back-conversion is complete and tests can find
mqt.qc-to-catalystquantum.
Comment on lines
+25
to
+26
| add_llvm_library(${TARGET_NAME} MODULE mqt-plugin.cpp LINK_LIBS CatalystQuantumToQC) | ||
| # QCToCatalystQuantum) |
Contributor
There was a problem hiding this comment.
Link QCToCatalystQuantum into the plugin module.
The plugin now expects both QC passes, but this target only links CatalystQuantumToQC. QCToCatalystQuantum is a separate library, so the back-conversion pass will not be exported from the plugin unless it is linked here.
🔧 Suggested linkage fix
-add_llvm_library(${TARGET_NAME} MODULE mqt-plugin.cpp LINK_LIBS CatalystQuantumToQC)
-# QCToCatalystQuantum)
+add_llvm_library(
+ ${TARGET_NAME}
+ MODULE
+ mqt-plugin.cpp
+ LINK_LIBS
+ CatalystQuantumToQC
+ QCToCatalystQuantum)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/CMakeLists.txt` around lines 25 - 26, The plugin module currently links
only CatalystQuantumToQC via the add_llvm_library(${TARGET_NAME} MODULE
mqt-plugin.cpp LINK_LIBS CatalystQuantumToQC) invocation; update that
add_llvm_library call to also link QCToCatalystQuantum so the back-conversion
pass is exported by the plugin. Locate the add_llvm_library line (with
TARGET_NAME and mqt-plugin.cpp) and add QCToCatalystQuantum to the LINK_LIBS
list alongside CatalystQuantumToQC.
| // "\n"; | ||
|
|
||
| // Create the new operation | ||
| Operation* qcOp = nullptr; |
Contributor
There was a problem hiding this comment.
Remove the dead qcOp temporary.
The rewrite only relies on the side effects of rewriter.create; the stored pointer is never read, and CI is already warning about -Wunused-but-set-variable here.
🧰 Tools
🪛 GitHub Actions: CI
[warning] 319-319: qcOp: variable defined but not used. (Wunused-but-set-variable)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
[warning] 319-319: lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp:319:16 [clang-diagnostic-unused-but-set-variable]
variable 'qcOp' set but not used
[warning] 319-319: lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp:319:5 [misc-const-correctness]
pointee of variable 'qcOp' of type 'Operation *' can be declared 'const'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp` at line 319, The
local variable qcOp is unused and should be removed: delete the declaration
"Operation* qcOp = nullptr;" and replace any assignment like "qcOp =
rewriter.create(...);" with just "rewriter.create(...);", relying on the side
effects of rewriter.create; ensure no other code references qcOp (search for
qcOp) and compile to verify the -Wunused-but-set-variable warning is resolved.
Comment on lines
+446
to
+515
| } else if (gateName == "RX") { | ||
| if (inCtrlQubits.empty()) { | ||
| qcOp = CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP(RX); | ||
| } else { | ||
| qcOp = rewriter.create<qc::CtrlOp>(op.getLoc(), inCtrlQubits, [&]() { | ||
| CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP(RX); | ||
| }); | ||
| } | ||
| } else if (gateName == "RY") { | ||
| if (inCtrlQubits.empty()) { | ||
| qcOp = CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP(RY); | ||
| } else { | ||
| qcOp = rewriter.create<qc::CtrlOp>(op.getLoc(), inCtrlQubits, [&]() { | ||
| CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP(RY); | ||
| }); | ||
| } | ||
| } else if (gateName == "RZ") { | ||
| if (inCtrlQubits.empty()) { | ||
| qcOp = CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP(RZ); | ||
| } else { | ||
| qcOp = rewriter.create<qc::CtrlOp>(op.getLoc(), inCtrlQubits, [&]() { | ||
| CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP(RZ); | ||
| }); | ||
| } | ||
| } else if (gateName == "PhaseShift") { | ||
| if (inCtrlQubits.empty()) { | ||
| qcOp = CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP(P); | ||
| } else { | ||
| qcOp = rewriter.create<qc::CtrlOp>(op.getLoc(), inCtrlQubits, [&]() { | ||
| CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP(P); | ||
| }); | ||
| } | ||
| } else if (gateName == "CRX") { | ||
| qcOp = rewriter.create<qc::CtrlOp>( | ||
| op.getLoc(), inQubits.take_front(1), [&]() { | ||
| rewriter.create<qc::RXOp>(op.getLoc(), inQubits[1], | ||
| paramsValues[0]); | ||
| }); | ||
| } else if (gateName == "CRY") { | ||
| qcOp = rewriter.create<qc::CtrlOp>( | ||
| op.getLoc(), inQubits.take_front(1), [&]() { | ||
| rewriter.create<qc::RYOp>(op.getLoc(), inQubits[1], | ||
| paramsValues[0]); | ||
| }); | ||
| } else if (gateName == "CRZ") { | ||
| qcOp = rewriter.create<qc::CtrlOp>( | ||
| op.getLoc(), inQubits.take_front(1), [&]() { | ||
| rewriter.create<qc::RZOp>(op.getLoc(), inQubits[1], | ||
| paramsValues[0]); | ||
| }); | ||
| } else if (gateName == "ControlledPhaseShift") { | ||
| qcOp = rewriter.create<qc::CtrlOp>( | ||
| op.getLoc(), inQubits.take_front(1), [&]() { | ||
| rewriter.create<qc::POp>(op.getLoc(), inQubits[1], paramsValues[0]); | ||
| }); | ||
| } else if (gateName == "IsingXY") { | ||
| // PennyLane IsingXY has 1 parameter (phi), OpenQASM XXPlusYY needs 2 | ||
| // (theta, beta) Relationship: IsingXY(phi) = XXPlusYY(phi, pi) | ||
| // Add pi as second parameter (since we add it during compilation) | ||
|
|
||
| qcOp = | ||
| rewriter.create<qc::XXPlusYYOp>(op.getLoc(), inQubits[0], inQubits[1], | ||
| paramsValues[0], std::numbers::pi); | ||
| } else if (gateName == "IsingXX") { | ||
| qcOp = CREATE_TWO_TARGET_ONE_PARAMETER_GATE_OP(RXX); | ||
| } else if (gateName == "IsingYY") { | ||
| qcOp = CREATE_TWO_TARGET_ONE_PARAMETER_GATE_OP(RYY); | ||
| } else if (gateName == "IsingZZ") { | ||
| qcOp = CREATE_TWO_TARGET_ONE_PARAMETER_GATE_OP(RZZ); | ||
| } else if (gateName == "CNOT") { |
Contributor
There was a problem hiding this comment.
adj is ignored for non-self-inverse parameterized gates.
Only the S/T/SX branches consult op.getAdjoint(). RX/RY/RZ, PhaseShift, CRX/CRY/CRZ, ControlledPhaseShift, and the Ising* branches all lower an adjointed quantum.custom to the forward QC op, which changes circuit semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp` around lines 446
- 515, The adjoint flag (op.getAdjoint()) is ignored for parameterized gates
(RX/RY/RZ, PhaseShift/P, CRX/CRY/CRZ, ControlledPhaseShift/POp,
IsingXY/IsingXX/IsingYY/IsingZZ and their lowering macros CREATE_* like
CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP and
CREATE_TWO_TARGET_ONE_PARAMETER_GATE_OP), so update the lowering to check
op.getAdjoint() and emit the adjointed operation: for single-parameter rotations
and phase gates negate the parameter (use -paramsValues[0]) when adjoint is
true; for controlled variants negate the same parameter passed into the inner
rewriter.create<qc::RXOp/RYOp/RZOp/POp>; for IsingXY negate the first parameter
(keep the added pi as-is) when adjoint is true; and for
CREATE_TWO_TARGET_ONE_PARAMETER_GATE_OP usages (RXX/RYY/RZZ) negate the
parameter when adjoint is set so the lowered qcOp preserves semantics. Ensure
you reference and modify the branches that inspect gateName (e.g., the RX/RY/RZ,
PhaseShift, CRX/CRY/CRZ, ControlledPhaseShift, Ising* branches) and use
op.getAdjoint() to decide whether to pass paramsValues[0] or -paramsValues[0].
Comment on lines
+39
to
+329
| #define GEN_PASS_DEF_QCTOCATALYSTQUANTUM | ||
| #include "mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.h.inc" | ||
|
|
||
| using namespace mlir; | ||
| using namespace mlir::arith; | ||
|
|
||
| // Helper functions to reduce code duplication | ||
| namespace { | ||
|
|
||
| /// Helper struct to hold control qubit information | ||
| struct ControlInfo { | ||
| SmallVector<Value> ctrlQubits; | ||
| SmallVector<Value> ctrlValues; | ||
|
|
||
| ControlInfo() noexcept = default; | ||
| }; | ||
|
|
||
| /// Extract and concatenate control qubits and create corresponding control | ||
| /// values | ||
| ControlInfo extractControlInfo(ValueRange posCtrlQubits, | ||
| ValueRange negCtrlQubits, | ||
| ConversionPatternRewriter& rewriter, | ||
| Location loc) { | ||
| ControlInfo info; | ||
|
|
||
| // Concatenate controls: [pos..., neg...] (preserve this order consistently) | ||
| info.ctrlQubits.reserve(posCtrlQubits.size() + negCtrlQubits.size()); | ||
| info.ctrlQubits.append(posCtrlQubits.begin(), posCtrlQubits.end()); | ||
| info.ctrlQubits.append(negCtrlQubits.begin(), negCtrlQubits.end()); | ||
|
|
||
| if (info.ctrlQubits.empty()) { | ||
| return info; | ||
| } | ||
|
|
||
| // Create control values: 1 for positive controls, 0 for negative controls | ||
| const Value one = | ||
| rewriter.create<mlir::arith::ConstantIntOp>(loc, /*value=*/1, | ||
| /*width=*/1); | ||
| const Value zero = | ||
| rewriter.create<mlir::arith::ConstantIntOp>(loc, /*value=*/0, | ||
| /*width=*/1); | ||
|
|
||
| info.ctrlValues.reserve(info.ctrlQubits.size()); | ||
| info.ctrlValues.append(posCtrlQubits.size(), one); // +controls => 1 | ||
| info.ctrlValues.append(negCtrlQubits.size(), zero); // -controls => 0 | ||
|
|
||
| return info; | ||
| } | ||
|
|
||
| /// Helper function to extract operands and control info - for more complex | ||
| /// cases | ||
| template <typename OpAdaptor> struct ExtractedOperands { | ||
| ValueRange inQubits; | ||
| ControlInfo ctrlInfo; | ||
| }; | ||
|
|
||
| template <typename OpAdaptor> | ||
| ExtractedOperands<OpAdaptor> | ||
| extractOperands(OpAdaptor adaptor, ConversionPatternRewriter& rewriter, | ||
| Location loc) { | ||
| const ValueRange inQubits = adaptor.getInQubits(); | ||
| const ValueRange posCtrlQubits = adaptor.getPosCtrlInQubits(); | ||
| const ValueRange negCtrlQubits = adaptor.getNegCtrlInQubits(); | ||
|
|
||
| const ControlInfo ctrlInfo = | ||
| extractControlInfo(posCtrlQubits, negCtrlQubits, rewriter, loc); | ||
|
|
||
| return {inQubits, ctrlInfo}; | ||
| } | ||
|
|
||
| } // anonymous namespace | ||
|
|
||
| class QCToCatalystQuantumTypeConverter final : public TypeConverter { | ||
| public: | ||
| explicit QCToCatalystQuantumTypeConverter(MLIRContext* ctx) { | ||
| // Identity conversion for types that don't need transformation | ||
| addConversion([](const Type type) { return type; }); | ||
|
|
||
| // Convert MemRef of QC QubitType to Catalyst QuregType | ||
| // Also handles memrefs where the element type was already converted | ||
| addConversion([ctx](MemRefType memrefType) -> Type { | ||
| auto elemType = memrefType.getElementType(); | ||
| if (isa<qc::QubitType>(elemType) || | ||
| isa<catalyst::quantum::QubitType>(elemType)) { | ||
| return catalyst::quantum::QuregType::get(ctx); | ||
| } | ||
| return memrefType; | ||
| }); | ||
|
|
||
| // Convert QC QubitType to Catalyst QubitType | ||
| addConversion([ctx](qc::QubitType /*type*/) -> Type { | ||
| return catalyst::quantum::QubitType::get(ctx); | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| struct ConvertQCAlloc final : OpConversionPattern<memref::AllocOp> { | ||
| using OpConversionPattern::OpConversionPattern; | ||
|
|
||
| LogicalResult | ||
| matchAndRewrite(memref::AllocOp op, OpAdaptor /*adaptor*/, | ||
| ConversionPatternRewriter& rewriter) const override { | ||
| // Only convert memrefs of qubit type | ||
| auto memrefType = dyn_cast<BaseMemRefType>(op.getType()); | ||
| auto elemType = memrefType ? memrefType.getElementType() : Type(); | ||
| if (!memrefType || !(isa<qc::QubitType>(elemType) || | ||
| isa<catalyst::quantum::QubitType>(elemType))) { | ||
| return failure(); | ||
| } | ||
|
|
||
| // Only handle ranked memrefs | ||
| auto rankedMemrefType = dyn_cast<MemRefType>(memrefType); | ||
| if (!rankedMemrefType) { | ||
| return failure(); | ||
| } | ||
|
|
||
| // Prepare the result type(s) | ||
| const auto resultType = | ||
| catalyst::quantum::QuregType::get(rewriter.getContext()); | ||
|
|
||
| // Get the size from memref type or dynamic operands | ||
| Value size = nullptr; | ||
| mlir::IntegerAttr nqubitsAttr = nullptr; | ||
|
|
||
| // Check if this is a statically shaped memref | ||
| if (rankedMemrefType.hasStaticShape() && | ||
| rankedMemrefType.getNumElements() >= 0) { | ||
| // For static memref: use attribute (no operand) | ||
| nqubitsAttr = | ||
| rewriter.getI64IntegerAttr(rankedMemrefType.getNumElements()); | ||
| } else { | ||
| // For dynamic memref: check if the size is actually a constant | ||
| auto dynamicOperands = op.getDynamicSizes(); | ||
| const Value dynamicSize = | ||
| dynamicOperands.empty() ? nullptr : dynamicOperands[0]; | ||
|
|
||
| if (dynamicSize) { | ||
| // Try to recover static size from constant operand | ||
| if (auto constOp = | ||
| dynamicSize.getDefiningOp<arith::ConstantIndexOp>()) { | ||
| // The size is a constant index, use it as an attribute instead | ||
| nqubitsAttr = rewriter.getI64IntegerAttr(constOp.value()); | ||
| } else if (auto constOp = | ||
| dynamicSize.getDefiningOp<arith::ConstantIntOp>()) { | ||
| // The size is a constant int, use it as an attribute instead | ||
| nqubitsAttr = rewriter.getI64IntegerAttr(constOp.value()); | ||
| } else { | ||
| // Truly dynamic size - use operand | ||
| size = dynamicSize; | ||
| // quantum.alloc expects i64, but memref size is index type | ||
| if (mlir::isa<IndexType>(size.getType())) { | ||
| size = rewriter.create<arith::IndexCastOp>( | ||
| op.getLoc(), rewriter.getI64Type(), size); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Replace with quantum alloc operation | ||
| rewriter.replaceOpWithNewOp<catalyst::quantum::AllocOp>(op, resultType, | ||
| size, nqubitsAttr); | ||
|
|
||
| return success(); | ||
| } | ||
| }; | ||
|
|
||
| struct ConvertQCDealloc final : OpConversionPattern<memref::DeallocOp> { | ||
| using OpConversionPattern::OpConversionPattern; | ||
|
|
||
| LogicalResult | ||
| matchAndRewrite(memref::DeallocOp op, OpAdaptor adaptor, | ||
| ConversionPatternRewriter& rewriter) const override { | ||
| // Only convert memrefs of qubit type | ||
| auto memrefType = dyn_cast<BaseMemRefType>(op.getMemref().getType()); | ||
| auto elemType = memrefType ? memrefType.getElementType() : Type(); | ||
| if (!memrefType || !(isa<qc::QubitType>(elemType) || | ||
| isa<catalyst::quantum::QubitType>(elemType))) { | ||
| return failure(); | ||
| } | ||
|
|
||
| // Create the new operation | ||
| const auto catalystOp = rewriter.create<catalyst::quantum::DeallocOp>( | ||
| op.getLoc(), TypeRange({}), adaptor.getMemref()); | ||
|
|
||
| // Replace the original with the new operation | ||
| rewriter.replaceOp(op, catalystOp); | ||
| return success(); | ||
| } | ||
| }; | ||
|
|
||
| struct ConvertQCMeasure final : OpConversionPattern<qc::MeasureOp> { | ||
| using OpConversionPattern::OpConversionPattern; | ||
|
|
||
| LogicalResult | ||
| matchAndRewrite(qc::MeasureOp op, OpAdaptor adaptor, | ||
| ConversionPatternRewriter& rewriter) const override { | ||
|
|
||
| // Extract operand(s) | ||
| auto inQubit = adaptor.getQubit(); | ||
|
|
||
| // Prepare the result type(s) | ||
| auto qubitType = catalyst::quantum::QubitType::get(rewriter.getContext()); | ||
| auto bitType = rewriter.getI1Type(); | ||
|
|
||
| // Create the new operation | ||
| const auto catalystOp = rewriter.create<catalyst::quantum::MeasureOp>( | ||
| op.getLoc(), bitType, qubitType, inQubit, | ||
| /*optional::mlir::IntegerAttr postselect=*/nullptr); | ||
|
|
||
| // Replace all uses of both results and then erase the operation | ||
| const auto catalystMeasure = catalystOp->getResult(0); | ||
| const auto catalystQubit = catalystOp->getResult(1); | ||
| rewriter.replaceOp(op, ValueRange{catalystQubit, catalystMeasure}); | ||
| return success(); | ||
| } | ||
| }; | ||
|
|
||
| struct ConvertQCLoad final : OpConversionPattern<memref::LoadOp> { | ||
| using OpConversionPattern::OpConversionPattern; | ||
|
|
||
| LogicalResult | ||
| matchAndRewrite(memref::LoadOp op, OpAdaptor adaptor, | ||
| ConversionPatternRewriter& rewriter) const override { | ||
| // Only convert loads of qubit type | ||
| if (!(isa<qc::QubitType>(op.getType()) || | ||
| isa<catalyst::quantum::QubitType>(op.getType()))) { | ||
| return failure(); | ||
| } | ||
|
|
||
| // Prepare the result type(s) | ||
| auto resultType = catalyst::quantum::QubitType::get(rewriter.getContext()); | ||
|
|
||
| // Get index (assuming single index for 1D memref) | ||
| auto indices = adaptor.getIndices(); | ||
| Value index = indices.empty() ? nullptr : indices[0]; | ||
|
|
||
| // Convert index type to i64 if needed | ||
| if (index && mlir::isa<IndexType>(index.getType())) { | ||
| index = rewriter.create<arith::IndexCastOp>(op.getLoc(), | ||
| rewriter.getI64Type(), index); | ||
| } | ||
|
|
||
| // Create the new operation | ||
| auto catalystOp = rewriter.create<catalyst::quantum::ExtractOp>( | ||
| op.getLoc(), resultType, adaptor.getMemref(), index, nullptr); | ||
|
|
||
| // Replace the load operation with the extracted qubit | ||
| rewriter.replaceOp(op, catalystOp.getResult()); | ||
| return success(); | ||
| } | ||
| }; | ||
|
|
||
| struct ConvertQCStore final : OpConversionPattern<memref::StoreOp> { | ||
| using OpConversionPattern::OpConversionPattern; | ||
|
|
||
| LogicalResult | ||
| matchAndRewrite(memref::StoreOp op, OpAdaptor adaptor, | ||
| ConversionPatternRewriter& rewriter) const override { | ||
| // Only convert stores to memrefs with qubit element type | ||
| auto memrefType = dyn_cast<BaseMemRefType>(op.getMemRef().getType()); | ||
| auto elemType = memrefType ? memrefType.getElementType() : Type(); | ||
| if (!memrefType || !(isa<qc::QubitType>(elemType) || | ||
| isa<catalyst::quantum::QubitType>(elemType))) { | ||
| return failure(); | ||
| } | ||
|
|
||
| // Get indices (assuming single index for 1D memref) | ||
| auto indices = adaptor.getIndices(); | ||
| Value index = indices.empty() ? nullptr : indices[0]; | ||
|
|
||
| // Convert index type to i64 if needed | ||
| if (index && mlir::isa<IndexType>(index.getType())) { | ||
| index = rewriter.create<arith::IndexCastOp>(op.getLoc(), | ||
| rewriter.getI64Type(), index); | ||
| } | ||
|
|
||
| // Prepare the result type(s) | ||
| auto resultType = catalyst::quantum::QuregType::get(rewriter.getContext()); | ||
|
|
||
| // Create the new operation | ||
| rewriter.create<catalyst::quantum::InsertOp>(op.getLoc(), resultType, | ||
| adaptor.getMemref(), index, | ||
| nullptr, adaptor.getValue()); | ||
|
|
||
| // Erase the original store operation (store has no results to replace) | ||
| rewriter.eraseOp(op); | ||
| return success(); | ||
| } | ||
| }; | ||
|
|
||
| } // namespace mqt::ir::conversions |
Contributor
There was a problem hiding this comment.
qc-to-catalystquantum is declared, but no pass is implemented.
This file stops after helper/type-converter/pattern declarations. There is no QCToCatalystQuantum::runOnOperation(), no legality setup, and no registration of the rewrite patterns below, so the pass advertised by the CMake/test changes cannot execute. The actual QC gate/control ops exercised in test/test_plugin.py are also not converted anywhere in this file yet.
🧰 Tools
🪛 Cppcheck (2.20.0)
[information] 40-40: Include file
(missingInclude)
[style] 97-97: The function 'extractOperands' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.cpp` around lines 39 -
329, The file defines conversions (QCToCatalystQuantumTypeConverter,
ConvertQCAlloc, ConvertQCDealloc, ConvertQCMeasure, ConvertQCLoad,
ConvertQCStore, and helpers like extractControlInfo/extractOperands) but never
implements or registers the pass; add a pass implementation using
GEN_PASS_DEF_QCTOCATALYSTQUANTUM that defines
QCToCatalystQuantum::runOnOperation(), builds a ConversionTarget, instantiates
QCToCatalystQuantumTypeConverter, populates a RewritePatternSet with the
provided OpConversionPattern classes (ConvertQCAlloc, ConvertQCDealloc,
ConvertQCMeasure, ConvertQCLoad, ConvertQCStore, etc.), marks legal/illegal ops
appropriately, and call applyFullConversion/applyPartialConversion; finally
register the pass so the declared pass actually runs in tests.
Comment on lines
+46
to
+47
| mqt::ir::conversions::registerCatalystQuantumToQCPasses(); | ||
| // mqt::ir::conversions::registerQCToCatalystQuantumPasses(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Back-conversion pass registration intentionally disabled.
The registerQCToCatalystQuantumPasses() call is commented out, which is consistent with the PR description stating that back-conversion is incomplete. The test suite's DEFAULT_PIPELINE includes "qc-to-catalystquantum", so tests exercising the full roundtrip will fail until this is uncommented.
Consider adding a TODO or FIXME comment to make this more discoverable:
mqt::ir::conversions::registerCatalystQuantumToQCPasses();
- // mqt::ir::conversions::registerQCToCatalystQuantumPasses();
+ // TODO(NiklasKostrzewa): Uncomment when back-conversion is complete
+ // mqt::ir::conversions::registerQCToCatalystQuantumPasses();📝 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
| mqt::ir::conversions::registerCatalystQuantumToQCPasses(); | |
| // mqt::ir::conversions::registerQCToCatalystQuantumPasses(); | |
| mqt::ir::conversions::registerCatalystQuantumToQCPasses(); | |
| // TODO(NiklasKostrzewa): Uncomment when back-conversion is complete | |
| // mqt::ir::conversions::registerQCToCatalystQuantumPasses(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/mqt-plugin.cpp` around lines 46 - 47, Back-conversion registration
(mqt::ir::conversions::registerQCToCatalystQuantumPasses) is commented out which
breaks tests that expect the full roundtrip (DEFAULT_PIPELINE includes
"qc-to-catalystquantum"); add a clear TODO/FIXME above the commented line that
names registerQCToCatalystQuantumPasses and references DEFAULT_PIPELINE, and
either (a) briefly describe what remains to be implemented for back-conversion
or (b) add a short conditional/feature-flag note explaining why it is disabled
so future readers/tests can find and re-enable it.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (10)
cmake/ExternalDependencies.cmake (1)
18-21:⚠️ Potential issue | 🟠 MajorRevert default
mqt-coresource to official owner + pinned commit SHA before merge.Line 18 and Line 20 still default to a personal fork and mutable branch. Since this file is executed at configure time, these defaults make builds non-reproducible and reduce supply-chain traceability. Keep fork/branch only as an opt-in local override, not as default values.
Suggested fix
-set(MQT_CORE_REV "test_Memref_explicit" # fix clang-tidy warnings 7d1d7fb +set(MQT_CORE_REV "<pinned-40-char-commit-sha>" CACHE STRING "MQT Core identifier (tag, branch or commit hash)") -set(MQT_CORE_REPO_OWNER "NiklasKostrzewa" +set(MQT_CORE_REPO_OWNER "munich-quantum-toolkit" CACHE STRING "MQT Core repository owner (change when using a fork)")#!/bin/bash set -euo pipefail FILE="$(fd -p ExternalDependencies.cmake | head -n1)" echo "Checking ${FILE}" nl -ba "${FILE}" | sed -n '14,24p' python - <<'PY' import re from pathlib import Path p = next(Path('.').rglob('ExternalDependencies.cmake')) text = p.read_text() owner_m = re.search(r'set\(MQT_CORE_REPO_OWNER\s+"([^"]+)"', text) rev_m = re.search(r'set\(MQT_CORE_REV\s+"([^"]+)"', text) owner = owner_m.group(1) if owner_m else None rev = rev_m.group(1) if rev_m else None print(f"MQT_CORE_REPO_OWNER={owner}") print(f"MQT_CORE_REV={rev}") print("owner_is_official:", owner == "munich-quantum-toolkit") print("rev_is_pinned_sha:", bool(re.fullmatch(r"[0-9a-f]{40}", rev or ""))) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/ExternalDependencies.cmake` around lines 18 - 21, The defaults for MQT_CORE_REPO_OWNER and MQT_CORE_REV must be reverted to the official upstream and a pinned commit SHA: update the variable MQT_CORE_REPO_OWNER to "munich-quantum-toolkit" and replace the mutable branch value in MQT_CORE_REV with a 40-character commit hash (hex) for a reproducible pinned revision; leave any fork/branch values only as optional overrides or documented examples in comments, not as the CACHE default, and ensure MQT_CORE_REV matches the full SHA pattern [0-9a-f]{40}.lib/CMakeLists.txt (1)
25-26:⚠️ Potential issue | 🟠 MajorLink
QCToCatalystQuantuminto the plugin target before re-enabling the pass.
QCToCatalystQuantumis a separate MLIR library, but the plugin currently links onlyCatalystQuantumToQC. OnceregisterQCToCatalystQuantumPasses()is turned back on, the reverse conversion pass still will not be available from the plugin unless this target is linked here.🔧 Suggested linkage fix
-add_llvm_library(${TARGET_NAME} MODULE mqt-plugin.cpp LINK_LIBS CatalystQuantumToQC) -# QCToCatalystQuantum) +add_llvm_library( + ${TARGET_NAME} + MODULE + mqt-plugin.cpp + LINK_LIBS + CatalystQuantumToQC + QCToCatalystQuantum)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/CMakeLists.txt` around lines 25 - 26, The plugin target created by add_llvm_library(${TARGET_NAME} MODULE mqt-plugin.cpp LINK_LIBS CatalystQuantumToQC currently omits QCToCatalystQuantum, so when registerQCToCatalystQuantumPasses() is re-enabled the reverse conversion won't be available; fix this by adding QCToCatalystQuantum to the LINK_LIBS for the target (i.e., ensure LINK_LIBS includes both CatalystQuantumToQC and QCToCatalystQuantum for the ${TARGET_NAME} plugin built from mqt-plugin.cpp).lib/mqt-plugin.cpp (2)
11-13:⚠️ Potential issue | 🔴 CriticalBuild failure due to missing generated header.
The CI reports:
fatal error: mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.h.inc: No such file or directory. TheQCToCatalystQuantum.hinclude depends on a TableGen-generated.h.incfile that isn't being produced correctly.Verify that
include/mlir/Conversion/QCToCatalystQuantum/CMakeLists.txtproperly invokesmlir_tablegenand that the generated include directory is added to the target's include paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mqt-plugin.cpp` around lines 11 - 13, The build fails because the TableGen-generated header QCToCatalystQuantum.h.inc isn't being produced/visible; update include/mlir/Conversion/QCToCatalystQuantum/CMakeLists.txt to run mlir_tablegen to generate QCToCatalystQuantum.h.inc (invoking the correct TABLEGEN or MLIR_TABLEGEN rule for the QCToCatalystQuantum tablegen file) and add the generated include directory to the consuming target via target_include_directories or target_include_directories(... SYSTEM BEFORE) so that the target building lib/mqt-plugin.cpp can find "mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.h" which expects the .h.inc; ensure the generated file name matches QCToCatalystQuantum.h.inc and that the target that builds mqt-plugin depends on the tablegen output target so generation happens before compilation.
46-47: 🧹 Nitpick | 🔵 TrivialAdd a TODO comment for the disabled back-conversion pass.
The
registerQCToCatalystQuantumPasses()call is commented out, which is consistent with the PR stating back-conversion is incomplete. Adding a TODO helps discoverability:mqt::ir::conversions::registerCatalystQuantumToQCPasses(); - // mqt::ir::conversions::registerQCToCatalystQuantumPasses(); + // TODO(NiklasKostrzewa): Uncomment when back-conversion is complete + // mqt::ir::conversions::registerQCToCatalystQuantumPasses();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mqt-plugin.cpp` around lines 46 - 47, Add a short TODO comment next to the commented-out registerQCToCatalystQuantumPasses() call in lib/mqt-plugin.cpp to indicate the back-conversion is incomplete and needs re-enabling or completion later; reference the symbol registerQCToCatalystQuantumPasses() and mention “back-conversion incomplete” and optionally link to the PR or issue number if available so future maintainers can discover why it is disabled.lib/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.cpp (2)
315-326:⚠️ Potential issue | 🔴 Critical
InsertOpresult is dropped — writes are lost in SSA semantics.
catalyst::quantum::InsertOpreturns a new!quantum.regvalue representing the updated register. Here, the result is discarded, so the write is semantically lost in Catalyst's SSA model. The result should be threaded to subsequent users or the value mapping should be updated.🐛 Proposed fix
- // Create the new operation - rewriter.create<catalyst::quantum::InsertOp>(op.getLoc(), resultType, - adaptor.getMemref(), index, - nullptr, adaptor.getValue()); - - // Erase the original store operation (store has no results to replace) - rewriter.eraseOp(op); + // Create the new operation and capture result + auto insertOp = rewriter.create<catalyst::quantum::InsertOp>( + op.getLoc(), resultType, adaptor.getMemref(), index, nullptr, + adaptor.getValue()); + + // Replace all uses of the original memref with the new register + rewriter.replaceAllUsesWith(adaptor.getMemref(), insertOp.getResult()); + rewriter.eraseOp(op);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.cpp` around lines 315 - 326, InsertOp's result is being dropped so the updated !quantum.reg is lost; change the creation to capture the returned value from rewriter.create<catalyst::quantum::InsertOp> (e.g., auto newInsert = rewriter.create<...>(...)) and then thread that result into the SSA flow by replacing the original op's result or updating the conversion mapping (use rewriter.replaceOp(op, newInsert.getResult(0)) or update your ValueMapping/rewriter mapping so later uses see newInsert.getResult(0)); ensure you remove the old op after replacing so no users remain referencing the discarded value.
37-41:⚠️ Potential issue | 🔴 CriticalPass implementation is incomplete — no
runOnOperation()defined.This file declares
GEN_PASS_DEF_QCTOCATALYSTQUANTUMbut never implements the pass'srunOnOperation()method. The patterns (ConvertQCAlloc, ConvertQCDealloc, etc.) are defined but never registered with aConversionTargetor applied viaapplyPartialConversion. The pass cannot execute in its current state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.cpp` around lines 37 - 41, Implement the pass's runOnOperation for the pass declared by GEN_PASS_DEF_QCTOCATALYSTQUANTUM: create a new instance of ConversionTarget, register legality for target dialects, populate a RewritePatternSet with the existing patterns (ConvertQCAlloc, ConvertQCDealloc, ConvertQCExtract, ConvertQCInject, etc.), and then call applyPartialConversion(getOperation(), target, patterns). Ensure the pass class (the one generated by GEN_PASS_DEF_QCTOCATALYSTQUANTUM) overrides runOnOperation(), constructs the target and patterns, marks ops as legal/illegal as appropriate, and handles failure by signalling signalPassFailure() when applyPartialConversion fails.lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp (2)
446-515:⚠️ Potential issue | 🔴 CriticalAdjoint flag is ignored for parameterized rotation gates.
Only
S,T, andSXgates checkop.getAdjoint(). ForRX,RY,RZ,PhaseShift,CRX,CRY,CRZ,ControlledPhaseShift, andIsing*gates, the adjoint flag is not consulted. For parameterized rotations, adjoint typically requires negating the rotation angle.For example,
RX†(θ) = RX(-θ). Without this, adjointed gates convert incorrectly.🐛 Example fix for RX
} else if (gateName == "RX") { + Value angle = paramsValues[0]; + if (op.getAdjoint()) { + angle = rewriter.create<arith::NegFOp>(op.getLoc(), angle); + } if (inCtrlQubits.empty()) { - qcOp = CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP(RX); + rewriter.create<qc::RXOp>(op.getLoc(), inQubits[0], angle); } else { qcOp = rewriter.create<qc::CtrlOp>(op.getLoc(), inCtrlQubits, [&]() { - CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP(RX); + rewriter.create<qc::RXOp>(op.getLoc(), inQubits[0], angle); }); } }Apply similar changes to
RY,RZ,PhaseShift,CRX,CRY,CRZ,ControlledPhaseShift, and theIsing*gates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp` around lines 446 - 515, Adjoint handling is missing for parameterized rotation and Ising gates: check op.getAdjoint() for RX/RY/RZ/PhaseShift (P) and their controlled variants (CRX/CRY/CRZ/ControlledPhaseShift) and for IsingXY/IsingXX/IsingYY/IsingZZ, and if adjoint is true, negate the rotation parameter(s) before creating the qc ops; e.g., use a local angle = op.getAdjoint() ? -paramsValues[0] : paramsValues[0] (and for IsingXY preserve the added pi for the second argument while negating the phi), then pass that angle into CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP(RX)/rewriter.create<qc::RXOp>/CREATE_TWO_TARGET_ONE_PARAMETER_GATE_OP(RXX)/rewriter.create<qc::XXPlusYYOp> etc., and for controlled forms negate the parameter supplied inside the lambda similarly.
319-319: 🧹 Nitpick | 🔵 TrivialRemove unused
qcOpvariable.The variable
qcOpis assigned but never read. CI warns: "variable 'qcOp' set but not used". Remove it and use therewriter.create<>()calls directly for their side effects.🧹 Proposed fix
- // Create the new operation - Operation* qcOp = nullptr; + // Create the new operations (side-effect only, results not needed)Then change all
qcOp = rewriter.create<...>to justrewriter.create<...>(or use(void)cast if intentional).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp` at line 319, Remove the unused variable qcOp (declared as Operation* qcOp = nullptr;) and update all places that assign to it (e.g., qcOp = rewriter.create<...>(...)) to call rewriter.create<...>(...) directly for their side effects (or use a (void) cast if you deliberately want to ignore the return). Ensure no other code reads qcOp and delete its declaration so the "set but not used" warning is resolved.test/test_plugin.py (2)
560-565:⚠️ Potential issue | 🟠 MajorNested
qc.ctrlCHECK patterns won't match flattened converter output.
ConvertQuantumCustomOpemits a singleqc.ctrlwith the full control list, not nestedqc.ctrl { qc.ctrl { ... } }. The doubly-controlled RX check at lines 560-565 expects nested controls but the converter produces flattened output.🧪 Proposed fix
- //CHECK: qc.ctrl(%[[Q0_2:.*]]) { - qc.ctrl(%[[Q0_1:.*]]) { - qc.rx({{.*}}) %[[Q0_0:.*]] : !qc.qubit - } : !qc.qubit - } : !qc.qubit + //CHECK: qc.ctrl(%[[Q0_2:.*]], %[[Q0_1:.*]]) { + qc.rx({{.*}}) %[[Q0_0:.*]] : !qc.qubit + } : !qc.qubit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_plugin.py` around lines 560 - 565, The test expects nested qc.ctrl blocks but ConvertQuantumCustomOp emits a single flattened qc.ctrl with the full control list; update the CHECK in test_plugin.py to match a single qc.ctrl containing qc.rx (i.e., replace the nested "qc.ctrl { qc.ctrl { qc.rx ... } }" pattern with a flattened "qc.ctrl(...) { qc.rx(...) ... }" pattern that accepts the full control list and the qc.rx operation, referencing ConvertQuantumCustomOp, qc.ctrl and qc.rx in the adjusted CHECK.
788-797:⚠️ Potential issue | 🟡 MinorToffoli CHECK pattern has a stray trailing comma and wrong nesting.
Line 793 has
qc.ctrl(%[[Q0_0:.*]], %[[Q0_1:.*]],)with a trailing comma before the closing parenthesis, which will cause FileCheck to fail. Additionally, the pattern expects nestedqc.ctrlbut the converter emits a single flattenedqc.ctrl.🧪 Proposed fix
check_after_qc = """ - //CHECK: qc.ctrl(%[[Q0_0:.*]], %[[Q0_1:.*]]) { - qc.x %[[Q0_2:.*]] : !qc.qubit - } : !qc.qubit - //CHECK: qc.ctrl(%[[Q0_3:.*]]) { - qc.ctrl(%[[Q0_0:.*]], %[[Q0_1:.*]],) { - qc.x %[[Q0_2:.*]] : !qc.qubit - } : !qc.qubit - } : !qc.qubit + //CHECK: qc.ctrl(%[[Q0_0:.*]], %[[Q0_1:.*]]) { + qc.x %[[Q0_2:.*]] : !qc.qubit + } : !qc.qubit + //CHECK: qc.ctrl(%[[Q0_3:.*]], %[[Q0_0:.*]], %[[Q0_1:.*]]) { + qc.x %[[Q0_2:.*]] : !qc.qubit + } : !qc.qubit """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_plugin.py` around lines 788 - 797, The CHECK pattern in the check_after_qc string is wrong: remove the stray trailing comma in the second qc.ctrl and change the nested qc.ctrl expectation to match the converter's flattened control construct; replace the nested pattern "qc.ctrl(%[[Q0_3:.*]]) { qc.ctrl(%[[Q0_0:.*]], %[[Q0_1:.*]],) { qc.x %[[Q0_2:.*]] : !qc.qubit } : !qc.qubit } : !qc.qubit" with a single flattened control like "qc.ctrl(%[[Q0_0:.*]], %[[Q0_1:.*]], %[[Q0_3:.*]]) { qc.x %[[Q0_2:.*]] : !qc.qubit } : !qc.qubit" so FileCheck matches the emitted Toffoli representation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.h`:
- Around line 17-21: The generated header QCToCatalystQuantum.h.inc is missing
from the build, so update the TableGen/include-side wiring for the
QCToCatalystQuantum pass: add the TableGen rule to emit
QCToCatalystQuantum.h.inc (or restore its name) and ensure the build target that
produces it is run before compiling code that includes it; specifically ensure
the pass name QCToCatalystQuantum is registered in your TableGen input and that
the GEN_PASS_DECL / GEN_PASS_REGISTRATION expansion for QCToCatalystQuantum is
being generated by the TableGen backend so the include
"QCToCatalystQuantum.h.inc" is produced for downstream translation units (update
build files or CMake target dependencies as needed).
In `@lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp`:
- Line 164: Rename the local variable QCOp to lowerCamelCase qcOp where it's
created with rewriter.create<qc::MeasureOp>(op.getLoc(), inQubit) and update all
subsequent uses (e.g., any reads, .getResult(), or replacements that reference
QCOp) to qcOp to satisfy the project's naming conventions and silence the CI
lint warning.
- Line 18: Remove the unused include directive: delete the line `#include
<cstddef>` from CatalystQuantumToQC.cpp so the file no longer contains an unused
header; verify compilation passes and no symbols from `<cstddef>` (e.g.,
`size_t`, `nullptr_t`) are relied on in functions or classes within this
translation unit before committing.
In `@lib/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.cpp`:
- Around line 95-107: The template helper function extractOperands
(template<typename OpAdaptor>) is unused; remove its definition and any related
forward declarations to clean up dead code—specifically delete the
extractOperands function block that returns ExtractedOperands and references
adaptor.getInQubits(), getPosCtrlInQubits(), getNegCtrlInQubits(), and
extractControlInfo; after removal, run a build to ensure no remaining references
to ExtractedOperands or extractOperands remain and remove any now-unneeded
includes or using declarations introduced solely for this helper.
In `@test/test_plugin.py`:
- Around line 43-44: DEFAULT_PIPELINE currently includes the back-conversion
step ("ToCatalystQuantum", ["builtin.module(qc-to-catalystquantum)"]) but
registerQCToCatalystQuantumPasses() in lib/mqt-plugin.cpp is commented out,
causing roundtrip tests to fail; fix by either removing/commenting the
("ToCatalystQuantum", ["builtin.module(qc-to-catalystquantum)"]) entry from
DEFAULT_PIPELINE in test/test_plugin.py or, if you prefer to keep the pipeline
unchanged, mark the failing roundtrip tests with pytest.mark.skip until
registerQCToCatalystQuantumPasses() is implemented—search for DEFAULT_PIPELINE
and registerQCToCatalystQuantumPasses to locate the relevant spots to change.
---
Duplicate comments:
In `@cmake/ExternalDependencies.cmake`:
- Around line 18-21: The defaults for MQT_CORE_REPO_OWNER and MQT_CORE_REV must
be reverted to the official upstream and a pinned commit SHA: update the
variable MQT_CORE_REPO_OWNER to "munich-quantum-toolkit" and replace the mutable
branch value in MQT_CORE_REV with a 40-character commit hash (hex) for a
reproducible pinned revision; leave any fork/branch values only as optional
overrides or documented examples in comments, not as the CACHE default, and
ensure MQT_CORE_REV matches the full SHA pattern [0-9a-f]{40}.
In `@lib/CMakeLists.txt`:
- Around line 25-26: The plugin target created by
add_llvm_library(${TARGET_NAME} MODULE mqt-plugin.cpp LINK_LIBS
CatalystQuantumToQC currently omits QCToCatalystQuantum, so when
registerQCToCatalystQuantumPasses() is re-enabled the reverse conversion won't
be available; fix this by adding QCToCatalystQuantum to the LINK_LIBS for the
target (i.e., ensure LINK_LIBS includes both CatalystQuantumToQC and
QCToCatalystQuantum for the ${TARGET_NAME} plugin built from mqt-plugin.cpp).
In `@lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp`:
- Around line 446-515: Adjoint handling is missing for parameterized rotation
and Ising gates: check op.getAdjoint() for RX/RY/RZ/PhaseShift (P) and their
controlled variants (CRX/CRY/CRZ/ControlledPhaseShift) and for
IsingXY/IsingXX/IsingYY/IsingZZ, and if adjoint is true, negate the rotation
parameter(s) before creating the qc ops; e.g., use a local angle =
op.getAdjoint() ? -paramsValues[0] : paramsValues[0] (and for IsingXY preserve
the added pi for the second argument while negating the phi), then pass that
angle into
CREATE_ONE_TARGET_ONE_PARAMETER_GATE_OP(RX)/rewriter.create<qc::RXOp>/CREATE_TWO_TARGET_ONE_PARAMETER_GATE_OP(RXX)/rewriter.create<qc::XXPlusYYOp>
etc., and for controlled forms negate the parameter supplied inside the lambda
similarly.
- Line 319: Remove the unused variable qcOp (declared as Operation* qcOp =
nullptr;) and update all places that assign to it (e.g., qcOp =
rewriter.create<...>(...)) to call rewriter.create<...>(...) directly for their
side effects (or use a (void) cast if you deliberately want to ignore the
return). Ensure no other code reads qcOp and delete its declaration so the "set
but not used" warning is resolved.
In `@lib/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.cpp`:
- Around line 315-326: InsertOp's result is being dropped so the updated
!quantum.reg is lost; change the creation to capture the returned value from
rewriter.create<catalyst::quantum::InsertOp> (e.g., auto newInsert =
rewriter.create<...>(...)) and then thread that result into the SSA flow by
replacing the original op's result or updating the conversion mapping (use
rewriter.replaceOp(op, newInsert.getResult(0)) or update your
ValueMapping/rewriter mapping so later uses see newInsert.getResult(0)); ensure
you remove the old op after replacing so no users remain referencing the
discarded value.
- Around line 37-41: Implement the pass's runOnOperation for the pass declared
by GEN_PASS_DEF_QCTOCATALYSTQUANTUM: create a new instance of ConversionTarget,
register legality for target dialects, populate a RewritePatternSet with the
existing patterns (ConvertQCAlloc, ConvertQCDealloc, ConvertQCExtract,
ConvertQCInject, etc.), and then call applyPartialConversion(getOperation(),
target, patterns). Ensure the pass class (the one generated by
GEN_PASS_DEF_QCTOCATALYSTQUANTUM) overrides runOnOperation(), constructs the
target and patterns, marks ops as legal/illegal as appropriate, and handles
failure by signalling signalPassFailure() when applyPartialConversion fails.
In `@lib/mqt-plugin.cpp`:
- Around line 11-13: The build fails because the TableGen-generated header
QCToCatalystQuantum.h.inc isn't being produced/visible; update
include/mlir/Conversion/QCToCatalystQuantum/CMakeLists.txt to run mlir_tablegen
to generate QCToCatalystQuantum.h.inc (invoking the correct TABLEGEN or
MLIR_TABLEGEN rule for the QCToCatalystQuantum tablegen file) and add the
generated include directory to the consuming target via
target_include_directories or target_include_directories(... SYSTEM BEFORE) so
that the target building lib/mqt-plugin.cpp can find
"mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.h" which expects the
.h.inc; ensure the generated file name matches QCToCatalystQuantum.h.inc and
that the target that builds mqt-plugin depends on the tablegen output target so
generation happens before compilation.
- Around line 46-47: Add a short TODO comment next to the commented-out
registerQCToCatalystQuantumPasses() call in lib/mqt-plugin.cpp to indicate the
back-conversion is incomplete and needs re-enabling or completion later;
reference the symbol registerQCToCatalystQuantumPasses() and mention
“back-conversion incomplete” and optionally link to the PR or issue number if
available so future maintainers can discover why it is disabled.
In `@test/test_plugin.py`:
- Around line 560-565: The test expects nested qc.ctrl blocks but
ConvertQuantumCustomOp emits a single flattened qc.ctrl with the full control
list; update the CHECK in test_plugin.py to match a single qc.ctrl containing
qc.rx (i.e., replace the nested "qc.ctrl { qc.ctrl { qc.rx ... } }" pattern with
a flattened "qc.ctrl(...) { qc.rx(...) ... }" pattern that accepts the full
control list and the qc.rx operation, referencing ConvertQuantumCustomOp,
qc.ctrl and qc.rx in the adjusted CHECK.
- Around line 788-797: The CHECK pattern in the check_after_qc string is wrong:
remove the stray trailing comma in the second qc.ctrl and change the nested
qc.ctrl expectation to match the converter's flattened control construct;
replace the nested pattern "qc.ctrl(%[[Q0_3:.*]]) { qc.ctrl(%[[Q0_0:.*]],
%[[Q0_1:.*]],) { qc.x %[[Q0_2:.*]] : !qc.qubit } : !qc.qubit } : !qc.qubit" with
a single flattened control like "qc.ctrl(%[[Q0_0:.*]], %[[Q0_1:.*]],
%[[Q0_3:.*]]) { qc.x %[[Q0_2:.*]] : !qc.qubit } : !qc.qubit" so FileCheck
matches the emitted Toffoli representation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e22f9b1a-28e1-47f3-807d-9502e02e7eec
📒 Files selected for processing (20)
cmake/ExternalDependencies.cmakeinclude/mlir/Conversion/CMakeLists.txtinclude/mlir/Conversion/CatalystQuantumToMQTOpt/CMakeLists.txtinclude/mlir/Conversion/CatalystQuantumToQC/CMakeLists.txtinclude/mlir/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.hinclude/mlir/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.tdinclude/mlir/Conversion/MQTOptToCatalystQuantum/CMakeLists.txtinclude/mlir/Conversion/QCToCatalystQuantum/CMakeLists.txtinclude/mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.hinclude/mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.tdlib/CMakeLists.txtlib/Conversion/CMakeLists.txtlib/Conversion/CatalystQuantumToMQTOpt/CatalystQuantumToMQTOpt.cpplib/Conversion/CatalystQuantumToQC/CMakeLists.txtlib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpplib/Conversion/MQTOptToCatalystQuantum/MQTOptToCatalystQuantum.cpplib/Conversion/QCToCatalystQuantum/CMakeLists.txtlib/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.cpplib/mqt-plugin.cpptest/test_plugin.py
💤 Files with no reviewable changes (4)
- include/mlir/Conversion/CatalystQuantumToMQTOpt/CMakeLists.txt
- include/mlir/Conversion/MQTOptToCatalystQuantum/CMakeLists.txt
- lib/Conversion/CatalystQuantumToMQTOpt/CatalystQuantumToMQTOpt.cpp
- lib/Conversion/MQTOptToCatalystQuantum/MQTOptToCatalystQuantum.cpp
Comment on lines
17
to
+21
| #define GEN_PASS_DECL | ||
| #include "mlir/Conversion/CatalystQuantumToMQTOpt/CatalystQuantumToMQTOpt.h.inc" | ||
| #include "mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.h.inc" | ||
|
|
||
| #define GEN_PASS_REGISTRATION | ||
| #include "mlir/Conversion/CatalystQuantumToMQTOpt/CatalystQuantumToMQTOpt.h.inc" | ||
| #include "mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.h.inc" |
Contributor
There was a problem hiding this comment.
Generate QCToCatalystQuantum.h.inc before including it here.
CI already fails because this renamed header is not being produced. Please add or fix the matching include-side TableGen wiring for QCToCatalystQuantum before merging; otherwise every translation unit that includes this header will fail to compile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/mlir/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.h` around
lines 17 - 21, The generated header QCToCatalystQuantum.h.inc is missing from
the build, so update the TableGen/include-side wiring for the
QCToCatalystQuantum pass: add the TableGen rule to emit
QCToCatalystQuantum.h.inc (or restore its name) and ensure the build target that
produces it is run before compiling code that includes it; specifically ensure
the pass name QCToCatalystQuantum is registered in your TableGen input and that
the GEN_PASS_DECL / GEN_PASS_REGISTRATION expansion for QCToCatalystQuantum is
being generated by the TableGen backend so the include
"QCToCatalystQuantum.h.inc" is produced for downstream translation units (update
build files or CMake target dependencies as needed).
| #include <Quantum/IR/QuantumDialect.h> | ||
| #include <Quantum/IR/QuantumOps.h> | ||
| #include <cassert> | ||
| #include <cstddef> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused include <cstddef>.
Static analysis indicates this header is not used directly.
-#include <cstddef>📝 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 <cstddef> |
🧰 Tools
🪛 Cppcheck (2.20.0)
[information] 18-18: Include file
(missingInclude)
[information] 18-18: Include file
(missingIncludeSystem)
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
[warning] 18-18: lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp:18:1 [misc-include-cleaner]
included header cstddef is not used directly
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp` at line 18,
Remove the unused include directive: delete the line `#include <cstddef>` from
CatalystQuantumToQC.cpp so the file no longer contains an unused header; verify
compilation passes and no symbols from `<cstddef>` (e.g., `size_t`, `nullptr_t`)
are relied on in functions or classes within this translation unit before
committing.
| // Create the new operation | ||
| // Note: quantum.measure returns (i1, !quantum.bit) | ||
| // qc.measure returns i1 | ||
| auto QCOp = rewriter.create<qc::MeasureOp>(op.getLoc(), inQubit); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fix variable naming: QCOp should be qcOp.
CI warns: "invalid case style for local variable 'QCOp'". Local variables should use lowerCamelCase.
- auto QCOp = rewriter.create<qc::MeasureOp>(op.getLoc(), inQubit);
+ auto qcOp = rewriter.create<qc::MeasureOp>(op.getLoc(), inQubit);🧰 Tools
🪛 GitHub Check: 🇨 Lint / 🚨 Lint
[warning] 164-164: lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp:164:10 [readability-identifier-naming]
invalid case style for local variable 'QCOp'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/Conversion/CatalystQuantumToQC/CatalystQuantumToQC.cpp` at line 164,
Rename the local variable QCOp to lowerCamelCase qcOp where it's created with
rewriter.create<qc::MeasureOp>(op.getLoc(), inQubit) and update all subsequent
uses (e.g., any reads, .getResult(), or replacements that reference QCOp) to
qcOp to satisfy the project's naming conventions and silence the CI lint
warning.
Comment on lines
+95
to
+107
| template <typename OpAdaptor> | ||
| ExtractedOperands<OpAdaptor> | ||
| extractOperands(OpAdaptor adaptor, ConversionPatternRewriter& rewriter, | ||
| Location loc) { | ||
| const ValueRange inQubits = adaptor.getInQubits(); | ||
| const ValueRange posCtrlQubits = adaptor.getPosCtrlInQubits(); | ||
| const ValueRange negCtrlQubits = adaptor.getNegCtrlInQubits(); | ||
|
|
||
| const ControlInfo ctrlInfo = | ||
| extractControlInfo(posCtrlQubits, negCtrlQubits, rewriter, loc); | ||
|
|
||
| return {inQubits, ctrlInfo}; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused extractOperands helper.
The extractOperands template function is never called. Static analysis confirms: "The function 'extractOperands' is never used."
🧹 Proposed fix
-/// Helper function to extract operands and control info - for more complex
-/// cases
-template <typename OpAdaptor> struct ExtractedOperands {
- ValueRange inQubits;
- ControlInfo ctrlInfo;
-};
-
-template <typename OpAdaptor>
-ExtractedOperands<OpAdaptor>
-extractOperands(OpAdaptor adaptor, ConversionPatternRewriter& rewriter,
- Location loc) {
- const ValueRange inQubits = adaptor.getInQubits();
- const ValueRange posCtrlQubits = adaptor.getPosCtrlInQubits();
- const ValueRange negCtrlQubits = adaptor.getNegCtrlInQubits();
-
- const ControlInfo ctrlInfo =
- extractControlInfo(posCtrlQubits, negCtrlQubits, rewriter, loc);
-
- return {inQubits, ctrlInfo};
-}🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 97-97: The function 'extractOperands' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/Conversion/QCToCatalystQuantum/QCToCatalystQuantum.cpp` around lines 95 -
107, The template helper function extractOperands (template<typename OpAdaptor>)
is unused; remove its definition and any related forward declarations to clean
up dead code—specifically delete the extractOperands function block that returns
ExtractedOperands and references adaptor.getInQubits(), getPosCtrlInQubits(),
getNegCtrlInQubits(), and extractControlInfo; after removal, run a build to
ensure no remaining references to ExtractedOperands or extractOperands remain
and remove any now-unneeded includes or using declarations introduced solely for
this helper.
Comment on lines
+43
to
+44
| ("ToQC", ["builtin.module(catalystquantum-to-qc)"]), | ||
| ("ToCatalystQuantum", ["builtin.module(qc-to-catalystquantum)"]), |
Contributor
There was a problem hiding this comment.
Tests will fail until back-conversion is enabled.
The DEFAULT_PIPELINE includes "qc-to-catalystquantum" at line 44, but registerQCToCatalystQuantumPasses() is commented out in lib/mqt-plugin.cpp. Tests exercising the full roundtrip will fail until back-conversion is complete.
Consider either:
- Commenting out the back-conversion step in
DEFAULT_PIPELINEfor now - Adding
pytest.mark.skipto roundtrip tests until back-conversion is ready
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/test_plugin.py` around lines 43 - 44, DEFAULT_PIPELINE currently
includes the back-conversion step ("ToCatalystQuantum",
["builtin.module(qc-to-catalystquantum)"]) but
registerQCToCatalystQuantumPasses() in lib/mqt-plugin.cpp is commented out,
causing roundtrip tests to fail; fix by either removing/commenting the
("ToCatalystQuantum", ["builtin.module(qc-to-catalystquantum)"]) entry from
DEFAULT_PIPELINE in test/test_plugin.py or, if you prefer to keep the pipeline
unchanged, mark the failing roundtrip tests with pytest.mark.skip until
registerQCToCatalystQuantumPasses() is implemented—search for DEFAULT_PIPELINE
and registerQCToCatalystQuantumPasses to locate the relevant spots to change.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 updates the plugin to convert to MQT qc dialect instead of MQTOpt dialect.
Back conversion is started, but needs to be finished later. This draft PR is mainly for feedback and discussion.
Tests via pytest are updated for the qc dialect, but will fail, since the back conversion is not finished.
If code for testing the back conversion is commented out, the test are successfull.
Checklist