✨ Add OpenQASM-to-QC translation#1671
Conversation
burgholzer
added
feature
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a direct OpenQASM3 → MLIR QC dialect translator: public API header, visitor-based importer with gate dispatch, complete translation logic (gates, measurements, controls, compound gates, if/else), tool integration into mqt-cc, and parameterized unit tests with shared QASM fixtures. ChangesQASM3-to-QC Direct Importer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)mlir/tools/mqt-cc/mqt-cc.cppmlir/tools/mqt-cc/mqt-cc.cpp:11:10: fatal error: 'mlir/Compiler/CompilerPipeline.h' file not found ... [truncated 2200 characters] ... od in file "src/clang/cFrontend_decl.ml" (inlined), line 54, characters 4-52 mlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cppmlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cpp:11:10: fatal error: 'mlir/Dialect/QC/Translation/TranslateQASM3ToQC.h' file not found ... [truncated 2200 characters] ... decl_funct.add_method.f in file "src/clang/cFrontend_decl.ml", line 48, characters 12-91 mlir/unittests/Dialect/QC/Translation/test_qasm3_translation.cppmlir/unittests/Dialect/QC/Translation/test_qasm3_translation.cpp:11:10: fatal error: 'TestCaseUtils.h' file not found ... [truncated 2200 characters] ... tend__CFrontend_decl.CFrontend_decl_funct.add_method in file "src/clang/cFrontend_decl.ml" (inlined), line 54, characters 4-52 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/tools/mqt-cc/mqt-cc.cpp (1)
142-146: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove duplicate
MemRefDialectregistration.
memref::MemRefDialectandmlir::memref::MemRefDialectare the same dialect here, so this registers it twice. Keep a single insertion to avoid redundant setup and future confusion.Suggested diff
registry.insert<func::FuncDialect>(); registry.insert<memref::MemRefDialect>(); registry.insert<scf::SCFDialect>(); registry.insert<LLVM::LLVMDialect>(); - registry.insert<mlir::memref::MemRefDialect>(); registry.insert<qtensor::QTensorDialect>();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/tools/mqt-cc/mqt-cc.cpp` around lines 142 - 146, The code registers the same dialect twice via registry.insert<memref::MemRefDialect>() and registry.insert<mlir::memref::MemRefDialect>(); remove one of these duplicate registry.insert calls (keep either the unqualified memref::MemRefDialect or the fully-qualified mlir::memref::MemRefDialect for clarity) so the dialect is registered only once and the subsequent registry.insert<qtensor::QTensorDialect>() remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mlir/include/mlir/Dialect/QC/Translation/TranslateQASM3ToQC.h`:
- Around line 28-32: Add a Doxygen comment for the stream overload
translateQASM3ToQC(MLIRContext* context, std::istream& input) matching the
filename overload: describe the function purpose, document parameters (context
and input), state the error/reporting behavior (errors are reported via
llvm::errs()), and specify the return contract (returns an OwningOpRef<ModuleOp>
or nullptr on failure). Place the comment immediately above the second
declaration so the stream-variant’s contract is clear to callers.
In `@mlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cpp`:
- Around line 891-909: The helper function convertComparisonKind is currently
dead code because the BinaryExpression branch in translateCondition throws
unconditionally; either remove convertComparisonKind to avoid unusedFunction
warnings or rewire translateCondition's BinaryExpression case to call
convertComparisonKind (and handle result) so register comparisons are actually
translated; locate convertComparisonKind and the BinaryExpression branch in
translateCondition and implement one of the two fixes consistently (delete the
unused static function and its declaration, or replace the throw in
translateCondition's BinaryExpression path with logic that maps
::qc::ComparisonKind via convertComparisonKind and uses the resulting
arith::CmpIPredicate).
- Around line 587-610: The modifier-controls loop iterates ctrlIdx and indexes
expandedOperands without checking bounds, which can UB if there are fewer
operands; add a guard inside the for (const auto& [n, positive] : ctrlSpec) /
inner for (size_t i = 0; i < n; ++i, ++ctrlIdx) before accessing
expandedOperands[ctrlIdx] to verify ctrlIdx < expandedOperands.size() and that
expandedOperands[ctrlIdx].size() == 1, and if not throw a qasm3::CompilerError
(using stmt->debugInfo) with a clear message (similar to the existing implicit
OQ2 control check) so posControls/negControls pushes are safe.
- Around line 666-678: The loop uses concat<const Value>(iterQubits,
posControls, negControls) which applies a const qualifier to the MLIR handle
type; change the call to concat<Value>(...) (drop the const) so the range yields
plain Value handles and matches project conventions—update the concat call where
seen (llvm::SmallDenseSet<Value> seen) and the subsequent for (auto q : ...)
loop that precedes emitGate(dispIt->second, iterQubits, params, posControls,
negControls, invert).
- Around line 825-837: The unary-negation branch currently assumes
unaryExpr->operand is an IndexedIdentifier and passes the result of
std::dynamic_pointer_cast<qasm3::IndexedIdentifier>(unaryExpr->operand) directly
to lookupBitValue, which can yield nullptr and cause a crash; update this branch
to check that idExpr is non-null and if it is null throw a CompilerError (or use
the project's existing error reporting) with a clear message (e.g., "unsupported
operand for unary negation: expected IndexedIdentifier") instead of calling
lookupBitValue; reference UnaryExpression, IndexedIdentifier, lookupBitValue and
the CompilerError path when locating and changing the code.
- Around line 742-766: The compound-gate body loop in the bodyFn lambda silently
drops unhandled statement kinds; update the for-loop over gate.body (inside
bodyFn) to add a terminal else branch that emits a clear diagnostic/throws
(using the statement's debugInfo when available) rather than ignoring unknown
types, so any unrecognized bodyStmt triggers a failure; locate the bodyFn lambda
and add the else to complement the existing
GateCallStatement/BarrierStatement/ResetStatement branches (which call
applyGateCallStatement, resolveGateOperandInScope, builder.barrier,
builder.reset) and produce a descriptive error/exception mentioning the
unexpected statement kind and debugInfo.
- Around line 312-331: The default branch of the switch on sizedTy->type
currently does nothing and can silently skip allocating registers (leaving
declarations populated but no register in qubitRegisters or classicalRegisters);
update the default case inside the switch (the branch handling sizedTy->type) to
explicitly fail fast for unsupported sized types (e.g., Float, Angle) by
reporting a clear error via the translation context/builder error API (or
throwing/returning an error) that includes the identifier and the unsupported
type, so unsupported declarations are rejected immediately instead of causing
later failures.
---
Outside diff comments:
In `@mlir/tools/mqt-cc/mqt-cc.cpp`:
- Around line 142-146: The code registers the same dialect twice via
registry.insert<memref::MemRefDialect>() and
registry.insert<mlir::memref::MemRefDialect>(); remove one of these duplicate
registry.insert calls (keep either the unqualified memref::MemRefDialect or the
fully-qualified mlir::memref::MemRefDialect for clarity) so the dialect is
registered only once and the subsequent
registry.insert<qtensor::QTensorDialect>() remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49e31367-7ed5-4787-8afb-2adf9d0d361d
📒 Files selected for processing (10)
mlir/include/mlir/Dialect/QC/Translation/TranslateQASM3ToQC.hmlir/lib/Dialect/QC/Translation/CMakeLists.txtmlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cppmlir/tools/mqt-cc/mqt-cc.cppmlir/unittests/Dialect/QC/Translation/CMakeLists.txtmlir/unittests/Dialect/QC/Translation/test_qasm3_translation.cppmlir/unittests/programs/CMakeLists.txtmlir/unittests/programs/qasm_programs.cppmlir/unittests/programs/qasm_programs.htest/circuits/bell.qasm
💤 Files with no reviewable changes (1)
- test/circuits/bell.qasm
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cpp (1)
369-406: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
visitGateStatementsilently accepts user-defined gates that later fail with a misleading "Unknown gate" error.For a non-opaque, non-standard gate declaration, this method validates
paramNames/qubitNamesinto locals that are never stored, and never inserts the gate intogates. When the same gate is invoked,applyGateCallStatement(line 533-534) raisesUnknown gate '<name>', which is misleading — the gate is in fact a compound gate that was intentionally dropped (per the "Drop support for compound gates for now" commit).Either fail fast here with a clear diagnostic, or remove the dead local validation. The former gives users a useful error pointing at the declaration site.
♻️ Suggested change — fail fast with a clear message
void visitGateStatement(std::shared_ptr<qasm3::GateDeclaration> stmt) override { auto id = stmt->identifier; if (stmt->isOpaque) { if (!gates.contains(id)) { throw qasm3::CompilerError("Unsupported opaque gate '" + id + "'.", stmt->debugInfo); } return; } if (gates.contains(id)) { if (std::dynamic_pointer_cast<qasm3::StandardGate>(gates[id])) { return; // ignore redeclaration of standard gate } throw qasm3::CompilerError("Gate '" + id + "' already declared.", stmt->debugInfo); } - std::vector<std::string> paramNames; - for (const auto& p : stmt->parameters->identifiers) { - if (std::ranges::find(paramNames, p->identifier) != paramNames.end()) { - throw qasm3::CompilerError("Parameter '" + p->identifier + - "' already declared in gate '" + id + - "'.", - stmt->debugInfo); - } - paramNames.push_back(p->identifier); - } - std::vector<std::string> qubitNames; - for (const auto& q : stmt->qubits->identifiers) { - if (std::ranges::find(qubitNames, q->identifier) != qubitNames.end()) { - throw qasm3::CompilerError("Qubit '" + q->identifier + - "' already declared in gate '" + id + - "'.", - stmt->debugInfo); - } - qubitNames.push_back(q->identifier); - } + throw qasm3::CompilerError( + "User-defined (compound) gate '" + id + + "' is not supported by direct MLIR import.", + stmt->debugInfo); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cpp` around lines 369 - 406, The visitGateStatement function currently validates parameters/qubits for non-opaque, non-standard gate declarations but never inserts the gate into the gates map, causing later "Unknown gate" errors; change visitGateStatement to fail fast for such compound (non-opaque, non-standard) gate declarations by throwing a qasm3::CompilerError (use stmt->debugInfo) with a clear message like "Compound gates are not supported: '<id>'" instead of silently dropping them (refer to visitGateStatement and applyGateCallStatement to locate the flow), so users get a direct diagnostic at the declaration site.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@mlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cpp`:
- Around line 369-406: The visitGateStatement function currently validates
parameters/qubits for non-opaque, non-standard gate declarations but never
inserts the gate into the gates map, causing later "Unknown gate" errors; change
visitGateStatement to fail fast for such compound (non-opaque, non-standard)
gate declarations by throwing a qasm3::CompilerError (use stmt->debugInfo) with
a clear message like "Compound gates are not supported: '<id>'" instead of
silently dropping them (refer to visitGateStatement and applyGateCallStatement
to locate the flow), so users get a direct diagnostic at the declaration site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f89c1f1-14f7-475f-bfe6-57b1e1604f05
📒 Files selected for processing (3)
mlir/include/mlir/Dialect/QC/Translation/TranslateQASM3ToQC.hmlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cppmlir/tools/mqt-cc/mqt-cc.cpp
denialhaag
force-pushed
the
open-qasm
branch
2 times, most recently
from
78a4271 to
4ec599d
Compare
|
Follow-ups are collected in #1733. |
This reverts commit 00835a8.
|
This PR was automatically closed when #1751 was merged. Sorry about that. I'll open up a new PR. |
|
The new PR is #1780. |
3 participants
Description
This PR adds a direct OpenQASM-to-QC translation that bypasses the
qc::QuantumComputation.The new flow reuses the existing parser and walks the AST directly to emit QC dialect ops. Skipping
QuantumComputationremoves an intermediate step and enables previously unsupported QASM3 features, such as gate modifiers, classical computations, and control flow structures.What works
ctrl @,negctrl @,inv @, nested combinationsif/elseover quantum statementsqc.staticCurrent limitations
powmodifier is unsupported (but will/should be once ✨ Add power modifier to MLIR #1603 is merged)QC)Checklist
I have updated the documentation to reflect these changes.I have added migration instructions to the upgrade guide (if needed).