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 <[email protected]>

* 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 <[email protected]>

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<F>(
         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<BlockResult> = 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<BlockResult> = 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);

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<VirtualQubit>,
+    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<DAGNode, ()>,
     pub first_layer: Vec<NodeIndex>,
-    pub nodes: Vec<(usize, Vec<VirtualQubit>, HashSet<usize>)>,
+    pub nodes: Vec<(usize, Vec<VirtualQubit>, HashSet<usize>, bool)>,
     pub node_blocks: HashMap<usize, Vec<SabreDAG>>,
 }
 
@@ -52,7 +53,7 @@ impl SabreDAG {
     pub fn new(
         num_qubits: usize,
         num_clbits: usize,
-        nodes: Vec<(usize, Vec<VirtualQubit>, HashSet<usize>)>,
+        nodes: Vec<(usize, Vec<VirtualQubit>, HashSet<usize>, bool)>,
         node_blocks: HashMap<usize, Vec<SabreDAG>>,
     ) -> PyResult<Self> {
         let mut qubit_pos: Vec<Option<NodeIndex>> = 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())
     }
 }

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)

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.

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]
         )
 
 

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)