Fix handling of conditions in SabreDAG creation #8727
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In Qiskit#8388 we moved all of the SabreSwap pass's processing into rust. To facilitate that we had to create a rust DAG data structure to represent the data flow graph solely in Rust to enable analyzing the DAG structure in rust code without having to callback to python. To do this the DAGCircuit (which has a nearly identical representation in python) would export a list of nodes and the bits (both quantum and classical) that they operated on. This information is then used to create the SabreDAG used in the rust code. However, in this process an edge case was missed with classical condtions. If a condition was specified solely as a property of the operation and not in cargs list the SabreDAG would not know about the classical bit dependency between any subsequent operations involving that classical bit. This would cause incorrect output because the ndoes would not get processed as they were in the circuit. This commit fixes this issue by explicitly checking if there is a condition on the operation and there are no cargs and if so adding the carg bits to the SabreDAG directly. This fixes the incorrect representation in SabreDAG. Fixes Qiskit#8675
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 3040977407
💛 - Coveralls |
jakelishman
reviewed
Sep 12, 2022
This commit fixes the logic for handling instructions that have both cargs and conditions set. The previous commit fixed the behavior only if cargs was mutually exclusive with having classical arguments. However, the same bug this PR is fixing would persist for instructions with clbit arguments and a condition. This fixes the behavior to correctly represent the data dependency in SabreDAG for these cases.
jakelishman
reviewed
Sep 12, 2022
This commit improves the efficiency of the condition handling on SabreDAG creation that was added in the previous commit. Previously, it was potentially expensive to loop over the list of cargs and condition bits as for instructions with a large number of both the repeated duplicate searches would get quite expensive. This commit fixes that by converting the cargs data structure to be a set to prevent adding duplicates. Since the order isn't significant for the cargs in sabre as it's only used to represent data dependency between instructions we can convert it to internally use a set in python and a HashSet in rust. This also removes storing of cargs for each node in the SabreDAG as this was never used and just wasted memory by storing the list and never using it.
jakelishman
approved these changes
Sep 15, 2022
| let mut dag: DiGraph<(usize, Vec<usize>, Vec<usize>), ()> = | ||
| let mut dag: DiGraph<(usize, Vec<usize>), ()> = |
There was a problem hiding this comment.
If we don't need to track clbit order within the SabreDAG (which I think you're right - we don't need to), I don't think we need to track qubit order either - it's not important for the Sabre algorithm. That said, it's not really worth bothering about, and if doing it becomes a memory/performance concern in the future we can revisit it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
In #8388 we moved all of the SabreSwap pass's processing into rust. To facilitate that we had to create a rust DAG data structure to represent the data flow graph solely in Rust to enable analyzing the DAG structure in rust code without having to callback to python. To do this the DAGCircuit (which has a nearly identical representation in python) would export a list of nodes and the bits (both quantum and classical) that they operated on. This information is then used to create the SabreDAG used in the rust code. However, in this process an edge case was missed with classical condtions. If a condition was specified solely as a property of the operation and not in cargs list the SabreDAG would not know about the classical bit dependency between any subsequent operations involving that classical bit. This would cause incorrect output because the ndoes would not get processed as they were in the circuit. This commit fixes this issue by explicitly checking if there is a condition on the operation and there are no cargs and if so adding the carg bits to the SabreDAG directly. This fixes the incorrect representation in SabreDAG.
Details and comments
Fixes #8675