From 6db5059b79c52cac558d0434b1d4708ddeee24ba Mon Sep 17 00:00:00 2001 From: Jake Lishman Date: Tue, 19 Apr 2022 15:57:41 -0400 Subject: [PATCH] Make SabreSwap account for clbits in predecessors (#7952) * Make SabreSwap account for clbits in predecessors Previously, SabreSwap only checked that it was valid to add a gate back to the DAG by ensuring that all its qubit predecessors had already been added to the DAG. It did not check for clbit successors. This meant that measurements and (potentially, unverified) classically conditioned gates could be re-ordered out from the correct topological ordering. The most notable effect of this was that measurements writing to the same bit could be re-ordered, changing the output of the circuit. See gh-7950 for more detail. * Add reno * Fix lint * Trim fat in SabreSwap._successors Co-authored-by: Matthew Treinish * Fix _successors method Co-authored-by: Matthew Treinish (cherry picked from commit 38f3705d5f2789adc6fc3ff19bb7cb281f69cb85) --- .../transpiler/passes/routing/sabre_swap.py | 14 +++++---- ...fix-sabreswap-clbits-428eb5f3a46063da.yaml | 9 ++++++ test/python/transpiler/test_sabre_swap.py | 29 +++++++++++++++++++ 3 files changed, 46 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/fix-sabreswap-clbits-428eb5f3a46063da.yaml diff --git a/qiskit/transpiler/passes/routing/sabre_swap.py b/qiskit/transpiler/passes/routing/sabre_swap.py index 3690c763f734..5e44b29e5d13 100644 --- a/qiskit/transpiler/passes/routing/sabre_swap.py +++ b/qiskit/transpiler/passes/routing/sabre_swap.py @@ -18,7 +18,6 @@ import numpy as np from qiskit.circuit.library.standard_gates import SwapGate -from qiskit.circuit.quantumregister import Qubit from qiskit.transpiler.basepasses import TransformationPass from qiskit.transpiler.exceptions import TranspilerError from qiskit.transpiler.layout import Layout @@ -276,15 +275,18 @@ def _reset_qubits_decay(self): self.qubits_decay = {k: 1 for k in self.qubits_decay.keys()} def _successors(self, node, dag): - for _, successor, edge_data in dag.edges(node): - if not isinstance(successor, DAGOpNode): - continue - if isinstance(edge_data, Qubit): + """Return an iterable of the successors along each wire from the given node. + + This yields the same successor multiple times if there are parallel wires (e.g. two adjacent + operations that have one clbit and qubit in common), which is important in the swapping + algorithm for detecting if each wire has been accounted for.""" + for _, successor, _ in dag.edges(node): + if isinstance(successor, DAGOpNode): yield successor def _is_resolved(self, node): """Return True if all of a node's predecessors in dag are applied.""" - return self.applied_predecessors[node] == len(node.qargs) + return self.applied_predecessors[node] == len(node.qargs) + len(node.cargs) def _obtain_extended_set(self, dag, front_layer): """Populate extended_set by looking ahead a fixed number of gates. diff --git a/releasenotes/notes/fix-sabreswap-clbits-428eb5f3a46063da.yaml b/releasenotes/notes/fix-sabreswap-clbits-428eb5f3a46063da.yaml new file mode 100644 index 000000000000..0401e5a36f74 --- /dev/null +++ b/releasenotes/notes/fix-sabreswap-clbits-428eb5f3a46063da.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + Fixed :class:`.SabreSwap`, and by extension :func:`.transpile` with + ``optimization_level=3``, occasionally re-ordering measurements invalidly. + Previously, if two measurements wrote to the same classical bit, + :class:`.SabreSwap` could (depending on the coupling map) re-order them to + produce a non-equivalent circuit. This behaviour was stochastic, so may + not have appeared reliably. diff --git a/test/python/transpiler/test_sabre_swap.py b/test/python/transpiler/test_sabre_swap.py index a19f574a305d..509225f65e75 100644 --- a/test/python/transpiler/test_sabre_swap.py +++ b/test/python/transpiler/test_sabre_swap.py @@ -13,6 +13,7 @@ """Test the Sabre Swap pass""" import unittest +from qiskit.circuit.library import CCXGate, HGate, Measure from qiskit.transpiler.passes import SabreSwap from qiskit.transpiler import CouplingMap, PassManager from qiskit import QuantumRegister, QuantumCircuit @@ -95,6 +96,34 @@ def test_do_not_change_cm(self): self.assertEqual(set(cm_edges), set(coupling.get_edges())) + def test_do_not_reorder_measurements(self): + """Test that SabreSwap doesn't reorder measurements to the same classical bit. + + With the particular coupling map used in this test and the 3q ccx gate, the routing would + invariably the measurements if the classical successors are not accurately tracked. + Regression test of gh-7950.""" + coupling = CouplingMap([(0, 2), (2, 0), (1, 2), (2, 1)]) + qc = QuantumCircuit(3, 1) + qc.compose(CCXGate().definition, [0, 1, 2], []) # Unroll CCX to 2q operations. + qc.h(0) + qc.barrier() + qc.measure(0, 0) # This measure is 50/50 between the Z states. + qc.measure(1, 0) # This measure always overwrites with 0. + passmanager = PassManager(SabreSwap(coupling)) + transpiled = passmanager.run(qc) + + last_h, h_qubits, _ = transpiled.data[-4] + self.assertIsInstance(last_h, HGate) + first_measure, first_measure_qubits, _ = transpiled.data[-2] + second_measure, second_measure_qubits, _ = transpiled.data[-1] + self.assertIsInstance(first_measure, Measure) + self.assertIsInstance(second_measure, Measure) + # Assert that the first measure is on the same qubit that the HGate was applied to, and the + # second measurement is on a different qubit (though we don't care which exactly - that + # depends a little on the randomisation of the pass). + self.assertEqual(h_qubits, first_measure_qubits) + self.assertNotEqual(h_qubits, second_measure_qubits) + if __name__ == "__main__": unittest.main()