✨ Add OpenQASM-to-QC translation#1780
Conversation
This reverts commit 00835a8.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cpp (1)
627-662:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject invalid operand counts before invoking the gate emitter.
Lines 627-662 check parameter arity, but never validate operand arity before the dispatch lambdas index
q[0],q[1], orq.back(). That means too-few operands can trip assertions / out-of-bounds access, while too-many operands are silently ignored by single-target emitters. Please validate fixed-arity gates against their declared operand count and add explicit minimum-arity checks for the variadicmcx*/mcphaseemitters before the broadcast loop.
🤖 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/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cpp`:
- Around line 808-823: The current branch treats any qasm3::UnaryExpression as a
logical/bitwise not via an assert that is removed in release builds; update the
UnaryExpression handling to explicitly check unaryExpr->op and only proceed when
it equals qasm3::UnaryExpression::LogicalNot or BitwiseNot, otherwise throw
qasm3::CompilerError(debugInfo) (use the same debugInfo message pattern as other
errors). Keep the rest of the logic (casting operand to
qasm3::IndexedIdentifier, calling lookupBitValue, creating arith::XOrIOp via
builder) unchanged, but replace the assert with an explicit guard that throws
for unsupported ops.
In `@mlir/unittests/programs/qasm_programs.h`:
- Around line 357-358: Update the Doxygen comment for the ifNot declaration to
use present tense to match the file's style; change the sentence beginning
"Created a circuit with an if operation with a negated condition." to "Creates a
circuit with an if operation with a negated condition." so it consistently uses
"Creates" for the ifNot symbol.
🪄 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: 13850f71-e4cc-4e1a-b10c-66d2fd09ed29
📒 Files selected for processing (4)
mlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cppmlir/unittests/Dialect/QC/Translation/test_qasm3_translation.cppmlir/unittests/programs/qasm_programs.cppmlir/unittests/programs/qasm_programs.h
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 (3)
mlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cpp (3)
868-869: 🩺 Stability & Availability | 🟠 MajorGuard
indexExpressionslength before indexing[0]in TranslateQASM3ToQC.cppAfter only checking
indices.size() <= 1, these helpers immediately read...->indexExpressions[0]without ensuring the singleIndexOperatoractually contains exactly oneindexExpression.Parser::parseIndexOperatorcan produce an emptyindexExpressionsvector (e.g.,id[]), andTypeCheckPassdoesn’t reject emptyindexExpressions, so this can lead to out-of-bounds access.Suggested guard pattern
- const auto& indexExpression = id->indices[0]->indexExpressions[0]; + const auto& indexExpressions = id->indices[0]->indexExpressions; + if (indexExpressions.size() != 1) { + throw qasm3::CompilerError("Only single-index expressions are supported.", + debugInfo); + } + const auto& indexExpression = indexExpressions[0];Applies to: 868-869, 934-935, 967-968
🤖 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 868 - 869, Guard accesses into indexExpressions before reading [0]: in places that read id->indices[0]->indexExpressions[0] (used with evaluatePositiveConstant and debugInfo), check that id->indices.size() > 0 and that id->indices[0]->indexExpressions.size() > 0 and handle the empty case (emit an error via debugInfo or return/fail gracefully) to avoid out-of-bounds access; similarly update the other occurrences that access indexExpressions[0] (same pattern as above) and prefer early-return or diagnostic logging consistent with surrounding helpers so callers like evaluatePositiveConstant aren’t passed an invalid reference.
322-327: 🎯 Functional Correctness | 🟠 MajorQASM2
if (creg == N)hard-fails becausetranslateConditionrejectsBinaryExpressioneven in OpenQASM2 compat mode.
visitVersionDeclarationenablesopenQASM2CompatModefor OpenQASM2 (<3), buttranslateCondition(used to build thescf::IfOpcondition) supports onlyIndexedIdentifierand unary!/~; anyqasm3::BinaryExpression(e.g.,creg == N/!=) immediately throwsRegister comparisons are not supported.openQASM2CompatModeis only applied to gate-call parsing (implicit leadingccontrols), not condition lowering (so measurement results inbitValuesdon’t help for these comparisons).
File:mlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cpp(322-327, 832-836)🤖 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 322 - 327, visitVersionDeclaration sets openQASM2CompatMode for QASM2 but translateCondition still rejects qasm3::BinaryExpression (e.g., creg == N), causing hard failures; update translateCondition to accept BinaryExpression when openQASM2CompatMode is true (function translateCondition) and handle equality/inequality ops (==, !=) by resolving lhs/rhs via existing bitValues/IndexedIdentifier logic (or valid Identifier→bit mapping) and producing the appropriate boolean Value for scf::IfOp, reusing the same lowering path as for IndexedIdentifier/unary ops; ensure the new branch validates operand types (Identifier/IndexedIdentifier or integer literal), uses bitValues lookup or index extraction, and emits the same error messages on unsupported forms.
566-588: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
ctrl/negctrlcontrols are forced to scalars before broadcast lowering.
- In
applyGateCallStatement, the modifier parsing consumes the would-be control operands and immediately rejects controls that resolve toSmallVector<Value>("Control operand must be a qubit."), converting them into scalarValues inposControls/negControls.- Broadcast lowering then runs only over the remaining
gateOperands(targets):broadcastWidthis computed fromgateOperands, whileposControls/negControlsstay lane-invariant, soctrl @ x q, r/negctrl @ x q, rcan’t broadcast whenqis a register (even thoughcx q, rsupports lane-wise broadcasting via its operands).- The same scalar-only guard exists in the OpenQASM2 implicit-controls loop just below.
🤖 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 566 - 588, The control parsing in applyGateCallStatement currently rejects and forces controls to scalar Values (throwing "Control operand must be a qubit.") which prevents lane-wise broadcasting; change the logic so posControls and negControls can store either a scalar Value or a SmallVector<Value> (e.g. use a variant or an existing container type), stop throwing on SmallVector<Value>, and push the SmallVector into the corresponding control list instead of erroring; then update subsequent broadcast-lowering logic that computes broadcastWidth from gateOperands to consider control operands (posControls/negControls) as potentially vector-shaped so broadcasting is applied consistently across controls and targets (ensure evaluatePositiveConstant, numControls incrementing, and uses of operands/gateOperands are adapted to handle the new scalar-or-vector control representation).
🤖 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 868-869: Guard accesses into indexExpressions before reading [0]:
in places that read id->indices[0]->indexExpressions[0] (used with
evaluatePositiveConstant and debugInfo), check that id->indices.size() > 0 and
that id->indices[0]->indexExpressions.size() > 0 and handle the empty case (emit
an error via debugInfo or return/fail gracefully) to avoid out-of-bounds access;
similarly update the other occurrences that access indexExpressions[0] (same
pattern as above) and prefer early-return or diagnostic logging consistent with
surrounding helpers so callers like evaluatePositiveConstant aren’t passed an
invalid reference.
- Around line 322-327: visitVersionDeclaration sets openQASM2CompatMode for
QASM2 but translateCondition still rejects qasm3::BinaryExpression (e.g., creg
== N), causing hard failures; update translateCondition to accept
BinaryExpression when openQASM2CompatMode is true (function translateCondition)
and handle equality/inequality ops (==, !=) by resolving lhs/rhs via existing
bitValues/IndexedIdentifier logic (or valid Identifier→bit mapping) and
producing the appropriate boolean Value for scf::IfOp, reusing the same lowering
path as for IndexedIdentifier/unary ops; ensure the new branch validates operand
types (Identifier/IndexedIdentifier or integer literal), uses bitValues lookup
or index extraction, and emits the same error messages on unsupported forms.
- Around line 566-588: The control parsing in applyGateCallStatement currently
rejects and forces controls to scalar Values (throwing "Control operand must be
a qubit.") which prevents lane-wise broadcasting; change the logic so
posControls and negControls can store either a scalar Value or a
SmallVector<Value> (e.g. use a variant or an existing container type), stop
throwing on SmallVector<Value>, and push the SmallVector into the corresponding
control list instead of erroring; then update subsequent broadcast-lowering
logic that computes broadcastWidth from gateOperands to consider control
operands (posControls/negControls) as potentially vector-shaped so broadcasting
is applied consistently across controls and targets (ensure
evaluatePositiveConstant, numControls incrementing, and uses of
operands/gateOperands are adapted to handle the new scalar-or-vector control
representation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b2114bd-5a19-4bff-9d74-2718c9b71a9d
📒 Files selected for processing (2)
mlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cppmlir/unittests/programs/qasm_programs.h
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cpp`:
- Around line 884-887: The current code relies on an assert to verify that an
unindexed register is single-bit, which is ineffective in release builds and can
lead to silently accepting invalid multi-bit classical conditions. Replace the
assert(regBits.size() == 1) with an explicit check using the declared width from
classicalRegisters: if the register width is not 1 (i.e., it is a multi-bit
register), throw a CompilerError describing that unindexed access to multi-bit
classical registers is not allowed. This ensures that multi-bit registers
without indices are properly rejected at compile time regardless of build
configuration.
🪄 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: 74f173b1-6219-4f80-b0b1-62ac861c9e92
📒 Files selected for processing (4)
mlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cppmlir/unittests/Dialect/QC/Translation/test_qasm3_translation.cppmlir/unittests/programs/qasm_programs.cppmlir/unittests/programs/qasm_programs.h
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
This generally looks very good to me as a direct port of the QuantumComputation QASM Importer 🙌
I only have a couple of inline comments or requests for changes.
We should be able to merge this quickly.
A thought that came up while reviewing this: how much is the current architecture of the parser and the visitor implementation limiting us in terms of expanding the capabilities?
There are quite a few things from the OQ3 spec that we would want to support (loops being one of the most prominent ones).
Would it be interesting to define our own OpenQASM3 dialect (inspired by the qe-compiler one) that can directly parse in QASM files. And then we would write a conversion to MLIR standard dialects plus QC. Does that make sense? Is that overkill? To me it feels like this could generally make sense. What do you think?
There was a problem hiding this comment.
Since none of this depends on the old MQT Core IR anymore, I am wondering whether we should split the translation target into to separate ones, where the OpenQASM3 one no longer links against MQT::CoreIR.
This would mean that mqt-cc as an executable has no direct connection to the old IR anymore.
This is something that can be done here.
If we manage to (somehow) find a way to interface with Qiskit without the MQT Core IR, we may finally be able to disable RTTI and Exception handling for the MLIR part of the code base.
I suppose the easiest way to get close to feature parity would be to expose the QCProgramBuilder to Python and replicate the existing Qiskit conversion through that API.
This is something to be done in a separate PR 😌
There was a problem hiding this comment.
Since none of this depends on the old MQT Core IR anymore, I am wondering whether we should split the translation target into to separate ones, where the OpenQASM3 one no longer links against MQT::CoreIR.
This would mean thatmqt-ccas an executable has no direct connection to the old IR anymore.
This is something that can be done here.
Good point! Done in 86bbf4b.
If we manage to (somehow) find a way to interface with Qiskit without the MQT Core IR, we may finally be able to disable RTTI and Exception handling for the MLIR part of the code base.
I suppose the easiest way to get close to feature parity would be to expose the QCProgramBuilder to Python and replicate the existing Qiskit conversion through that API.
This is something to be done in a separate PR 😌
I'll keep that in mind!
| [[nodiscard]] OwningOpRef<ModuleOp> translateQASM3ToQC(MLIRContext* context, | ||
| std::istream& input); |
There was a problem hiding this comment.
IIRC LLVM ships its own streams and recommends against using the stdlib ones. Worth checking.
There was a problem hiding this comment.
I went with llvm::SourceMgr in a7a81b9. This is also used in Translation.h, which we should look into at some point.
Let me know what you think!
| void | ||
| visitInitialLayout(std::shared_ptr<qasm3::InitialLayout> layout) override { | ||
| throw qasm3::CompilerError("InitialLayout pragmas are not supported.", | ||
| layout->debugInfo); | ||
| } | ||
|
|
||
| void visitOutputPermutation( | ||
| std::shared_ptr<qasm3::OutputPermutation> perm) override { | ||
| throw qasm3::CompilerError("OutputPermutation pragmas are not supported.", | ||
| perm->debugInfo); | ||
| } |
There was a problem hiding this comment.
I think these should not be errors. The pragmas are informational for the most part. They are otherwise simply comments.
Just skipping them is fine.
| if (stmt->thenStatements.empty()) { | ||
| throw qasm3::CompilerError( | ||
| "If statements with empty then blocks are not supported.", | ||
| stmt->debugInfo); | ||
| } |
| } | ||
|
|
||
| if (it == gates.end()) { | ||
| throw qasm3::CompilerError("No OpenQASM 3 definition found for gate '" + |
There was a problem hiding this comment.
| throw qasm3::CompilerError("No OpenQASM 3 definition found for gate '" + | |
| throw qasm3::CompilerError("No definition found for gate '" + |
There was a problem hiding this comment.
This is actually semi-intentional to be in contrast with this error message:
core/mlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cpp
Lines 682 to 683 in 77043ba
I will think about this a bit and respond tomorrow! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 13-26: The function translateQASM3ToQC uses OwningOpRef<ModuleOp>
as its return type but the header does not directly include the definition of
OwningOpRef, relying instead on transitive includes from BuiltinOps.h. Add
`#include` <mlir/IR/OwningOpRef.h> to the include section at the top of the file
along with the other mlir includes to make this header self-contained and
explicit about its dependencies.
🪄 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: 4b5a8e6b-daab-46b3-96dc-c6efd737f14b
📒 Files selected for processing (7)
mlir/include/mlir/Dialect/QC/Translation/TranslateQASM3ToQC.hmlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cppmlir/tools/mqt-cc/CMakeLists.txtmlir/tools/mqt-cc/mqt-cc.cppmlir/unittests/Dialect/QC/Translation/test_qasm3_translation.cppmlir/unittests/programs/qasm_programs.cppmlir/unittests/programs/qasm_programs.h
💤 Files with no reviewable changes (1)
- mlir/tools/mqt-cc/CMakeLists.txt
3 participants
Description
This is a rebased version of #1671 after #1671 was automatically closed when #1751 was merged.
Original description by @J4MMlE:
Checklist
I have updated the documentation to reflect these changes.I have added migration instructions to the upgrade guide (if needed).