Auxiliary qubit tracking in HighLevelSynthesis
#12911
Conversation
Co-authored-by: AlexanderIvrii <alexi@il.ibm.com>
Cryoris
requested review from
alexanderivrii,
ShellyGarion and
a team
as code owners
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 10370210258Details
💛 - Coveralls |
There was a problem hiding this comment.
Overall, LGTM Great job on the implementation and the thorough testing.
However, I noticed that it fails a lint test. This should be fixed by removing the unused import in clifford_decompose_bm.py.
Additionally, there were several sections in high_level_synthesis.py that seemed to have outdated comments. It would be helpful to update these to maintain clarity.
I also left some suggestions for small improvements to qubit_tracker.py that could enhance its efficiency. These changes involve optimizing set operations and membership testing, which should help improve the performance of the qubit tracking.
Once these minor issues are addressed, it should be good to merge!
| @@ -66,6 +66,7 @@ def transpile( # pylint: disable=too-many-return-statements | |||
| optimization_method: Optional[str] = None, | |||
| ignore_backend_supplied_default_methods: bool = False, | |||
| num_processes: Optional[int] = None, | |||
| qubits_initially_zero: bool = True, | |||
There was a problem hiding this comment.
I remember we talked about the variable naming earlier today. This works well, but if you want to try out a shorter name, I think qubits_zeroed works here :)
There was a problem hiding this comment.
nice idea, or maybe qubits_reset if we want to avoid the "zero" 🤔
There was a problem hiding this comment.
Oh yes, I like that suggestion. It is clear and avoids any confusion with the term "zero" :)
There was a problem hiding this comment.
FWIW, personally I like the current name qubits_initially_zero a bit more than qubits_reset or qubits_zeroed since it's more clear what it means.
| @deprecate_arg( | ||
| "use_qubit_indices", | ||
| since="1.3", | ||
| additional_msg="The qubit indices will then always be used.", | ||
| pending=True, | ||
| ) |
There was a problem hiding this comment.
I am still trying to figure out whether we need use_qubit_indices or not. The motivation was to provide the context whether HLS runs before or after layout/routing. In the default transpiler flow, it runs before, i.e. on the logical circuit. Note that HLS has access to target/coupling_map but (as no layout/routing has been done) it does not know the mapping between logical and physical qubits. In addition, most of the available synthesis plugins do not take the coupling map into account, e.g., all clifford synthesis methods synthesizes cliffords implicitly assuming the all-to-all connectivity. However, in some cases we might want to run HLS after layout/routing and we have a few internal and external plugins that ensure adherence to the coupling map (e.g., the token swapper plugin, or the external sat-based synthesis plugins for cliffods).
Hmm... I think the only problematic usecase would be if we wanted to run say token swapper on the logical circuit, in which case it would wrongly assume that it has to adhere to the target connectivity and introduce more logic (that would later be routed potentially introducing even more swaps).
There was a problem hiding this comment.
Summarizing the offline discussion: let's keep it and if use_qubit_indices=False, then we assume we cannot query the target whether a gate is supported. Should we rename this to physical_qubits as well to make it clearer that the circuit has been mapped to physical qubits already?
There was a problem hiding this comment.
Thanks @Cryoris for taking over and greatly improving the PR. I have left a few additional questions related to potential edgecases which I am not sure might or might not happen.
There was a problem hiding this comment.
Thank you for changes in the first round of reviews. I've looked over the updates, and everything appears to be in order. Great work!
I noticed that Sasha has also left some insightful comments. Once those are addressed, I believe we'll be in a good position to merge the changes
There was a problem hiding this comment.
Thanks for addressing all of my review comments and adding tests for different cornercases (plugin mechanism returning None, having clbits).
The only remaining minor nitpick is that _synthesize_operation should return a tuple (consisting of the synthesized object and used qubits), can you please update the signature of this function accordingly?
There was a problem hiding this comment.
Thanks Julien! I am now fully happy with this PR. @henryzou50, please take another look and see if it can be merged.
ShellyGarion
added
the
Changelog: New Feature
Summary
Enable tracking the state of auxiliary qubits in high-level synthesis. This allows synthesis plugins to leverage additional qubits in a decomposition, leading to a potential reduction of circuit depth and number of operations (which is why I tagged it for the performance demo 🙂). This PR unblocks work on the circuit library, in particular refactoring of the MCX and MCMT gates.
Details and comments
This first implementation follows a greedy approach, where each operation is given access to all currently available auxiliary qubits. It does so by using the newly added$|0\rangle$ ) or "dirty" (an unknown state). Generally, if qubits start out as clean, and as soon as it is involved in an operation, it changes to dirty (special rules apply for operations like resets, barriers or delays).
QubitTracker, which keeps track of which qubits in the circuit/dag are "clean" (in stateAnother important change is that synthesized blocks must be synthesized again. This recursion is necessary, since unrolling or decomposing an operation, could yield other operations that must be synthesized. This slows down the code compared to the current state, but only by another iteration over the DAG, so that should be a reasonably small overhead.
Follow-ups could involve:
CircuitDataor an iterator over circuit instructions)The asv utility benchmark finds that this does not significantly reduce performance.
<\details>