-
Notifications
You must be signed in to change notification settings - Fork 1
QASM2 to squin #644
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
base: main
Are you sure you want to change the base?
QASM2 to squin #644
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
|
Used Callgraph pass after talking to Phil, I originally had a pretty funky rewrite rule to handle subkernels 😅 |
|
Latest round of feedback (from Phil): use dictionary style mapping (faster than pattern matching) + can use pre-existing QASM2 to py expr rewriterule. Should also try to avoid multi-layer matching logic, easier to reason through/test smaller rules. |
david-pl
left a comment
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.
Overall looks good and the tests are quite extensive, which is great. I have some questions and small comments.
| else: | ||
| return RewriteResult() | ||
|
|
||
| new_stmt = func.Invoke( |
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 feel like you could just do this within the if above, since the args have to match the node type anyway.
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.
something like
if isinstance(node, (core_stmts.QRegGet, core_stmts.QRegNew, core_stmts.Reset):
node.replace_by(func.Invoke(node.args, callee = callees[type(note))
else:
return RewriteResult()|
|
||
| def rewrite_Statement(self, node: ir.Statement) -> RewriteResult: | ||
|
|
||
| if isinstance(node, glob.UGate): |
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.
Again, this seems a bit redundant: you might as well just assign squin_equivalent_stmt in each case here rather than using the map above.
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.
The dictionary was something that @weinbe58 recommended, I could be doing the pattern wrong here but in cases where I do see a dictionary used it only turns out nice if the attribute you're accessing exits across all the statements.
Like for arithmetic operation conversion, you'll always have an lhs and rhs attribute. Here the number and kinds of attributes change.
I actually realize if I wanted to be clever I could add some strings to the values in the dictionary and then __getattribute__ things which would resolve the clunkiness at the expense of making things a little uglier.
Wish I could still do pattern matching but I'm told the performance would take a hit
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.
you do not need to unpack the arguments, unless they do not match the signature of the standard library function in squin but I think we have been consistent on this front so you should not need to do this @johnzl-777 see my comment above.
| return RewriteResult() | ||
|
|
||
| squin_noise_stmt = NOISE_TO_SQUIN_MAP[type(node)] | ||
| invoke_stmt = func.Invoke( |
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.
If you know that you are going for the broadcast version, why not just rewrite to the statement directly instead of adding an invoke to the stdlib?
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.
see the comment above
weinbe58
left a comment
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.
Overall looks good I just have some small updates to the code to reduce the number of special cases.
In general you could wrap all the "easy" rewrites with statements that match the stdlib signature exactly into a single rewrite pass that using one master dictionary to map the statement to their equivalent stdlib function and then handle the special cases like U1 and U2 in a separate rewrite. as their own separate rewrites and that might be a bit cleaner then spreading out the rewrites over
| else: | ||
| return RewriteResult() | ||
|
|
||
| new_stmt = func.Invoke( |
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.
something like
if isinstance(node, (core_stmts.QRegGet, core_stmts.QRegNew, core_stmts.Reset):
node.replace_by(func.Invoke(node.args, callee = callees[type(note))
else:
return RewriteResult()|
|
||
| def rewrite_Statement(self, node: ir.Statement) -> RewriteResult: | ||
|
|
||
| if isinstance(node, glob.UGate): |
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.
you do not need to unpack the arguments, unless they do not match the signature of the standard library function in squin but I think we have been consistent on this front so you should not need to do this @johnzl-777 see my comment above.
| return RewriteResult() | ||
|
|
||
| squin_noise_stmt = NOISE_TO_SQUIN_MAP[type(node)] | ||
| invoke_stmt = func.Invoke( |
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.
see the comment above
| if isinstance(node, (uop_stmts.RX, uop_stmts.RY, uop_stmts.RZ)): | ||
| args = (node.theta, node.qarg) | ||
| elif isinstance(node, (uop_stmts.UGate)): | ||
| args = (node.theta, node.phi, node.lam, node.qarg) | ||
| elif isinstance(node, (uop_stmts.U1)): | ||
| zero_stmt = py.Constant(value=0.0) | ||
| zero_stmt.insert_before(node) | ||
| args = (zero_stmt.result, zero_stmt.result, node.lam, node.qarg) | ||
| elif isinstance(node, (uop_stmts.U2)): | ||
| half_pi_stmt = py.Constant(value=pi / 2) | ||
| half_pi_stmt.insert_before(node) | ||
| args = (half_pi_stmt.result, node.phi, node.lam, node.qarg) | ||
| else: | ||
| return RewriteResult() |
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.
| if isinstance(node, (uop_stmts.RX, uop_stmts.RY, uop_stmts.RZ)): | |
| args = (node.theta, node.qarg) | |
| elif isinstance(node, (uop_stmts.UGate)): | |
| args = (node.theta, node.phi, node.lam, node.qarg) | |
| elif isinstance(node, (uop_stmts.U1)): | |
| zero_stmt = py.Constant(value=0.0) | |
| zero_stmt.insert_before(node) | |
| args = (zero_stmt.result, zero_stmt.result, node.lam, node.qarg) | |
| elif isinstance(node, (uop_stmts.U2)): | |
| half_pi_stmt = py.Constant(value=pi / 2) | |
| half_pi_stmt.insert_before(node) | |
| args = (half_pi_stmt.result, node.phi, node.lam, node.qarg) | |
| else: | |
| return RewriteResult() | |
| if type(node) not in PARAMETRIZED_1Q_GATES_TO_SQUIN_MAP: | |
| return RewriteResult() | |
| args = node.args | |
| if isinstance(node, (uop_stmts.U1)): | |
| zero_stmt = py.Constant(value=0.0) | |
| zero_stmt.insert_before(node) | |
| args = (zero_stmt.result, zero_stmt.result) + args | |
| elif isinstance(node, (uop_stmts.U2)): | |
| half_pi_stmt = py.Constant(value=pi / 2) | |
| half_pi_stmt.insert_before(node) | |
| args = (half_pi_stmt.result, ) + args | |
This is my implementation of #540 , there is one thing that I've held off on after talking with @weinbe58 that's a bit tricky: handling measurements. My understanding is we don't need it for now but in the future it will be relevant.
While 95% of qasm2 maps very nicely to existing squin infrastructure (with the exception of some of the less common 2 and 3 qubit gates I had to skip - unless we want to do some implicit conversion for them) you run into problems trying to map the qasm2 measurement semantics.
Take the following examples:
You could potentially just have creg be an empty ilist and then when the measure happens produce the ilist of measurements - making sure that all subsequent references point to the new ilist. This seems acceptable but it gets tricky if a user wants to do something like
Now the classical register somehow has to already be
get_item-able which is tricky considering there's no notion of a fixed-length list. Things get even trickier if a user wants to save at a different index:P.S. The sub kernel rewrite seems "off" to me, I'm sure there must be a better way to do it but I'm not aware of what the proper API for it is 😅