Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SabreSwap with classically conditioned gates #8041

Merged
merged 6 commits into from
May 12, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions qiskit/transpiler/passes/routing/sabre_swap.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ def __init__(
self.heuristic = heuristic
self.seed = seed
self.fake_run = fake_run
self.applied_predecessors = None
self.required_predecessors = None
self.qubits_decay = None
self._bit_indices = None
self.dist_matrix = None
Expand Down Expand Up @@ -188,12 +188,9 @@ def run(self, dag):
self.qubits_decay = dict.fromkeys(dag.qubits, 1)

# Start algorithm from the front layer and iterate until all gates done.
self.required_predecessors = self._build_required_predecessors(dag)
num_search_steps = 0
front_layer = dag.front_layer()
self.applied_predecessors = defaultdict(int)
for _, input_node in dag.input_map.items():
for successor in self._successors(input_node, dag):
self.applied_predecessors[successor] += 1

while front_layer:
execute_gate_list = []
Expand Down Expand Up @@ -228,7 +225,7 @@ def run(self, dag):
for node in execute_gate_list:
self._apply_gate(mapped_dag, node, current_layout, canonical_register)
for successor in self._successors(node, dag):
self.applied_predecessors[successor] += 1
self.required_predecessors[successor] -= 1
if self._is_resolved(successor):
front_layer.append(successor)

Expand Down Expand Up @@ -314,6 +311,15 @@ def _reset_qubits_decay(self):
"""
self.qubits_decay = {k: 1 for k in self.qubits_decay.keys()}

def _build_required_predecessors(self, dag):
out = defaultdict(int)
# We don't need to count in- or out-wires: outs can never be predecessors, and all input
# wires are automatically satisfied at the start.
for node in dag.op_nodes():
for successor in self._successors(node, dag):
out[successor] += 1
Comment on lines +318 to +320
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if doing something like:

Suggested change
for node in dag.op_nodes():
for successor in self._successors(node, dag):
out[successor] += 1
for node in dag.op_nodes():
out_successor[node] += dag._multi_graph.in_degree(node)

would work and be faster. Not that I expect this to be a bottleneck. It just took me a sec to grok this.
I guess there might be an issue because of the input nodes as we'd be including the first layer nodes and they'd have a required predecessor count for the inputs. That's not too different than what this outputs now because we'll end up with output nodes in the out dict but that should be ok because we'll never do a lookup on an output wire.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will count DAGInNodes too? We ought to discount those to do things properly, but we could just loop through the original front_layer and automatically set those to zero.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I clearly didn't read your comment properly sorry!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets just leave this as is. We can revisit this after #7977 and see if we're actually seeing this in profiles or not.

return out

def _successors(self, node, dag):
"""Return an iterable of the successors along each wire from the given node.

Expand All @@ -326,22 +332,22 @@ def _successors(self, node, dag):

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) + len(node.cargs)
return self.required_predecessors[node] == 0

def _obtain_extended_set(self, dag, front_layer):
"""Populate extended_set by looking ahead a fixed number of gates.
For each existing element add a successor until reaching limit.
"""
extended_set = []
incremented = []
decremeented = []
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
tmp_front_layer = front_layer
done = False
while tmp_front_layer and not done:
new_tmp_front_layer = []
for node in tmp_front_layer:
for successor in self._successors(node, dag):
incremented.append(successor)
self.applied_predecessors[successor] += 1
decremeented.append(successor)
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
self.required_predecessors[successor] -= 1
if self._is_resolved(successor):
new_tmp_front_layer.append(successor)
if len(successor.qargs) == 2:
Expand All @@ -350,8 +356,8 @@ def _obtain_extended_set(self, dag, front_layer):
done = True
break
tmp_front_layer = new_tmp_front_layer
for node in incremented:
self.applied_predecessors[node] -= 1
for node in decremeented:
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
self.required_predecessors[node] += 1
return extended_set

def _obtain_swaps(self, front_layer, current_layout):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
fixes:
- |
The :class:`.SabreSwap` transpiler pass, used in :func:`.transpile` when
``routing_method="sabre"`` is set, will no longer sporadically drop
classically conditioned gates and their successors from circuits during the
routing phase of transpilation. See
`#8040 <https://github.com/Qiskit/qiskit-terra/issues/8040>`__.
29 changes: 27 additions & 2 deletions test/python/transpiler/test_sabre_swap.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
import ddt

from qiskit.circuit.library import CCXGate, HGate, Measure, SwapGate
from qiskit.transpiler.passes import SabreSwap
from qiskit.transpiler.passes import SabreSwap, TrivialLayout
from qiskit.transpiler import CouplingMap, PassManager
from qiskit import QuantumRegister, QuantumCircuit
from qiskit import ClassicalRegister, QuantumRegister, QuantumCircuit
from qiskit.test import QiskitTestCase
from qiskit.utils import optionals

Expand Down Expand Up @@ -237,6 +237,31 @@ def leak_number_of_swaps(cls, *args, **kwargs):
out_results = sim.run(routed, shots=4096).result().get_counts()
self.assertEqual(set(in_results), set(out_results))

def test_classical_condition(self):
"""Test that :class:`.SabreSwap` correctly accounts for classical conditions in its
reckoning on whether a node is resolved or not. If it is not handled correctly, the second
gate might not appear in the output.

Regression test of gh-8040."""
with self.subTest("1 bit in register"):
qc = QuantumCircuit(2, 1)
qc.z(0)
qc.z(0).c_if(qc.cregs[0], 0)
cm = CouplingMap([(0, 1), (1, 0)])
expected = PassManager([TrivialLayout(cm)]).run(qc)
actual = PassManager([TrivialLayout(cm), SabreSwap(cm)]).run(qc)
self.assertEqual(expected, actual)
with self.subTest("multiple registers"):
cregs = [ClassicalRegister(3), ClassicalRegister(4)]
qc = QuantumCircuit(QuantumRegister(2, name="q"), *cregs)
qc.z(0)
qc.z(0).c_if(cregs[0], 0)
qc.z(0).c_if(cregs[1], 0)
cm = CouplingMap([(0, 1), (1, 0)])
expected = PassManager([TrivialLayout(cm)]).run(qc)
actual = PassManager([TrivialLayout(cm), SabreSwap(cm)]).run(qc)
self.assertEqual(expected, actual)


if __name__ == "__main__":
unittest.main()