-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Support standalone Var
throughout transpiler
#12322
Conversation
One or more of the the following people are requested to review this:
|
d625cba
to
6b5a829
Compare
Pull Request Test Coverage Report for Build 8923296656Details
💛 - Coveralls |
This adds the missing pieces to fully support standalone `Var` nodes through every part of the transpiler (that I could detect). It's quite possible there's some problem in a more esoteric non-preset pass somewhere, but I couldn't spot them. For the most part there were very few changes needed to the actual passes, and only one place in `QuantumCircuit` that had previously been missed. Most of the commit is updating passes to correctly pass `inline_captures=True` when appropriate for dealing with `DAGCircuit.compose`, and making sure that any place that needed to build a raw `DAGCircuit` for a rebuild _without_ using `DAGCircuit.copy_empty_like` made sure to correctly add in the variables. This commit adds specific tests for every pass that I touched, plus the general integration tests that we have for the transpiler to make sure that OQ3 and QPY serialisation work afterwards.
6b5a829
to
1731cd9
Compare
Now rebased over #12308 (speculatively assuming the merge-queue commit passes CI). |
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.
This LGTM, this was much easier to integrate than I expected. Before we merge though I had one question about treating store as a directive for backends that don't have any dynamic circuits capabilities and won't support Vars at all.
# This is a compiler/backend intrinsic operation, separate to any quantum processing. | ||
_directive = True |
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'm fine with this, but this basically means we're treating this as always supported through the transpiler regardless of what the target says. Is that the correct behavior for this? I'm mostly wondering for backends that don't have any classical logic support, and how we intend to indicate this to backends. For other control flow operations they have to be explicitly in the target to work. Is the answer just documentation?
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.
Short of adding more magic fields to supported_instruction
Target
, which could never really capture all the constraints of dynamic-circuits requirements already, or some sort of global flag on Target
/BackendV2
that we haven't really prepared to put in place, I think documentation is the answer.
Dynamic circuits have always been tricky through these tests, because there's so much that's never really been possible to represent as a restriction to the transpiler (nesting can only be to n
levels, max of m
blocks, switches must have a default
, etc etc).
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.
Let just go with documenting this for now as part of the 1.1.x final documentation, I'll convert this into an issue to track. Both as an upgrade note in the final release notes and also as a guidance in the writing a backend section of the docs.
I think for 1.2.x we can look at adding a global flag in the target for something like supports store, etc to provide a high level indicator for this kind of capability above the per instruction look ups. We'd probably need to do this into an opt-in way (so it defaults true) but I think having the capability to express this in the target is what we'll want longer term.
# TODO: There's not a huge amount we can sensibly test for the output here until we can | ||
# round-trip the OpenQASM 3 back into a Terra circuit. Mostly we're concerned that the dump | ||
# itself doesn't throw an error, though. |
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 could assert the stores are preserved and present in the qasm string independent of the transpilers optimization or something I guess. But yeah it probably just makes more sense to wait for the parser to gain support for this.
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.
This comment is basically just copied from the tests above from when we added the enhanced conditions to control flow.
I'll try closing and re-opening this to see if that triggers GH to recalculate the merge base. |
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.
This LGTM, I'm still mildly concerned about how we handling Store
for the target/backends, but I'm fine documenting it. Realistically I feel like this will be an uncommon use case in practice and the only people using it to start will know the backend is capable of supporting it. We can look at evolving our target model to better account for this in 1.2.0
* Support standalone `Var` throughout transpiler This adds the missing pieces to fully support standalone `Var` nodes through every part of the transpiler (that I could detect). It's quite possible there's some problem in a more esoteric non-preset pass somewhere, but I couldn't spot them. For the most part there were very few changes needed to the actual passes, and only one place in `QuantumCircuit` that had previously been missed. Most of the commit is updating passes to correctly pass `inline_captures=True` when appropriate for dealing with `DAGCircuit.compose`, and making sure that any place that needed to build a raw `DAGCircuit` for a rebuild _without_ using `DAGCircuit.copy_empty_like` made sure to correctly add in the variables. This commit adds specific tests for every pass that I touched, plus the general integration tests that we have for the transpiler to make sure that OQ3 and QPY serialisation work afterwards. * Clarify comment
Summary
This adds the missing pieces to fully support standalone
Var
nodes through every part of the transpiler (that I could detect). It's quite possible there's some problem in a more esoteric non-preset pass somewhere, but I couldn't spot them.For the most part there were very few changes needed to the actual passes, and only one place in
QuantumCircuit
that had previously been missed. Most of the commit is updating passes to correctly passinline_captures=True
when appropriate for dealing withDAGCircuit.compose
, and making sure that any place that needed to build a rawDAGCircuit
for a rebuild without usingDAGCircuit.copy_empty_like
made sure to correctly add in the variables.This commit adds specific tests for every pass that I touched, plus the general integration tests that we have for the transpiler to make sure that OQ3 and QPY serialisation work afterwards.
Details and comments
Close #10931.
This is the top of the stack and depends on #12215 and #12308 (the latter of which depends on #12307, though those could be merged together).
I'll write the full release note as a full PR after the tagging of rc1.