-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ OpenQASM 3.0 support #309
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some very early comments on the early draft.
It would be great to separate the generation of the parser from the actual library. Ideally the underlying grammar hardly changes and the Lexer and syntax tree construction remains rather constant.
As such, the parser generation should probably be hidden behind a CMake option (just as we hide the tests behind some CMake option).
The generation itself only needs to work on one particular system (probably Ubuntu, because it's easiest and the fastest).
It would be nice to have a separate CI check that can be used for checking whether everything still works. Maybe there even is some GitHub action for properly setting up ANTLr. Wouldn't be surprised. That CI check should probably also only run on changes to the respective files.
Of course, the generated parser should work under all systems. Maybe this setup becomes obsolete if we were to adopt QIR and the corresponding OQ3 parser. We'll have to see.
c8df7d1
to
ded8393
Compare
There remains some refactoring and cleanup to do; currently, |
Really just a comment since I briefly looked at the PR: I think we don't really need the explicit UTF-8 support via an external library. Another comment: try to factor out as much functionality as possible from the parser and try to put it into the Core library. Most importantly the modifiers. These can all happen in their own PR. Then this PR can be rebased and use the newly created functions 🙂 |
This now also allows including `qelib1.inc`, which will print a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey 👋🏼
I know you are still working on this, but I just had some time to kill at the airport and thought I'd give you some feedback. You should be able to use GitHub's batch commit feature (go to files changed and then add suggestions to commit batch) to resolve most of the suggestions/comments. Hope all of that makes sense.
Co-authored-by: Lukas Burgholzer <burgholzer@me.com>
Great, many thanks. Updated the list of linked issues accordingly. |
I've updated the description. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now also briefly looked over the parser code itself and it is looking good ✨
I just left one last comment where I would like to get your opinion on a couple of things. Those things need not necessarily happen as part of this PR if they turn out to be to complicated.
And I seem to have screwed up one of the tests with one of the changes above. Could you maybe fix that? 😅 |
If the parser detects an `OpenQASM 2.0` version declaration, `c` prefixes are treated as control modifiers. This allows parsing the following gate: ```qasm cccc x q[0], q[1], q[2], q[3], q[4]; // equivalent to ctrl(4) @ x q[0], q[1], q[2], q[3], q[4]; ```
The corresponding member was hardly maintained and even less frequently used. Might as well not bother keeping it.
…ason for not assuming these are supported natively in either of the two standards.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #309 +/- ##
=======================================
+ Coverage 90.4% 90.8% +0.3%
=======================================
Files 114 127 +13
Lines 12033 13850 +1817
Branches 2058 2176 +118
=======================================
+ Hits 10889 12583 +1694
- Misses 1144 1267 +123
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many many thanks again for this nice contribution! Great to see OpenQASM 3 support within the MQT finally happening 🎉
Looking forward to some future additions to enhance the support even further.
This PR replaces the existing OpenQASM 2.0 parser with a new OpenQASM 3.0 parser.
The new parser now builds a syntax tree, where type checking, constant evaluation, and translation to the Quantum circuit.
The parser can handle the following new features:
New Syntax
New syntax for declaring bits, qubit, measure operations. The old syntax (
creg
,qreg
) is still supported.Gate modifiers
Gate modifiers (
inv
,ctrl
, andnegctrl
) are now supported. This replaces thec
prefix.See the OpenQASM 3.0 specification for more information: https://openqasm.com/language/gates.html#quantum-gate-modifiers
Classical constant values
The parser now supports classical computation with constant values. This can be used to e.g. define the number of quantum registers.
Additionally, all features of the previous parser are still supported.
The big features from OpenQASM 3.0 still missing are:
pow
modifier (Support for the powering modifier of OpenQASM 3.0 #27)