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.

Sign up for GitHub

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 split barriers leaking during disjoint layout processing #11655

Merged
merged 5 commits into from
Jan 30, 2024
Merged

Fix split barriers leaking during disjoint layout processing #11655

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a234d47
Fix split barriers leaking during disjoint layout processing
mtreinish Jan 26, 2024
bd9cdf5
Merge remote-tracking branch 'origin/main' into fix-barrier-split-leak
mtreinish Jan 29, 2024
56d7d6a
Always use string uuid
mtreinish Jan 29, 2024
02fd819
Merge remote-tracking branch 'origin/main' into fix-barrier-split-leak
mtreinish Jan 29, 2024
43fb7d8
Add release note
mtreinish Jan 29, 2024
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
22 changes: 18 additions & 4 deletions qiskit/transpiler/passes/layout/disjoint_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,10 @@ def split_barriers(dag: DAGCircuit):
num_qubits = len(node.qargs)
if num_qubits == 1:
continue
barrier_uuid = uuid.uuid4()
if node.op.label:
barrier_uuid = f"{node.op.label}_uuid={uuid.uuid4()}"
else:
barrier_uuid = uuid.uuid4()
Copy link
Member

Choose a reason for hiding this comment

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

Can we unconditionally string-ify the UUID here? The first branch does, the second doesn't, and it's technically breaking the typing assumptions of Instruction.label - it would be valid for a pass to examine the labels and assume that they should be str | None.

As a follow-up: perhaps instead of the name mangling, we could return a {new_label: old_label} map from this function, which we then pass into combine_barriers so it can restore them? That has a side benefit of letting it be sure it only targets Barriers that were split by this operation, without needing the "uuid=" in label or isinstance(label, UUID) heuristic to guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I was using the UUID type here to differentiate between "" and None. But I can add a variant to the prefix to handle that.

As for the follow-up about the return the issue is that combine_barriers isn't always called consistently through every call path. There are several paths where we still have split barriers that get processed in the layout pass and those could manually call combine_barriers (sabre does for example). We'd have to update all of those to work with this return type, but it's more of a risk regarding out of tree layout passes.

Copy link
Member

Choose a reason for hiding this comment

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

If there's multiple places that call these utilities, I think it's even more important that we have the re-combiner target specifically the desired barriers - it could be possible for a pass dependent or one of the places where we use an "inner" pass manager within a single pass to call the barrier splitter and recombiner while there are split barriers from the outer pass, which could desync the outer logic.

If backwards compatibility is a concern, we can leave this PR's logic in place in there's no dictionary given to the combine_barriers, but document that the argument always should be passed. If you want to do that in a follow-up, though, I'm ok with it.

In fancier language, I think it's a liability for us to have non-reentrant logic in a utility function here, when we have in-tree places that call pass managers recursively.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, lets do it in a follow-up I'll open a tracking issue from your comments here,

Copy link
Member

Choose a reason for hiding this comment

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

#11671

split_dag = DAGCircuit()
split_dag.add_qubits([Qubit() for _ in range(num_qubits)])
for i in range(num_qubits):
Expand All @@ -124,10 +127,15 @@ def combine_barriers(dag: DAGCircuit, retain_uuid: bool = True):
qubit_indices = {bit: index for index, bit in enumerate(dag.qubits)}
uuid_map: dict[uuid.UUID, DAGOpNode] = {}
for node in dag.op_nodes(Barrier):
if isinstance(node.op.label, uuid.UUID):
barrier_uuid = node.op.label
if node.op.label:
if isinstance(node.op.label, uuid.UUID) or (
isinstance(node.op.label, str) and "_uuid=" in node.op.label
):
barrier_uuid = node.op.label
else:
continue
if barrier_uuid in uuid_map:
other_node = uuid_map[node.op.label]
other_node = uuid_map[barrier_uuid]
num_qubits = len(other_node.qargs) + len(node.qargs)
new_op = Barrier(num_qubits, label=barrier_uuid)
new_node = dag.replace_block_with_op([node, other_node], new_op, qubit_indices)
Expand All @@ -138,6 +146,9 @@ def combine_barriers(dag: DAGCircuit, retain_uuid: bool = True):
for node in dag.op_nodes(Barrier):
if isinstance(node.op.label, uuid.UUID):
node.op.label = None
if isinstance(node.op.label, str) and "_uuid=" in node.op.label:
original_label = "_uuid=".join(node.op.label.split("_uuid=")[:-1])
node.op.label = original_label


def require_layout_isolated_to_component(
Expand Down Expand Up @@ -202,4 +213,7 @@ def separate_dag(dag: DAGCircuit) -> List[DAGCircuit]:
new_dag.remove_clbits(*idle_clbits)
combine_barriers(new_dag)
decomposed_dags.append(new_dag)
# Reverse split barriers on input dag to avoid leaking out internal transformations as
# part of splitting
combine_barriers(dag, retain_uuid=False)
return decomposed_dags
13 changes: 13 additions & 0 deletions test/python/compiler/test_transpiler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3234,6 +3234,19 @@ def test_transpile_target_with_qubits_without_ops_circuit_too_large_disconnected
with self.assertRaises(TranspilerError):
transpile(qc, target=target, optimization_level=opt_level)

@data(0, 1, 2, 3)
def test_barrier_no_leak_disjoint_connectivity(self, opt_level):
"""Test that we don't leak an internal labelled barrier from disjoint layout processing."""
cmap = CouplingMap([(0, 1), (1, 2), (3, 4)])
qc = QuantumCircuit(cmap.size(), cmap.size())
qc.cx(0, 1)
qc.cx(1, 2)
qc.cx(2, 0)
qc.cx(3, 4)
qc.measure(qc.qubits, qc.clbits)
out = transpile(qc, coupling_map=cmap, optimization_level=opt_level)
self.assertNotIn("barrier", out.count_ops())

@data(0, 1, 2, 3)
def test_transpile_does_not_affect_backend_coupling(self, opt_level):
"""Test that transpiliation of a circuit does not mutate the `CouplingMap` stored by a V2
Expand Down
Loading