Refactor Sabre state into separately mutable components#14909
Refactor Sabre state into separately mutable components#14909alexanderivrii merged 3 commits intoQiskit:mainfrom
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 16969141168Details
💛 - Coveralls |
jakelishman
force-pushed
the
sabre/layers/1
branch
from
a539fe6 to
b994982
Compare
jakelishman
force-pushed
the
sabre/layers/1
branch
from
b994982 to
5ede84f
Compare
This commit separates out the three logical components of the Sabre routing tracking into three separate structs, where each struct groups objects that have the same mutation tendency. The previous Sabre state stored its problem description, internal tracking, and output tracking altogether in the same flat structure. Those three components have different tendencies to mutate: the problem description never mutates, the internal tracking frequently does, and the output tracking only occasionally does and has a lifetime validity tied to that of the problem description. Putting them together made it impossible to call methods that mutated the state while passing an object that borrowed from the problem description, such as when recursing into control-flow operations, because the borrow checker could not validate it. This applied interface pressure to inline more into the same method, which made code-reuse of separate concerns harder.
jakelishman
force-pushed
the
sabre/layers/1
branch
from
5ede84f to
623c5dc
Compare
|
Uncoupled from its parents, so no longer on hold. |
alexanderivrii
left a comment
alexanderivrii
left a comment
There was a problem hiding this comment.
Overall, this split into components makes sense to me. Most of the changes are more or less mechanical and easy to follow (and I had a question about one particular code simplification).
| pub fn rebuild(&self) -> PyResult<DAGCircuit> { | ||
| let num_swaps = self.swap_count(); | ||
| let dag = self.dag.physical_empty_like_with_capacity( | ||
| let num_swaps = self.order.swap_count(); |
There was a problem hiding this comment.
self.swap_count() would also work, correct?
There was a problem hiding this comment.
Yeah. Some of the slightly weird diff here might be from rebases / reworking the chain a couple of times.
| if current_swaps.len() > 1 { | ||
| smallvec![closest_node] | ||
| } else { | ||
| // check if the closest node has neighbors that are now routable -- for that we get | ||
| // the other physical qubit that was swapped and check whether the node on it | ||
| // is now routable | ||
| let mut possible_other_qubit = current_swaps[0] | ||
| match current_swaps.as_slice() { | ||
| [swap] => swap | ||
| .iter() | ||
| // check if other nodes are in the front layer that are connected by this swap | ||
| .filter_map(|&swap_qubit| self.front_layer.qubits()[swap_qubit.index()]) | ||
| // remove the closest_node, which we know we already routed | ||
| .filter(|(node_index, _other_qubit)| *node_index != closest_node) | ||
| .map(|(_node_index, other_qubit)| other_qubit); | ||
|
|
||
| // if there is indeed another candidate, check if that gate is routable | ||
| if let Some(other_qubit) = possible_other_qubit.next() { | ||
| if let Some(also_routed) = self.routable_node_on_qubit(other_qubit) { | ||
| return smallvec![closest_node, also_routed]; | ||
| } | ||
| } | ||
| smallvec![closest_node] | ||
| .filter_map(|q| self.routable_node_on_qubit(problem, *q)) | ||
| .collect(), | ||
| _ => smallvec![closest_node], |
There was a problem hiding this comment.
I have not looked at this change closely yet, but it removes quite a number of lines. Can you comment as to what is going on?
There was a problem hiding this comment.
Normally in the release valve, we expect that there's only one newly routable gate. If there's more than one swap in the chain, it's only possible for there to be only one routable gate. In the special case of the release valve needing only a single swap (which needs a very particular set of heuristics and awkwardness in the layer structure), you can get something like A - B - A - B where the middle two qubits get swapped, and now two gates are routed.
The old code for this (see #13114) was kind of complex, as you see - its logic was something like:
- for each qubit in involved in the swap...
- find the gate (if any) that it is involved in
- ignore that gate if it's
closest_node - get the other qubit involved in that gate
- get the routable node on that qubit (if any, except we already know that there is one)
- emit both
closest_nodeand the other node.
Apparently I changed this as part of this PR (presumably I had to modify the self.front_layer and went overboard?), but the new logic is just:
- ignore
closest_node- we'll find it again - for each qubit in the swap...
- emit the routable gate on it, if any
which is obviously way simpler, and is still guaranteed to include closest_node and not duplicate it: the two qubits can't both be in closest_node or we wouldn't have entered the release valve; and we know closest_node is now routable because we took the Dijkstra path specifically to it.
There was a problem hiding this comment.
Thanks for the explanation -- the code simplification makes sense. My only mild concern is that this should ideologically belong to a separate PR.
There was a problem hiding this comment.
No worries, reverted in 607d159 and I can open a new PR once this merges.
alexanderivrii
left a comment
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks Jake. This looks good to me, modulo possibly splitting the release valve mechanism simplification into a separate PR. Or at least mentioning this simplification in the PR summary.
ShellyGarion
added
the
Changelog: None
8 participants
This commit separates out the three logical components of the Sabre routing tracking into three separate structs, where each struct groups objects that have the same mutation tendency.
The previous Sabre state stored its problem description, internal tracking, and output tracking altogether in the same flat structure. Those three components have different tendencies to mutate: the problem description never mutates, the internal tracking frequently does, and the output tracking only occasionally does and has a lifetime validity tied to that of the problem description.
Putting them together made it impossible to call methods that mutated the state while passing an object that borrowed from the problem description, such as when recursing into control-flow operations, because the borrow checker could not validate it. This applied interface pressure to inline more into the same method, which made code-reuse of separate concerns harder.