Fix handling of conditions in SabreDAG creation (#8727)

* Fix handling of conditions in SabreDAG creation

In #8388 we moved all of the SabreSwap pass's processing into rust. To
facilitate that we had to create a rust DAG data structure to represent
the data flow graph solely in Rust to enable analyzing the DAG structure
in rust code without having to callback to python. To do this the
DAGCircuit (which has a nearly identical representation in python) would
export a list of nodes and the bits (both quantum and classical) that
they operated on. This information is then used to create the SabreDAG
used in the rust code. However, in this process an edge case was missed
with classical condtions. If a condition was specified solely as a
property of the operation and not in cargs list the SabreDAG would not
know about the classical bit dependency between any subsequent
operations involving that classical bit. This would cause incorrect
output because the ndoes would not get processed as they were in the
circuit. This commit fixes this issue by explicitly checking if there is
a condition on the operation and there are no cargs and if so adding the
carg bits to the SabreDAG directly. This fixes the incorrect
representation in SabreDAG.

Fixes #8675

* Fix handling of instructions with condition and cargs

This commit fixes the logic for handling instructions that have both
cargs and conditions set. The previous commit fixed the behavior only if
cargs was mutually exclusive with having classical arguments. However,
the same bug this PR is fixing would persist for instructions with clbit
arguments and a condition. This fixes the behavior to correctly
represent the data dependency in SabreDAG for these cases.

* Improve efficiency of condition handling

This commit improves the efficiency of the condition handling on
SabreDAG creation that was added in the previous commit. Previously, it
was potentially expensive to loop over the list of cargs and condition
bits as for instructions with a large number of both the repeated
duplicate searches would get quite expensive. This commit fixes that by
converting the cargs data structure to be a set to prevent adding
duplicates. Since the order isn't significant for the cargs in sabre as
it's only used to represent data dependency between instructions we can
convert it to internally use a set in python and a HashSet in rust. This
also removes storing of cargs for each node in the SabreDAG as this was
never used and just wasted memory by storing the list and never using
it.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

qiskit/transpiler/passes/routing/sabre_swap.py

@@ -210,11 +210,16 @@ def run(self, dag):
 
         dag_list = []
         for node in dag.topological_op_nodes():
+            cargs = {self._clbit_indices[x] for x in node.cargs}
+            if node.op.condition is not None:
+                for clbit in dag._bits_in_condition(node.op.condition):
+                    cargs.add(self._clbit_indices[clbit])
+
             dag_list.append(
                 (
                     node._node_id,
                     [self._qubit_indices[x] for x in node.qargs],
-                    [self._clbit_indices[x] for x in node.cargs],
+                    cargs,
                 )
             )
         front_layer = np.asarray([x._node_id for x in dag.front_layer()], dtype=np.uintp)

src/sabre_swap/sabre_dag.rs

@@ -10,7 +10,7 @@
 // copyright notice, and modified files need to carry a notice indicating
 // that they have been altered from the originals.
 
-use hashbrown::HashMap;
+use hashbrown::{HashMap, HashSet};
 use numpy::PyReadonlyArray1;
 use pyo3::prelude::*;
 use retworkx_core::petgraph::prelude::*;
@@ -23,7 +23,7 @@ use retworkx_core::petgraph::prelude::*;
 #[pyo3(text_signature = "(num_qubits, num_clbits, nodes, front_layer, /)")]
 #[derive(Clone, Debug)]
 pub struct SabreDAG {
-    pub dag: DiGraph<(usize, Vec<usize>, Vec<usize>), ()>,
+    pub dag: DiGraph<(usize, Vec<usize>), ()>,
     pub first_layer: Vec<NodeIndex>,
 }
 
@@ -33,18 +33,18 @@ impl SabreDAG {
     pub fn new(
         num_qubits: usize,
         num_clbits: usize,
-        nodes: Vec<(usize, Vec<usize>, Vec<usize>)>,
+        nodes: Vec<(usize, Vec<usize>, HashSet<usize>)>,
         front_layer: PyReadonlyArray1<usize>,
     ) -> PyResult<Self> {
         let mut qubit_pos: Vec<usize> = vec![usize::MAX; num_qubits];
         let mut clbit_pos: Vec<usize> = vec![usize::MAX; num_clbits];
         let mut reverse_index_map: HashMap<usize, NodeIndex> = HashMap::with_capacity(nodes.len());
-        let mut dag: DiGraph<(usize, Vec<usize>, Vec<usize>), ()> =
+        let mut dag: DiGraph<(usize, Vec<usize>), ()> =
             Graph::with_capacity(nodes.len(), 2 * nodes.len());
         for node in &nodes {
             let qargs = &node.1;
             let cargs = &node.2;
-            let gate_index = dag.add_node(node.clone());
+            let gate_index = dag.add_node((node.0, qargs.clone()));
             reverse_index_map.insert(node.0, gate_index);
             for x in qargs {
                 if qubit_pos[*x] != usize::MAX {

test/python/transpiler/test_sabre_swap.py

@@ -262,6 +262,51 @@ def test_classical_condition(self):
             actual = PassManager([TrivialLayout(cm), SabreSwap(cm)]).run(qc)
             self.assertEqual(expected, actual)
 
+    def test_classical_condition_cargs(self):
+        """Test that classical conditions are preserved even if missing from cargs DAGNode field.
+
+        Created from reproduction in https://github.com/Qiskit/qiskit-terra/issues/8675
+        """
+        with self.subTest("missing measurement"):
+            qc = QuantumCircuit(3, 1)
+            qc.cx(0, 2).c_if(0, 0)
+            qc.measure(1, 0)
+            qc.h(2).c_if(0, 0)
+            expected = QuantumCircuit(3, 1)
+            expected.swap(1, 2)
+            expected.cx(0, 1).c_if(0, 0)
+            expected.measure(2, 0)
+            expected.h(1).c_if(0, 0)
+            result = SabreSwap(CouplingMap.from_line(3), seed=12345)(qc)
+            self.assertEqual(result, expected)
+        with self.subTest("reordered measurement"):
+            qc = QuantumCircuit(3, 1)
+            qc.cx(0, 1).c_if(0, 0)
+            qc.measure(1, 0)
+            qc.h(0).c_if(0, 0)
+            expected = QuantumCircuit(3, 1)
+            expected.cx(0, 1).c_if(0, 0)
+            expected.measure(1, 0)
+            expected.h(0).c_if(0, 0)
+            result = SabreSwap(CouplingMap.from_line(3), seed=12345)(qc)
+            self.assertEqual(result, expected)
+
+    def test_conditional_measurement(self):
+        """Test that instructions with cargs and conditions are handled correctly."""
+        qc = QuantumCircuit(3, 2)
+        qc.cx(0, 2).c_if(0, 0)
+        qc.measure(2, 0).c_if(1, 0)
+        qc.h(2).c_if(0, 0)
+        qc.measure(1, 1)
+        expected = QuantumCircuit(3, 2)
+        expected.swap(1, 2)
+        expected.cx(0, 1).c_if(0, 0)
+        expected.measure(1, 0).c_if(1, 0)
+        expected.h(1).c_if(0, 0)
+        expected.measure(2, 1)
+        result = SabreSwap(CouplingMap.from_line(3), seed=12345)(qc)
+        self.assertEqual(result, expected)
+
 
 if __name__ == "__main__":
     unittest.main()