Fix split barriers leaking during disjoint layout processing #11655
Conversation
This commit fixes an issue in the disjoint layout processing caused by a shared reference from the input dag to a layout pass to the internal transformations the layout processing performs. As part of the disjoint layout processing the input dag needs to be split into smaller dags for each connected component in the circuit. To enable this any barriers present in the circuit need to be split up prior to analyzing the connected components in the dag. A multiqubit barrier doesn't represent a real connectivity requirement so they need to be split prior to analysis to ensure they don't factor into the connected component computation. To faciliate not losing the semantic meaning of the barrier for the layout analysis of each connected component, the barrier split is done by first taking an n-qubit barrier and converting it into n parallel 1q barriers with a uuid label applied to it. Then after we split the circuit into the separate connected components the uuid is used to identify any components of the same barrier and combine them together. There were two issues with this approach the first is that the splitting of the barriers was done on the input dag without copying first. This causes the input dag to be modified and because in most cases it's a shared instance which would leak out the barrier splitting if the input dag was returned verbatim from the pass, which in most cases it would be. This issue is fixed by ensuring that we re-combine any split barriers after we've split the dag into it's connected components. The second issue is the uuid label assignment would overwrite any existing labels on barriers. While setting labels on barriers is uncommon for users to do (but still completely valid) this is causing a bigger issues since Qiskit#10323 because the transpiler is assigning labels to barriers it creates internally so they can be removed before the circuit is returned. This is also fixed in this commit by appending a uuid to the existing label instead of overwriting it, so we're able to restore the original label when we recombine barriers. Fixes Qiskit#11649
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 7702562231
💛 - Coveralls |
|
I'm on the fence of whether this should be in the changelog and release note or not. The specific bug of leaking UUID labels is only on main for 1.0.0. On 0.45.x we still leaked the split barriers into the output, but the labels were stripped, while weird it was valid and wouldn't cause any issues. The barriers generated during transpile was expected in qiskit < 1.0.0 because we hadn't introduced #10323 yet. |
There was a problem hiding this comment.
This generally looks good to me, thanks, just a comment on the particulars of the name-mangling for inter-function communication.
I think we ought to have a release note about fixing the incorrect barriers - I think we could have been affecting our own subsequent peephole optimisations like OptimizeSwapBeforeMeasure or whatever the one that does diagonal gates is, and a user might have had their own too.
| if node.op.label: | ||
| barrier_uuid = f"{node.op.label}_uuid={uuid.uuid4()}" | ||
| else: | ||
| barrier_uuid = uuid.uuid4() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sure, lets do it in a follow-up I'll open a tracking issue from your comments here,
Summary
This commit fixes an issue in the disjoint layout processing caused by a shared reference from the input dag to a layout pass to the internal transformations the layout processing performs. As part of the disjoint layout processing the input dag needs to be split into smaller dags for each connected component in the circuit. To enable this any barriers present in the circuit need to be split up prior to analyzing the connected components in the dag. A multiqubit barrier doesn't represent a real connectivity requirement so they need to be split prior to analysis to ensure they don't factor into the connected component computation. To faciliate not losing the semantic meaning of the barrier for the layout analysis of each connected component, the barrier split is done by first taking an n-qubit barrier and converting it into n parallel 1q barriers with a uuid label applied to it. Then after we split the circuit into the separate connected components the uuid is used to identify any components of the same barrier and combine them together.
There were two issues with this approach the first is that the splitting of the barriers was done on the input dag without copying first. This causes the input dag to be modified and because in most cases it's a shared instance which would leak out the barrier splitting if the input dag was returned verbatim from the pass, which in most cases it would be. This issue is fixed by ensuring that we re-combine any split barriers after we've split the dag into it's connected components. The second issue is the uuid label assignment would overwrite any existing labels on barriers. While setting labels on barriers is uncommon for users to do (but still completely valid) this is causing a bigger issues since #10323 because the transpiler is assigning labels to barriers it creates internally so they can be removed before the circuit is returned. This is also fixed in this commit by appending a uuid to the existing label instead of overwriting it, so we're able to restore the original label when we recombine barriers.
Details and comments
Fixes #11649