✨ Translate for and while from OpenQASM to QC - #1862
Conversation
denialhaag
added
enhancement
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
2f7d2d3 to
f29d3b7
Compare
f29d3b7 to
7e379f5
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds standalone OpenQASM 3 lexer, parser, and QC MLIR emitter components. Translation now delegates through the new pipeline, with support and tests for loops, conditions, expressions, broadcasting, measurements, and source diagnostics. ChangesQASM3 translation pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SourceMgr
participant TranslateQASM3ToQC
participant Lexer
participant Parser
participant QCEmitter
participant QCProgramBuilder
SourceMgr->>TranslateQASM3ToQC: provide QASM3 source
TranslateQASM3ToQC->>Lexer: tokenize source
Lexer-->>Parser: return tokens
Parser->>QCEmitter: dispatch parsed statements
QCEmitter->>QCProgramBuilder: emit QC MLIR operations
QCProgramBuilder-->>QCEmitter: return constructed values
QCEmitter-->>TranslateQASM3ToQC: return finalized module
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@include/mqt-core/qasm3/DebugInfo.hpp`:
- Around line 20-36: Add Doxygen-style comments for the new DebugInfo struct,
its constructor, and toString() in DebugInfo.hpp so the purpose of DebugInfo,
each constructor parameter (including parentDebugInfo), and the return value of
toString() are documented; place the comments directly above the DebugInfo
declaration, the DebugInfo constructor, and the toString() method.
- Around line 26-29: The DebugInfo constructor is using a redundant double
std::move on the filename parameter, which should be cleaned up to satisfy
clang-tidy. Update the DebugInfo initializer list so the filename member is
initialized with a single move from the constructor argument, and leave the
parentDebugInfo handling unchanged.
In `@mlir/lib/Dialect/QC/Translation/QASM3Parser.cpp`:
- Around line 593-630: The qubit and classical register declaration parsers
duplicate the same size-parsing, declaration registration, and allocation logic
across parseQubitDeclaration()/parseQregDeclaration() and
parseBitDeclaration()/parseCregDeclaration(). Extract shared helpers such as
declareQubitRegister and declareClassicalRegister, then have these parser
methods delegate to them while preserving the existing keyword/operand ordering
differences so the behavior stays consistent and easier to maintain.
- Around line 853-903: parseFor() currently forwards any parsed step into scfFor
and always bumps the upper bound with stopVal + 1, but scf.for only accepts a
positive step. Add validation in parseFor() after computing stepVal (or before
lowering) to reject zero and negative steps with an error, and keep the existing
inclusive-stop adjustment only for valid ascending ranges. Use parseFor,
emitIntegerExpression, and builder.scfFor to locate the lowering path.
- Around line 400-407: Both QASM3Parser::error overloads always throw
CompilerError, so mark each error(...) helper as [[noreturn]] to make the
control flow explicit at bare call sites and improve static analysis. Update
both the Token-based and DebugInfo-based overloads in QASM3Parser.cpp, keeping
their behavior unchanged while adding the attribute to the function
declarations/definitions.
In `@mlir/unittests/Dialect/QC/Translation/test_qasm3_translation.cpp`:
- Around line 465-467: The test case name in QASM3TranslationTestCase is
misspelled as "NestedForLooWhilefOp"; update the string to
"NestedForLoopWhileOp" so the case is searchable and consistent with
qasm::nestedForLoopWhileOp and MQT_NAMED_BUILDER(nestedForLoopWhileOp).
🪄 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: cd38d502-c3a9-475e-b5c0-57cb0847777d
📒 Files selected for processing (11)
include/mqt-core/qasm3/DebugInfo.hppinclude/mqt-core/qasm3/Exception.hppinclude/mqt-core/qasm3/Statement.hppmlir/include/mlir/Dialect/QC/Translation/TranslateQASM3ToQC.hmlir/lib/Dialect/QC/Translation/CMakeLists.txtmlir/lib/Dialect/QC/Translation/QASM3Parser.cppmlir/lib/Dialect/QC/Translation/QASM3Parser.hmlir/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.
|
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Thanks @denialhaag for working on this! 🙏🏼
I pushed one small commit with some cosmetic fixed I stumbled over while reviewing.
As you will probably see through the reviews, I am not yet quite happy where this is at. Overall, the current solution almost feels more constrained than the previous solution and still does not quite naturally fit to MLIR. I believe we should be taking much more advantage of MLIR built-in functionality for this.
It's also in one of the comments, but I'd like to highlight this here again: I am not 100% certain the approach of directly emitting the program while parsing really creates a nice design.
It mixes a couple too many things for my taste.
Maybe you can take all of the thoughts from the comments in this review back to the drawing board and re-evaluate what a good approach for all of this looks like.
b9c73a0 to
4981a09
Compare
4981a09 to
99fa5db
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 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/.clang-tidy`:
- Line 11: Remove the broad cppcoreguidelines-slicing suppression from the
mlir/.clang-tidy configuration and add a targeted NOLINT annotation at the
specific code triggering the known false positive. Keep the slicing check
enabled for the rest of the mlir/ subtree and follow the project’s existing
clang-tidy annotation style.
In `@mlir/include/mlir/Dialect/QC/Translation/qasm3/QASM3Parser.h`:
- Around line 844-854: Update the parameter parsing block to call
parseIdentifierList only when the token after the opening parenthesis is not
RParen, allowing empty gate parameter lists while preserving the existing
expect(TokenKind::RParen) validation.
- Around line 272-278: Update the parser’s symbol management around declare(),
parseBlock(), and parseFor() to use a lexical scope stack instead of one global
symbols map. Push a new scope when entering each block and pop it on exit so
declarations do not escape and inner declarations can shadow outer names; ensure
declare() checks/inserts only in the current scope. Register each parseFor()
iteration variable in the body’s scope before parsing the loop body so
assignments resolve correctly.
In `@mlir/include/mlir/Dialect/QC/Translation/TranslateQASM3ToQC.h`:
- Around line 28-40: Update the Doxygen `@return` documentation for both
translateQASM3ToQC overloads to state that translation failure may return
nullptr, while successful translation returns the QC module.
In `@mlir/lib/Dialect/QC/Translation/qasm3/QASM3Emitter.cpp`:
- Around line 395-425: Validate folded register sizes in qubitRegister and
classicalRegister before calling builder.allocQubitRegister or
builder.allocClassicalBitRegister, rejecting non-positive values with a source
diagnostic that uses loc. Ensure invalid sizes return failure and valid sizes
continue through the existing allocation paths.
- Around line 1367-1392: Update the Expr::Kind::Pow branch to use checked
exponentiation by squaring instead of a loop proportional to **rhs. Bound the
exponent so excessively large user-controlled values are rejected, and detect
overflow in every intermediate multiplication, returning error(expr.loc, ...)
rather than invoking signed-overflow undefined behavior.
- Around line 557-572: Update ifBegin in QCProgramBuilder so entering the
scf::IfOp then region also synchronizes the builder’s private regionStack. Use
an existing builder region-scope API if available, or add a safe API that pushes
the region while preserving the IfScope lifetime and restores it when the scope
exits; do not only call setInsertionPointToStart. Ensure dynamic register
indexing inside the branch observes the correct region depth in memrefLoad.
- Around line 336-392: Prevent assignBool, assignInt, and assignFloat from
inserting branch-local SSA values directly into the global booleanValues,
intValues, or floatValues tables when called inside if, while, or for control
flow. Lower such assignments through structured-control-flow results or loop
iter_args so values are merged and carried correctly, or reject assignments to
variables defined outside the active control-flow region; apply the same
handling to the control-flow emission logic covering the referenced later range.
- Around line 257-284: Update finalize() and the bare-register lookup path to
validate every accessed or returned slot in info.bits, not merely the vector
length. Reject any register containing a missing/null Value, including gaps such
as an unmeasured lower-index bit, before appending or returning values. Preserve
the existing output-register error handling and valid measured-bit behavior.
- Around line 341-382: Update the dynamic-load cache key construction and lookup
in the relevant QASM3 emitter logic to use the resolved SSA index value or
version, rather than the source expression spelling. Ensure reassignment through
assignInt and related integer-binding updates cannot reuse a cached load created
for an earlier value, while preserving reuse when the resolved index remains
unchanged.
- Around line 1466-1471: Update the parse/finalization flow around
parser.parseProgram() and emitter.finalize() to return nullptr immediately when
status indicates failure, before calling finalize(). Only finalize the emitter
after successful parsing, while preserving the existing mod failure check and
null return.
- Around line 598-652: Update the loop-lowering logic around
tryEvaluateIntConstant and builder.scfFor so dynamically resolved steps are
guaranteed strictly positive before constructing scf.for. Preserve the existing
static-negative countdown lowering, and for unknown or runtime non-positive
steps either emit a runtime validation/error guard or normalize them through an
equivalent dynamic countdown path rather than passing them directly as the
scf.for step.
- Around line 897-908: Update resolveStandardGate to validate gate.nTargets
against the supplied target operands before returning the dispatch entry,
rejecting both missing and extra targets with the existing gate error pattern.
Keep the parameter-count validation and successful GATE_DISPATCH lookup behavior
unchanged.
In `@mlir/lib/Dialect/QC/Translation/qasm3/QASM3Lexer.cpp`:
- Around line 114-141: The skipTrivia() block-comment path must detect when
scanning reaches end without finding */ instead of silently allowing next() to
return Eof. Preserve an unterminated-comment error state or make next() return
an Error token, while retaining normal trivia skipping for properly terminated
block comments.
- Around line 157-194: Update next() so a period followed by a digit is routed
to lexNumber(), alongside digit-prefixed tokens, allowing leading-dot literals
such as .5 to be recognized as FloatLiteral while preserving existing handling
for standalone periods.
- Around line 196-213: Update Lexer::lexString to accept the opening quote
delimiter and terminate on that same delimiter, preserving unterminated-string
error handling. In the include-string lexing dispatch, recognize and pass both
single-quote and double-quote delimiters so inputs such as single-quoted
includes produce StringLiteral tokens.
In `@mlir/unittests/Dialect/QC/Translation/test_qasm3_translation.cpp`:
- Around line 721-727: Add the missing forLoopOffsetIndex fixture to the // ---
ForOp --- // test suite in QASM3TranslationTestCase, pairing
qasm::forLoopOffsetIndex with its matching MQT_NAMED_BUILDER reference builder.
🪄 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: 6afd7c09-7460-41d8-9733-eb8874f34d2e
📒 Files selected for processing (18)
include/mqt-core/qasm3/DebugInfo.hppinclude/mqt-core/qasm3/Exception.hppinclude/mqt-core/qasm3/Statement.hppmlir/.clang-tidymlir/include/mlir/Dialect/QC/Builder/QCProgramBuilder.hmlir/include/mlir/Dialect/QC/Translation/TranslateQASM3ToQC.hmlir/include/mlir/Dialect/QC/Translation/qasm3/QASM3Emitter.hmlir/include/mlir/Dialect/QC/Translation/qasm3/QASM3Lexer.hmlir/include/mlir/Dialect/QC/Translation/qasm3/QASM3Parser.hmlir/lib/Dialect/QC/Builder/QCProgramBuilder.cppmlir/lib/Dialect/QC/Translation/CMakeLists.txtmlir/lib/Dialect/QC/Translation/TranslateQASM3ToQC.cppmlir/lib/Dialect/QC/Translation/qasm3/QASM3Emitter.cppmlir/lib/Dialect/QC/Translation/qasm3/QASM3Lexer.cppmlir/tools/mqt-cc/mqt-cc.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.
Actionable comments posted: 2
🤖 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/qasm3/QASM3Parser.h`:
- Around line 761-764: The assignment validation around
scopes.back().contains(id) incorrectly rejects writes to variables declared in
enclosing scopes. Remove this rejection and propagate enclosing-scope values
through the sink/SCF region arguments so assignments in while, if, and for
bodies update the original variable correctly.
In `@mlir/lib/Dialect/QC/Translation/qasm3/QASM3Emitter.cpp`:
- Around line 573-581: Extend DeclarationScope and the QCEmitter control-flow
handling so qubitRegister() and classicalRegister() state is saved and restored
alongside bindings and loadedQubits. Ensure declarations made inside scf.if,
scf.for, or scf.while remain scoped to that region and cannot affect later
lookups or finalize(); alternatively reject block-local register declarations
consistently.
🪄 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: 2e5f9002-046b-4e93-9b61-05aad7f5fe61
📒 Files selected for processing (10)
include/mqt-core/qasm3/Exception.hppinclude/mqt-core/qasm3/Statement.hppmlir/include/mlir/Dialect/QC/Translation/TranslateQASM3ToQC.hmlir/include/mlir/Dialect/QC/Translation/qasm3/QASM3Emitter.hmlir/include/mlir/Dialect/QC/Translation/qasm3/QASM3Lexer.hmlir/include/mlir/Dialect/QC/Translation/qasm3/QASM3Parser.hmlir/lib/Dialect/QC/Translation/qasm3/QASM3Emitter.cppmlir/lib/Dialect/QC/Translation/qasm3/QASM3Lexer.cppmlir/unittests/Dialect/QC/Translation/test_qasm3_translation.cppmlir/unittests/programs/qasm_programs.h
💤 Files with no reviewable changes (1)
- mlir/unittests/programs/qasm_programs.h
| if (!scopes.back().contains(id)) { | ||
| return sink.error(loc, "cannot assign to '" + id + | ||
| "' from inside a block; it is declared in an " | ||
| "enclosing scope"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Allow assignments to enclosing-scope variables inside nested control-flow bodies. This rejects valid updates like while (...) { i = i + 1; } and assignments in if/for blocks. Thread these values through the sink/SCF region arguments instead of treating every outer-scope write as illegal.
🤖 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/include/mlir/Dialect/QC/Translation/qasm3/QASM3Parser.h` around lines
761 - 764, The assignment validation around scopes.back().contains(id)
incorrectly rejects writes to variables declared in enclosing scopes. Remove
this rejection and propagate enclosing-scope values through the sink/SCF region
arguments so assignments in while, if, and for bodies update the original
variable correctly.
There was a problem hiding this comment.
@burgholzer, I would move the point @coderabbitai is raising here to a follow-up. I'm not sure how our conversions and the builders would deal with this at the moment. 🤔
There was a problem hiding this comment.
@denialhaag, that sounds reasonable — deferring this to a follow-up avoids scope creep given the uncertainty around how the SCF region/builder plumbing would need to change to support enclosing-scope mutations. Let me know if you'd like me to open a follow-up issue to track this (referencing this PR and comment), and I'll take care of it.
There was a problem hiding this comment.
@coderabbitai hm. I'll look into this as part of my review. at a quick glance, this feels a bit like an arbitrary limitation to be honest.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
It is a limitation, but I don't think it's a limitation of the translation. The QCProgramBuilder currently cannot create a while loop that modifies a classical variable. For this to be necessary, we would need to add block arguments and respective yields. In addition, all conversions that involve scf operations would not respect a classical block argument at the moment. This is all fixable, but I probably wouldn't do it as part of this PR.
There was a problem hiding this comment.
Would we? SCF while regions are not isolated from above, aren't they?
I am probably just missing something here. I'll look at this with fresh eyes in the coming days.
There was a problem hiding this comment.
No, the regions are not isolated from above. That said, I think you need to thread any values you are updating through the region arguments anyway. For example, see this example from the MLIR docs:
func.func @reduce(%buffer: memref<1024xf32>, %lb: index,
%ub: index, %step: index) -> (f32) {
// Initial sum set to 0.
%sum_0 = arith.constant 0.0 : f32
// iter_args binds initial values to the loop's region arguments.
%sum = scf.for %iv = %lb to %ub step %step
iter_args(%sum_iter = %sum_0) -> (f32) {
%t = load %buffer[%iv] : memref<1024xf32>
%sum_next = arith.addf %sum_iter, %t : f32
// Yield current iteration sum to next iteration %sum_iter or to %sum
// if final iteration.
scf.yield %sum_next : f32
}
return %sum : f32
}There was a problem hiding this comment.
Okay, I'm definitely getting too tired. Reassignment obviously isn't a thing in MLIR. A reassignment in OpenQASM corresponds to the definition of a new variable in MLIR. If this new variable is defined within a region, it would definitely need to be yielded to be accessible outside.
|
Hey @denialhaag 👋🏼 I let GPT-5.6-Codex review the PR and compare it against the OpenQASM 3 live specification. Below is the improvement plan it came up with. To me, this makes a lot of sense. If this direction is okay with you, I’d task Codex with implementing the stabilizations for this PR and then continue the broader work in follow-up PRs. Improve PR #1862 into a specification-driven OpenQASM 3 frontendSummaryPR #1862 has a strong foundation: zero-copy lexing, However, it is not yet suitable for the stated “arbitrary OpenQASM 3” goal. The custom parser implements only a narrow subset of the official grammar, while syntax, semantic analysis, and MLIR emission remain interleaved despite the sink abstraction. This causes rejected valid programs, accepted invalid syntax, target-specific parser restrictions, and some paths that can produce invalid MLIR. Use a staged delivery:
Target stable OpenQASM 3.0 and 3.1 with version-gated behavior. Track later live-spec additions explicitly. Review ConclusionsMerge-blocking correctness issues
Important coverage gapsThe current frontend is missing substantial language families documented by the live language specification:
Architecture assessmentKeep these strengths:
Replace these weaknesses:
Implementation Changes1. Stabilize #1862 before merge
2. Introduce a specification-driven source stage
3. Lower typed OpenQASM IR using MLIR infrastructure
4. Public interface additionsKeep both existing overloads and add an options overload without breaking callers:
5. Performance work
Test Plan
Assumptions
|
|
The stability improvements generally look good to me. I only have two comments:
Now to the bigger part: If we really want to go with an Generally speaking, I don't really know how to deal with all of this efficiently. Doing too many LLM-generated refactors is very annoying. Given the size this PR is already at, I spent way too long reading and streamlining LLM-generated code. I agree that the result is still not ideal, and we should improve upon it. I might not be having a point, but I guess I want to say that we should be really sure what we want this to look like. Any LLM-generated refactor is another start from zero. I would really like to get to a point where we have a stable foundation we know and can easily work with. |
Let me try the following: I'll hand the full rewrite from the plan above to Codex and let it create a new draft PR showing how the result would look like. Based on that, we can (hopefully) better judge whether that is a direction worth pursuing. |
|
Two more things that just came to mind and can maybe be thrown into Codex as well:
|
2 participants
Description
This PR adds support for translating
forandwhilefrom OpenQASM to QC. In doing so, this PR also completely refactors the translation. Instead of relying onqasm3::InstVisitor, the new version of the translation dispatches onqasm3::Token. While more manual, the new implementation is lighter and less tied to legacy decisions.Fixes #1846
AI notice: I have used Opus 4.8 via Claude Code for a first version of the changes. I have then iterated on this version (largely manually, sometimes with further help of Opus 4.8).
Checklist
I have updated the documentation to reflect these changes.I have added migration instructions to the upgrade guide (if needed).If PR contains AI-assisted content:
AI-assisted commits include anAssisted-by: [Model Name] via [Tool Name]footer.