Collapse Sabre nodes with no routing implications#14607
Merged
mtreinish merged 2 commits intoQiskit:mainfrom Aug 13, 2025
Merged
Collapse Sabre nodes with no routing implications#14607mtreinish merged 2 commits intoQiskit:mainfrom
mtreinish merged 2 commits intoQiskit:mainfrom
Conversation
jakelishman
added
performance
Rust
Collaborator
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 16883428058Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Not all nodes in the `SabreDAG` actually imply additional routing constraints. For example, a 2q gate immediately following a prior 2q gate on the same qubits doesn't need to be routed; it's automatically valid as soon as its predecessor is. Similarly, 1q gates (without additional classical wires) never impact routing and can be folded into previous instructions. More formally, a `Synchronize` node whose incoming edges are all from the same predecessor (or are at the start of the circuit) does not actually synchronise anything, and can be folded into any preceding node. A `TwoQ` node whose incoming edges are all from the same predecessor can be folded into that predecessor if it is another `TwoQ` node (even if there were already intermediate `Synchronize` nodes folded into that `TwoQ` node). `ControlFlow` nodes always have to be considered by the router, because they need to be recursed into. Collapsing these nodes as part of the `SabreDAG` construction has a few benefits: - In layout, when we keep multiple versions of a `SabreDAG` around, we have to store less memory, especially because we can entirely throw away all tracking of the collapsed DAG nodes, since we never need to rebuild afterwards. - In routing, when we look ahead to populate the extended set, we no longer need to have the separate `visit_now` queue because all the nodes (and more!) that we would "look through" to make a DFS over the 2q gates are now automatically folded into the preceding 2q gate already. The behaviour is not entirely identical (the handling of wide `Synchronize` items can be slightly different), but the spirit is still there, and the handling is much easier. - When populating the extended set, runs of 2q gates can no longer be multiply counted. Previously, a run _could_ saturate the extended set, biasing the algorithm towards that particular pair, even though the routing constraint was no stronger than any other single gate.
jakelishman
force-pushed
the
sabre/compress
branch
from
71ecbb4 to
a4d18f1
Compare
Member
Author
|
Now rebased over #14317. |
mtreinish
previously approved these changes
Aug 6, 2025
Member
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
This LGTM, it's fairly straightforward and is a nice optimization that I think is really neat. I had one nit inline. Feel free to enqueue if you don't want to change it otherwise I'll circle back with a re-approval if you do change it.
| InteractionKind::Synchronize => { | ||
| initial.push(dag_node); | ||
| } | ||
| kind => { |
Member
There was a problem hiding this comment.
There a so many weird match syntax things I don't know about. I really didn't know this worked until just now.
Member
Author
There was a problem hiding this comment.
I think this is just the root case of pattern-match syntax you're already familiar with? You use
match <expr> {
InteractionKind::ControlFlow(num_blocks) => (),
}happily, where num_blocks assigns a name to a partial destructuring. The highlighted line here is the same, there's just no destructuring happening; it's matching the root pattern. It's the same thing as a _ => () line, but I gave the pattern a name rather than throwing it away.
raynelfss
added
the
Changelog: None
mtreinish
approved these changes
Aug 13, 2025
littlebullGit
pushed a commit
to littlebullGit/qiskit
that referenced
this pull request
Sep 5, 2025
* Collapse Sabre nodes with no routing implications Not all nodes in the `SabreDAG` actually imply additional routing constraints. For example, a 2q gate immediately following a prior 2q gate on the same qubits doesn't need to be routed; it's automatically valid as soon as its predecessor is. Similarly, 1q gates (without additional classical wires) never impact routing and can be folded into previous instructions. More formally, a `Synchronize` node whose incoming edges are all from the same predecessor (or are at the start of the circuit) does not actually synchronise anything, and can be folded into any preceding node. A `TwoQ` node whose incoming edges are all from the same predecessor can be folded into that predecessor if it is another `TwoQ` node (even if there were already intermediate `Synchronize` nodes folded into that `TwoQ` node). `ControlFlow` nodes always have to be considered by the router, because they need to be recursed into. Collapsing these nodes as part of the `SabreDAG` construction has a few benefits: - In layout, when we keep multiple versions of a `SabreDAG` around, we have to store less memory, especially because we can entirely throw away all tracking of the collapsed DAG nodes, since we never need to rebuild afterwards. - In routing, when we look ahead to populate the extended set, we no longer need to have the separate `visit_now` queue because all the nodes (and more!) that we would "look through" to make a DFS over the 2q gates are now automatically folded into the preceding 2q gate already. The behaviour is not entirely identical (the handling of wide `Synchronize` items can be slightly different), but the spirit is still there, and the handling is much easier. - When populating the extended set, runs of 2q gates can no longer be multiply counted. Previously, a run _could_ saturate the extended set, biasing the algorithm towards that particular pair, even though the routing constraint was no stronger than any other single gate. * Make conditional binding clearer
aaryav-3
pushed a commit
to aaryav-3/qiskit
that referenced
this pull request
Oct 21, 2025
* Collapse Sabre nodes with no routing implications Not all nodes in the `SabreDAG` actually imply additional routing constraints. For example, a 2q gate immediately following a prior 2q gate on the same qubits doesn't need to be routed; it's automatically valid as soon as its predecessor is. Similarly, 1q gates (without additional classical wires) never impact routing and can be folded into previous instructions. More formally, a `Synchronize` node whose incoming edges are all from the same predecessor (or are at the start of the circuit) does not actually synchronise anything, and can be folded into any preceding node. A `TwoQ` node whose incoming edges are all from the same predecessor can be folded into that predecessor if it is another `TwoQ` node (even if there were already intermediate `Synchronize` nodes folded into that `TwoQ` node). `ControlFlow` nodes always have to be considered by the router, because they need to be recursed into. Collapsing these nodes as part of the `SabreDAG` construction has a few benefits: - In layout, when we keep multiple versions of a `SabreDAG` around, we have to store less memory, especially because we can entirely throw away all tracking of the collapsed DAG nodes, since we never need to rebuild afterwards. - In routing, when we look ahead to populate the extended set, we no longer need to have the separate `visit_now` queue because all the nodes (and more!) that we would "look through" to make a DFS over the 2q gates are now automatically folded into the preceding 2q gate already. The behaviour is not entirely identical (the handling of wide `Synchronize` items can be slightly different), but the spirit is still there, and the handling is much easier. - When populating the extended set, runs of 2q gates can no longer be multiply counted. Previously, a run _could_ saturate the extended set, biasing the algorithm towards that particular pair, even though the routing constraint was no stronger than any other single gate. * Make conditional binding clearer
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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
Not all nodes in the
SabreDAGactually imply additional routing constraints. For example, a 2q gate immediately following a prior 2q gate on the same qubits doesn't need to be routed; it's automatically valid as soon as its predecessor is. Similarly, 1q gates (without additional classical wires) never impact routing and can be folded into previous instructions.More formally, a
Synchronizenode whose incoming edges are all from the same predecessor (or are at the start of the circuit) does not actually synchronise anything, and can be folded into any preceding node. ATwoQnode whose incoming edges are all from the same predecessor can be folded into that predecessor if it is anotherTwoQnode (even if there were already intermediateSynchronizenodes folded into thatTwoQnode).ControlFlownodes always have to be considered by the router, because they need to be recursed into.Collapsing these nodes as part of the
SabreDAGconstruction has a few benefits:In layout, when we keep multiple versions of a
SabreDAGaround, we have to store less memory, especially because we can entirely throw away all tracking of the collapsed DAG nodes, since we never need to rebuild afterwards.In routing, when we look ahead to populate the extended set, we no longer need to have the separate
visit_nowqueue because all the nodes (and more!) that we would "look through" to make a DFS over the 2q gates are now automatically folded into the preceding 2q gate already. The behaviour is not entirely identical (the handling of wideSynchronizeitems can be slightly different), but the spirit is still there, and the handling is much easier.When populating the extended set, runs of 2q gates can no longer be multiply counted. Previously, a run could saturate the extended set, biasing the algorithm towards that particular pair, even though the routing constraint was no stronger than any other single gate.
Details and comments
Based on #14317.