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.
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
Add support for disjoint coupling maps to SabreLayout #9802
Add support for disjoint coupling maps to SabreLayout #9802
Changes from all commits
f0fb38e
b5f0017
4611e08
2801978
e637d19
35fdc84
3092e68
1ddcb03
19d9570
b3443d4
0fb0d4f
1575b8a
3b078b8
0019460
01690e5
7085288
fe54898
1fc880e
a093283
117223c
08afa5d
5df53a7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 am potentially worried about this potentially introducing a cycle when combining a barrier across qubits. I think it's possible this could cause that which is why I left the cycle check enabled for this. But we should try to confirm this before merging.
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.
A potential future optimization that I'm thinking of doing after this and #9840 merge is to skip the intermediate dag creation here, and go from connected qubits straight to a list of SabreDAGs. This should reduce the number of dag iterations we do and speed things up. But for the first iteration of this I'd like to keep everything unified on using full dags.
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.
Talking with @kevinhartman offline about this and writing extra test cases to confirm there is a potential bug with classical bits here that will result in a reordering of the data dependency between components. Basically the component which is first in
layout_components
will end up being first on the classical bits regardless of what the actual data dependency is (the order here is from largest component to smallest).I think the best fix for this is to just skip routing if
layout_components > 1
and just return the layout as metadata. The problem with trying to do routing across components like this is getting the topological order exactly right between components manually is very tricky, especially after the routing pass has done 3 different topological iterations over the subcircuits. It's just easier to run routing again even at a small performance penalty.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 fixed this in: 1575b8a so we don't run routing as part of
SabreLayout
if there is a potential data dependency between components for classical bits.