From a01e36c4e1f7ddc04749c018f512670b68970fff Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Mon, 9 Sep 2024 15:35:56 +0200 Subject: [PATCH 1/7] Fix edge case in Sabre release valve If a Sabre trial does not find a set of Swaps to route nodes, the "release valve" adds Swaps to route the two-qubit gate in between the the closest two qubits. In rare cases, this leads to _more_ than one gate being routable, which was not handled correctly previously. Co-authored-by: Jake Lishman --- crates/accelerate/src/sabre/layer.rs | 5 +++- crates/accelerate/src/sabre/route.rs | 37 +++++++++++++++++++++++++--- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/crates/accelerate/src/sabre/layer.rs b/crates/accelerate/src/sabre/layer.rs index 899321a96681..dec8253ad282 100644 --- a/crates/accelerate/src/sabre/layer.rs +++ b/crates/accelerate/src/sabre/layer.rs @@ -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; } diff --git a/crates/accelerate/src/sabre/route.rs b/crates/accelerate/src/sabre/route.rs index bef6d501b4aa..2d586d1a7813 100644 --- a/crates/accelerate/src/sabre/route.rs +++ b/crates/accelerate/src/sabre/route.rs @@ -12,6 +12,7 @@ use std::cmp::Ordering; +use hashbrown::HashSet; use pyo3::prelude::*; use pyo3::Python; @@ -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}; @@ -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 @@ -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. + 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); + + // 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. @@ -573,14 +600,16 @@ pub fn swap_map_trial( } if routable_nodes.is_empty() { // 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)); } state.update_route(&routable_nodes, current_swaps); if state.heuristic.decay.is_some() { From 9925cfc6b285ed149cb906ca9b3bdb16a5de65db Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Mon, 9 Sep 2024 16:32:45 +0200 Subject: [PATCH 2/7] add reno and test --- ...x-sabre-releasevalve-7f9af9bfc0482e04.yaml | 7 ++++++ test/python/transpiler/test_sabre_layout.py | 25 ++++++++++++++++--- 2 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/fix-sabre-releasevalve-7f9af9bfc0482e04.yaml diff --git a/releasenotes/notes/fix-sabre-releasevalve-7f9af9bfc0482e04.yaml b/releasenotes/notes/fix-sabre-releasevalve-7f9af9bfc0482e04.yaml new file mode 100644 index 000000000000..17432259be29 --- /dev/null +++ b/releasenotes/notes/fix-sabre-releasevalve-7f9af9bfc0482e04.yaml @@ -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 `__. \ No newline at end of file diff --git a/test/python/transpiler/test_sabre_layout.py b/test/python/transpiler/test_sabre_layout.py index a5ebef43540f..7565ee17655f 100644 --- a/test/python/transpiler/test_sabre_layout.py +++ b/test/python/transpiler/test_sabre_layout.py @@ -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 @@ -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.""" From 2d999b80226e8f11be4aed645634cd0312891d2c Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Mon, 9 Sep 2024 21:32:31 +0200 Subject: [PATCH 3/7] Use sensible syntax Co-authored-by: Kevin Hartman --- crates/accelerate/src/sabre/route.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/accelerate/src/sabre/route.rs b/crates/accelerate/src/sabre/route.rs index 2d586d1a7813..09105cb6ccae 100644 --- a/crates/accelerate/src/sabre/route.rs +++ b/crates/accelerate/src/sabre/route.rs @@ -607,9 +607,7 @@ pub fn swap_map_trial( .rev() .for_each(|swap| state.apply_swap(swap)); let force_routed = state.force_enable_closest_node(&mut current_swaps); - force_routed - .iter() - .for_each(|routable_node| routable_nodes.push(*routable_node)); + routable_nodes.extend(force_routed); } state.update_route(&routable_nodes, current_swaps); if state.heuristic.decay.is_some() { From e7a5eb23f5542e99dc1852c3f85c3c0f8d81e515 Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Mon, 9 Sep 2024 21:33:00 +0200 Subject: [PATCH 4/7] Use sensible grammar Co-authored-by: Kevin Hartman --- crates/accelerate/src/sabre/route.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/accelerate/src/sabre/route.rs b/crates/accelerate/src/sabre/route.rs index 09105cb6ccae..fcd35a969e28 100644 --- a/crates/accelerate/src/sabre/route.rs +++ b/crates/accelerate/src/sabre/route.rs @@ -333,7 +333,7 @@ impl<'a, 'b> RoutingState<'a, 'b> { // 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. + // and we swap the middle two qubits. This cannot happen if we apply 2 or more swaps. if current_swaps.len() > 1 { smallvec![closest_node] } else { From c41bedda9f84e2714471cca99c3886cd760bba9c Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Mon, 9 Sep 2024 21:51:59 +0200 Subject: [PATCH 5/7] clippy --- crates/accelerate/src/sabre/route.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/accelerate/src/sabre/route.rs b/crates/accelerate/src/sabre/route.rs index 2d586d1a7813..9b3c6d0a4c0b 100644 --- a/crates/accelerate/src/sabre/route.rs +++ b/crates/accelerate/src/sabre/route.rs @@ -12,7 +12,6 @@ use std::cmp::Ordering; -use hashbrown::HashSet; use pyo3::prelude::*; use pyo3::Python; From 01bcdc7ca225220e7f5a319e2ef52f626251610a Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Tue, 10 Sep 2024 13:18:07 +0200 Subject: [PATCH 6/7] Check if the new test breaks CI --- test/python/transpiler/test_sabre_layout.py | 36 ++++++++++----------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/test/python/transpiler/test_sabre_layout.py b/test/python/transpiler/test_sabre_layout.py index 7565ee17655f..f40b6d8f1c8b 100644 --- a/test/python/transpiler/test_sabre_layout.py +++ b/test/python/transpiler/test_sabre_layout.py @@ -315,24 +315,24 @@ 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")) + # @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): From a6b4e5bafed1b85e87357def3b79c30448c19b94 Mon Sep 17 00:00:00 2001 From: Julien Gacon Date: Tue, 10 Sep 2024 17:05:20 +0200 Subject: [PATCH 7/7] Revert "Check if the new test breaks CI" This reverts commit 01bcdc7ca225220e7f5a319e2ef52f626251610a. --- test/python/transpiler/test_sabre_layout.py | 36 ++++++++++----------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/test/python/transpiler/test_sabre_layout.py b/test/python/transpiler/test_sabre_layout.py index f40b6d8f1c8b..7565ee17655f 100644 --- a/test/python/transpiler/test_sabre_layout.py +++ b/test/python/transpiler/test_sabre_layout.py @@ -315,24 +315,24 @@ 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")) + @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):