From 920fa57ce9d577a8c6b5fc7e691699f3bcc85427 Mon Sep 17 00:00:00 2001 From: Matthew Treinish Date: Thu, 23 Nov 2023 00:18:33 -0500 Subject: [PATCH] Avoid routing 2q barriers in SabreSwap (#11295) * Avoid routing 2q barriers in SabreSwap This commit fixes an oversight in the sabre swap pass where a 2 qubit barrier would have been treated like a 2 qubit gate and swaps could potentially be inserted if the sabre algorithm thought it didn't have connectivity for the qargs to the barrier. However as barrier is just a compiler directive it is valid on all qubit pairs so this swap insertion was unnecessary, it was still valid but just not an optimal output. This commit fixes it by adding context to the sabre dag around whether a given node is a directive (and valid on all qubits) or not. * Update rust tests too * Skip directives in extended set generation too Co-authored-by: Kevin Hartman * Add release note * Update test layout in test_layout_many_search_trials This commit updates a layout in the: `TestSabreLayout.test_layout_many_search_trials` test case which was recently changed in #10323. The change in test ouput caused by #10323 is what triggered the investigation into this bugfix and now that barriers are being treated correctly by sabre the layout doesn't change in the test case anymore and this is reverting the test assertion to use the original layout before #10323 was merged. --------- Co-authored-by: Kevin Hartman --- crates/accelerate/src/sabre_swap/mod.rs | 73 ++++++++++--------- crates/accelerate/src/sabre_swap/sabre_dag.rs | 22 +++++- .../transpiler/passes/routing/sabre_swap.py | 1 + ...bre-no-route-barrier-bc82fecb65d3ab9a.yaml | 9 +++ test/python/transpiler/test_sabre_layout.py | 2 +- test/python/transpiler/test_sabre_swap.py | 23 ++++++ 6 files changed, 91 insertions(+), 39 deletions(-) create mode 100644 releasenotes/notes/sabre-no-route-barrier-bc82fecb65d3ab9a.yaml diff --git a/crates/accelerate/src/sabre_swap/mod.rs b/crates/accelerate/src/sabre_swap/mod.rs index d4345a02e2c1..ef5ee9e35a0a 100644 --- a/crates/accelerate/src/sabre_swap/mod.rs +++ b/crates/accelerate/src/sabre_swap/mod.rs @@ -178,7 +178,9 @@ fn populate_extended_set( *decremented.entry(successor_index).or_insert(0) += 1; required_predecessors[successor_index] -= 1; if required_predecessors[successor_index] == 0 { - if !dag.node_blocks.contains_key(&successor_index) { + if !dag.dag[successor_node].directive + && !dag.node_blocks.contains_key(&successor_index) + { if let [a, b] = dag.dag[successor_node].qubits[..] { extended_set.push([a.to_phys(layout), b.to_phys(layout)]); } @@ -582,41 +584,44 @@ fn route_reachable_nodes( let node_id = to_visit[i]; let node = &dag.dag[node_id]; i += 1; - - match dag.node_blocks.get(&node.py_node_id) { - Some(blocks) => { - // Control flow op. Route all blocks for current layout. - let mut block_results: Vec = Vec::with_capacity(blocks.len()); - for inner_dag in blocks { - let (inner_dag_routed, inner_final_layout) = route_block_dag(inner_dag, layout); - // For now, we always append a swap circuit that gets the inner block - // back to the parent's layout. - let swap_epilogue = - gen_swap_epilogue(coupling, inner_final_layout, layout, seed); - let block_result = BlockResult { - result: inner_dag_routed, - swap_epilogue, - }; - block_results.push(block_result); + // If the node is a directive that means it can be placed anywhere + if !node.directive { + match dag.node_blocks.get(&node.py_node_id) { + Some(blocks) => { + // Control flow op. Route all blocks for current layout. + let mut block_results: Vec = Vec::with_capacity(blocks.len()); + for inner_dag in blocks { + let (inner_dag_routed, inner_final_layout) = + route_block_dag(inner_dag, layout); + // For now, we always append a swap circuit that gets the inner block + // back to the parent's layout. + let swap_epilogue = + gen_swap_epilogue(coupling, inner_final_layout, layout, seed); + let block_result = BlockResult { + result: inner_dag_routed, + swap_epilogue, + }; + block_results.push(block_result); + } + node_block_results.insert_unique_unchecked(node.py_node_id, block_results); } - node_block_results.insert_unique_unchecked(node.py_node_id, block_results); + None => match node.qubits[..] { + // A gate op whose connectivity must match the device to be + // placed in the gate order. + [a, b] + if !coupling.contains_edge( + NodeIndex::new(a.to_phys(layout).index()), + NodeIndex::new(b.to_phys(layout).index()), + ) => + { + // 2Q op that cannot be placed. Add it to the front layer + // and move on. + front_layer.insert(node_id, [a.to_phys(layout), b.to_phys(layout)]); + continue; + } + _ => {} + }, } - None => match node.qubits[..] { - // A gate op whose connectivity must match the device to be - // placed in the gate order. - [a, b] - if !coupling.contains_edge( - NodeIndex::new(a.to_phys(layout).index()), - NodeIndex::new(b.to_phys(layout).index()), - ) => - { - // 2Q op that cannot be placed. Add it to the front layer - // and move on. - front_layer.insert(node_id, [a.to_phys(layout), b.to_phys(layout)]); - continue; - } - _ => {} - }, } gate_order.push(node.py_node_id); diff --git a/crates/accelerate/src/sabre_swap/sabre_dag.rs b/crates/accelerate/src/sabre_swap/sabre_dag.rs index 7178c8570aea..037470b9b79d 100644 --- a/crates/accelerate/src/sabre_swap/sabre_dag.rs +++ b/crates/accelerate/src/sabre_swap/sabre_dag.rs @@ -23,6 +23,7 @@ use crate::nlayout::VirtualQubit; pub struct DAGNode { pub py_node_id: usize, pub qubits: Vec, + pub directive: bool, } /// A DAG representation of the logical circuit to be routed. This represents the same dataflow @@ -41,7 +42,7 @@ pub struct SabreDAG { pub num_clbits: usize, pub dag: DiGraph, pub first_layer: Vec, - pub nodes: Vec<(usize, Vec, HashSet)>, + pub nodes: Vec<(usize, Vec, HashSet, bool)>, pub node_blocks: HashMap>, } @@ -52,7 +53,7 @@ impl SabreDAG { pub fn new( num_qubits: usize, num_clbits: usize, - nodes: Vec<(usize, Vec, HashSet)>, + nodes: Vec<(usize, Vec, HashSet, bool)>, node_blocks: HashMap>, ) -> PyResult { let mut qubit_pos: Vec> = vec![None; num_qubits]; @@ -65,6 +66,7 @@ impl SabreDAG { let gate_index = dag.add_node(DAGNode { py_node_id: node.0, qubits: qargs.clone(), + directive: node.3, }); let mut is_front = true; for x in qargs { @@ -118,12 +120,24 @@ mod test { #[test] fn no_panic_on_bad_qubits() { let bad_qubits = vec![VirtualQubit::new(0), VirtualQubit::new(2)]; - assert!(SabreDAG::new(2, 0, vec![(0, bad_qubits, HashSet::new())], HashMap::new()).is_err()) + assert!(SabreDAG::new( + 2, + 0, + vec![(0, bad_qubits, HashSet::new(), false)], + HashMap::new() + ) + .is_err()) } #[test] fn no_panic_on_bad_clbits() { let good_qubits = vec![VirtualQubit::new(0), VirtualQubit::new(1)]; - assert!(SabreDAG::new(2, 1, vec![(0, good_qubits, [0, 1].into())], HashMap::new()).is_err()) + assert!(SabreDAG::new( + 2, + 1, + vec![(0, good_qubits, [0, 1].into(), false)], + HashMap::new() + ) + .is_err()) } } diff --git a/qiskit/transpiler/passes/routing/sabre_swap.py b/qiskit/transpiler/passes/routing/sabre_swap.py index 3f83a02237cd..250b310d3a80 100644 --- a/qiskit/transpiler/passes/routing/sabre_swap.py +++ b/qiskit/transpiler/passes/routing/sabre_swap.py @@ -309,6 +309,7 @@ def process_dag(block_dag, wire_map): node._node_id, [wire_map[x] for x in node.qargs], cargs, + getattr(node.op, "_directive", False), ) ) return SabreDAG(num_physical_qubits, block_dag.num_clbits(), dag_list, node_blocks) diff --git a/releasenotes/notes/sabre-no-route-barrier-bc82fecb65d3ab9a.yaml b/releasenotes/notes/sabre-no-route-barrier-bc82fecb65d3ab9a.yaml new file mode 100644 index 000000000000..fe77e9c51ef0 --- /dev/null +++ b/releasenotes/notes/sabre-no-route-barrier-bc82fecb65d3ab9a.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + Fixed an issue in the :class:`.SabreSwap` and :class:`.SabreLayout` + transpiler passes where it would incorrectly treat 2 qubit + :class:`.Barrier` instructions as an instruction that needs to be + routed according the transpiler :class:`.Target`. When this occured the + output was still correct but would potentially include unecessary + :class:`.SwapGate` instructions. diff --git a/test/python/transpiler/test_sabre_layout.py b/test/python/transpiler/test_sabre_layout.py index 8cd38222691c..abc48f63ae68 100644 --- a/test/python/transpiler/test_sabre_layout.py +++ b/test/python/transpiler/test_sabre_layout.py @@ -254,7 +254,7 @@ def test_layout_many_search_trials(self): self.assertIsInstance(res, QuantumCircuit) layout = res._layout.initial_layout self.assertEqual( - [layout[q] for q in qc.qubits], [10, 16, 8, 4, 11, 20, 15, 19, 18, 7, 12, 1, 2, 0] + [layout[q] for q in qc.qubits], [22, 7, 2, 12, 1, 5, 14, 4, 11, 0, 16, 15, 3, 10] ) diff --git a/test/python/transpiler/test_sabre_swap.py b/test/python/transpiler/test_sabre_swap.py index aaa84257d490..100e7882c782 100644 --- a/test/python/transpiler/test_sabre_swap.py +++ b/test/python/transpiler/test_sabre_swap.py @@ -129,6 +129,29 @@ def test_trivial_case(self): self.assertEqual(new_qc, qc) + def test_2q_barriers_not_routed(self): + """Test that a 2q barrier is not routed.""" + coupling = CouplingMap.from_line(5) + + qr = QuantumRegister(5, "q") + qc = QuantumCircuit(qr) + qc.barrier(0, 1) + qc.barrier(0, 2) + qc.barrier(0, 3) + qc.barrier(2, 3) + qc.h(0) + qc.barrier(1, 2) + qc.barrier(1, 0) + qc.barrier(1, 3) + qc.barrier(1, 4) + qc.barrier(4, 3) + qc.barrier(0, 4) + + passmanager = PassManager(SabreSwap(coupling, "basic")) + new_qc = passmanager.run(qc) + + self.assertEqual(new_qc, qc) + def test_trivial_with_target(self): """Test that an already mapped circuit is unchanged with target.""" coupling = CouplingMap.from_ring(5)