Skip to content
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

Fix an edge case in Sabre's release valve #13114

Merged
merged 10 commits into from
Sep 11, 2024
5 changes: 4 additions & 1 deletion crates/accelerate/src/sabre/layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ impl FrontLayer {
pub fn remove(&mut self, index: &NodeIndex) {
// The actual order in the indexmap doesn't matter as long as it's reproducible.
// Swap-remove is more efficient than a full shift-remove.
let [a, b] = self.nodes.swap_remove(index).unwrap();
let [a, b] = self
.nodes
.swap_remove(index)
.expect("Tried removing index that does not exist.");
self.qubits[a.index()] = None;
self.qubits[b.index()] = None;
}
Expand Down
37 changes: 33 additions & 4 deletions crates/accelerate/src/sabre/route.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

use std::cmp::Ordering;

use hashbrown::HashSet;
use pyo3::prelude::*;
use pyo3::Python;

Expand All @@ -27,6 +28,7 @@ use rustworkx_core::petgraph::prelude::*;
use rustworkx_core::petgraph::visit::EdgeRef;
use rustworkx_core::shortest_path::dijkstra;
use rustworkx_core::token_swapper::token_swapper;
use smallvec::{smallvec, SmallVec};

use crate::getenv_use_multiple_threads;
use crate::nlayout::{NLayout, PhysicalQubit};
Expand Down Expand Up @@ -286,7 +288,7 @@ impl<'a, 'b> RoutingState<'a, 'b> {
fn force_enable_closest_node(
&mut self,
current_swaps: &mut Vec<[PhysicalQubit; 2]>,
) -> NodeIndex {
) -> SmallVec<[NodeIndex; 2]> {
let (&closest_node, &qubits) = {
let dist = &self.target.distance;
self.front_layer
Expand Down Expand Up @@ -328,7 +330,32 @@ impl<'a, 'b> RoutingState<'a, 'b> {
current_swaps.push([shortest_path[end], shortest_path[end - 1]]);
}
current_swaps.iter().for_each(|&swap| self.apply_swap(swap));
closest_node

// If we apply a single swap it could be that we route 2 nodes; that is a setup like
// A - B - A - B
// and we swap the middle two qubits. This cannot happen if we apply more than 2 swaps.
Cryoris marked this conversation as resolved.
Show resolved Hide resolved
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]
.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);
Comment on lines +345 to +348
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this just be a single filter map. Something like:

Suggested change
.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);
.filter_map(|&swap_qubit| {
self.front_layer.qubits()[swap_qubit.index()]).map(|(node_index, other_qubit)| {
// remove the closest_node, which we know we already routed
if node_index != closest_node {
Some(other_qubit)
} else {
None
}
})
});

Or even:

Suggested change
.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);
.filter_map(|&swap_qubit| self.front_layer.qubits()[swap_qubit.index()])
// remove the closest_node, which we know we already routed
.filter_map(|(node_index, other_qubit)| if *node_index != closest_node {
Some(other_qubit)
} else {
None
})

Not that this makes a big difference. I'm surprised clippy isn't complaining about the filter followed by a map though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you're really going hard on fancy methods, you can even do (*node_index != closest_node).then_some(other_qubit) haha.


// 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]
}
}

/// Return the swap of two virtual qubits that produces the best score of all possible swaps.
Expand Down Expand Up @@ -573,14 +600,16 @@ pub fn swap_map_trial(
}
if routable_nodes.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't comment above this line, but is there any reason we shouldn't use a SmallVec for routable_nodes itself?

And, should we increase the capacity of the pre-allocation to 3 now on line 573?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't necessarily, because the release valve should only be taken rarely, and we don't want to overallocate the happy path, and we should still only be routing two nodes in the new case.

// If we exceeded the max number of heuristic-chosen swaps without making progress,
// unwind to the last progress point and greedily swap to bring a ndoe together.
// unwind to the last progress point and greedily swap to bring a node together.
// Efficiency doesn't matter much; this path never gets taken unless we're unlucky.
current_swaps
.drain(..)
.rev()
.for_each(|swap| state.apply_swap(swap));
let force_routed = state.force_enable_closest_node(&mut current_swaps);
routable_nodes.push(force_routed);
force_routed
.iter()
.for_each(|routable_node| routable_nodes.push(*routable_node));
Cryoris marked this conversation as resolved.
Show resolved Hide resolved
}
state.update_route(&routable_nodes, current_swaps);
if state.heuristic.decay.is_some() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
fixes:
- |
Fixed an edge case in :class:`.SabreLayout`, where in rare cases on large
devices and challenging circuits, the routing would fail. This was due to the
release valve making more than one two-qubit gate routable, where only one was expected.
Fixed `#13081 <https://github.com/Qiskit/qiskit/issues/13081>`__.
25 changes: 22 additions & 3 deletions test/python/transpiler/test_sabre_layout.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,16 @@

from qiskit import QuantumRegister, QuantumCircuit
from qiskit.circuit.classical import expr, types
from qiskit.circuit.library import EfficientSU2
from qiskit.circuit.library import EfficientSU2, QuantumVolume
from qiskit.transpiler import CouplingMap, AnalysisPass, PassManager
from qiskit.transpiler.passes import SabreLayout, DenseLayout, StochasticSwap
from qiskit.transpiler.passes import SabreLayout, DenseLayout, StochasticSwap, Unroll3qOrMore
from qiskit.transpiler.exceptions import TranspilerError
from qiskit.converters import circuit_to_dag
from qiskit.compiler.transpiler import transpile
from qiskit.providers.fake_provider import GenericBackendV2
from qiskit.transpiler.passes.layout.sabre_pre_layout import SabrePreLayout
from qiskit.transpiler.preset_passmanagers import generate_preset_pass_manager
from test import QiskitTestCase # pylint: disable=wrong-import-order
from test import QiskitTestCase, slow_test # pylint: disable=wrong-import-order

from ..legacy_cmaps import ALMADEN_CMAP, MUMBAI_CMAP

Expand Down Expand Up @@ -315,6 +315,25 @@ def test_support_var_with_explicit_routing_pass(self):
layout = pass_.property_set["layout"]
self.assertEqual([layout[q] for q in qc.qubits], [2, 3, 4, 1, 5])

@slow_test
def test_release_valve_routes_multiple(self):
"""Test Sabre works if the release valve routes more than 1 operation.

Regression test of #13081.
"""
qv = QuantumVolume(500, seed=42)
qv.measure_all()
qc = Unroll3qOrMore()(qv)

cmap = CouplingMap.from_heavy_hex(21)
pm = PassManager(
[
SabreLayout(cmap, swap_trials=20, layout_trials=20, max_iterations=4, seed=100),
]
)
_ = pm.run(qc)
self.assertIsNotNone(pm.property_set.get("layout"))


class DensePartialSabreTrial(AnalysisPass):
"""Pass to run dense layout as a sabre trial."""
Expand Down
Loading