✨ 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! |
denialhaag
force-pushed
the
improve-openqasm-translation
branch
from
2f7d2d3 to
f29d3b7
Compare
denialhaag
force-pushed
the
improve-openqasm-translation
branch
from
f29d3b7 to
7e379f5
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughAdds a standalone QASM3 parser for QC translation, moves ChangesDebugInfo Header Extraction
QASM3 Parser Rewrite with for/while Translation
Estimated code review effort: 4 (Complex) | ~75 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
| t["mcx"] = mcxInfo; | ||
| t["mcx_gray"] = mcxInfo; | ||
| t["mcx_vchain"] = mcxInfo; | ||
| t["mcx_recursive"] = mcxInfo; | ||
|
|
||
| t["mcphase"] = GateInfo{ | ||
| .nControls = 0, .nTargets = 0, .nParameters = 1, .type = ::qc::OpType::P}; |
There was a problem hiding this comment.
I am not the biggest fan of this kind of special handling. Can we get rid of this somehow and find a "proper" solution?
| const auto index = evaluateNonNegativeConstant(*bit.index, debugInfo); | ||
| if (index >= registerBits.size() || !registerBits[index]) { | ||
| error(*debugInfo, "Bit " + std::to_string(index) + " of register '" + | ||
| registerName + "' has been not measured yet."); | ||
| } | ||
| return registerBits[index]; |
There was a problem hiding this comment.
Hm.. I am beginning to more and more think that this is something that would be perfectly suitable for an MLIR verifier on an OpenQASM3 dialect.. or at least some kind of semantic analysis.
I am also not 100% sure whether it is really so clever to mix semantic analysis with the actual program emission like it is currently being done here.
From past conversations with LLMs, they mostly suggested to parse to an AST, then perform semantic analysis, then emit code. The current implementation kind of combines all three.
Feels like it may be worth to revisit this design decision.
| // Resolve parameters | ||
| SmallVector<Value> params; | ||
| params.reserve(call.parameters.size()); | ||
| for (const auto& arg : call.parameters) { | ||
| params.push_back(emitFloatExpression(arg, debugInfo)); | ||
| } |
There was a problem hiding this comment.
Parameters do not necessarily need to be floating point expressions, right? One could declare a custom gate that uses other kind or arguments for parameters, I suppose.
| // Inline compound gate | ||
| if (const auto* compound = std::get_if<ParsedCompoundGate>(&it->second)) { | ||
| emitCompoundGate(*compound, params, split.targets, split.posControls, | ||
| split.negControls, split.invert, debugInfo); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I believe this is another opportunity to improve the parser. There is no need to inline custom gate definitions.
We can simply materialize the definitions as proper func::func functions in the MLIR code and call the respective methods.
Inlining is something that can then happen in MLIR.
There was a problem hiding this comment.
I'd prefer to do this as part of #1659.
-
This would definitely break equivalence checking because the
QCProgramBuildercannot build the respective reference programs. -
I'm also pretty sure we'll need to implement additional canonicalization patterns. Consider, for example, this program:
OPENQASM 3.0; include "stdgates.inc"; qubit[3] q; gate compound q0, q1 { x q0; x q0; } ctrl(2) @ compound q[0], q[1], q[2];
If translated into a new function, it would likely be canonicalized to an empty function. Subsequently, the control modifier would need to check whether the called function is empty and then remove the call, unless this is already handled by built-in patterns.
-
I'm not 100 percent sure how our modifiers would handle function calls right now. They might be removed if their body does not contain a
UnitaryOpInterface. -
I'm all but certain that we could not convert a program with multiple functions to QCO (let alone QIR).
There was a problem hiding this comment.
Yeah, that makes sense. Maybe one can add a couple of notes there or point the issue to this discussion
| /// Statically evaluate `ParsedExpr` to an `size_t`. | ||
| [[nodiscard]] size_t evaluateNonNegativeConstant( | ||
| const ParsedExpr& expr, | ||
| const std::shared_ptr<DebugInfo>& debugInfo) const { | ||
| return static_cast<size_t>(evaluateIntegerConstant(expr, debugInfo)); | ||
| } |
There was a problem hiding this comment.
This kind of feels like something that should already happen in MLIR. I would have assumed we can have the translation to rather plainly emit the respective constructs and MLIR constant propagation and folding do the job for us.
There was a problem hiding this comment.
This function is only used in four cases:
- Translating
q[<index>]: I guess this could always be translated tomemref.loadoperations. There are some limitations inQCProgramBuilderthat prevent repeated loads. Plus, we currently do not allowmemref.loadoperations within modifiers. All of these points have a solution, but it could get nasty. - Translating
c[<index>]: Cannot be changed before #1726 is implemented. - Translating
ctrl(<number of controls>): I don't really think this can ever be a dynamic value. - Translating
pow<exponent>: This can be computed dynamically. I will change it.
There was a problem hiding this comment.
This function is only used in four cases:
- Translating
q[<index>]: I guess this could always be translated tomemref.loadoperations. There are some limitations inQCProgramBuilderthat prevent repeated loads. Plus, we currently do not allowmemref.loadoperations within modifiers. All of these points have a solution, but it could get nasty.
I see the nastiness. But this is exactly one of the examples for the point I was trying to raise in the other comment: indexed accessors are a standard thing in OpenQASM; so people will use them and tools will produce them in all kinds of places. We need to be able to handle those cases and find ways how these constructs can be represented well in our dialect.
Given how gate statements are parsed "as a whole" including the modifiers, it should be moderately straight forward to first compute the required qubit values and then do all the modifier handling and wrapping.
- Translating
c[<index>]: Cannot be changed before #1726 is implemented.
Another good reason for bumping the priority on that one 😌
- Translating
ctrl(<number of controls>): I don't really think this can ever be a dynamic value.
Yeah, probably not, otherwise the operand list would have to be dynamic as well.
- Translating
pow<exponent>: This can be computed dynamically. I will change it.
👍🏼
Co-authored-by: Lukas Burgholzer <burgholzer@me.com>
denialhaag
force-pushed
the
improve-openqasm-translation
branch
from
e11e3a7 to
3b1e39b
Compare
Sorry, I forgot to pull, and |
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.