-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add ElidePermutations transpiler pass #9523
Conversation
This commit adds a new transpiler pass ElideSwaps which is a logical optimization pass designed to run prior to layout or any other physical embedding steps in the transpiler pipeline. It traverses the DAG in topological order and when a swap gate is encountered it removes that gate and instead permutes the logical qubits for any subsequent gates in the DAG. This will eliminate any swaps in a circuit not caused by routing. Additionally, this pass is added to the preset pass manager for optimization level 3, we can consider adding it to other levels too if we think it makes sense (there is little overhead, and almost 0 if there are no swaps). One thing to note is that this pass does not recurse into control flow blocks at all, it treats them as black box operations. So if any control flow blocks contain swap gates those will not be optimized away. This change was made because mapping the permutation outside and inside any control flow block was larger in scope than what the intent for this pass was. That type of work is better suited for the routing passes which already have to reason about this.
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
elif getattr(node.op, "condition", None) is not None: | ||
new_dag.apply_operation_back(node.op, _apply_mapping(node.qargs), node.cargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good thinking - I'd not have thought about this edge case.
Added a new optimization transpiler pass, :class:`~.ElideSwaps`, | ||
which is designed to run prior to the :ref:`layout_stage` and will | ||
optimize away any :class:`~.SwapGate`\s in a circuit by permuting virtual | ||
qubits. For example, taking a circuit with :class:`~.SwapGate`\s: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we mention that routing will still add required swaps into the circuit but that this strictly enables more opportunity for improvements in routing? That might be too much detail for the note, though.
I just realized that I need to update the layout in the property set as part of this pass too, it's breaking 2 tests that are checking unitary equivalence and the |
The new ElideSwap pass is causing an output permutation just as a routing pass would. This information needs to be passed through to the output in case there is no layout or routing run. In those cases the information about the output permutation caused by the swap elision will be lost and doing layout aware operations like Operator.from_circuit() will not be able to reason about the permutation. This commit fixes this by inserting the original layout and qubit mapping into the property set along with the final layout. Then the base pass class and pass manager class are updated to use the original layout as the initial layout if one isn't set. In cases where we run layout and routing the output metadata from those passes will superscede these new metadata fields.
This commit fixes 2 issues in the execution of the new ElideSwaps pass as part of optimization level 3. First we were running it too late in the process both after high level synthesis (which isn't relavant yet, but will be soon when this is expanded to elide all permutations not just swaps) and also after reset diagonal before measurement. The second issue is that if the user is specifying to run with a manual layout set we should not run this pass, as it will interfere with the user intent.
There were 2 issues with the permutation tracking done in an earlier commit. First, it conflicted with the final_layout property set via routing (or internally by the routing done in the combined sabre layout). This was breaking conditional checks in the preset pass manager around embedding. To fix this a different property is used and set as the output final layout if no final layout is set. The second issue was the output layout object was not taking into account a set initial layout which will permute the qubits and cause the output to not be up to date. This is fixed by updating apply layout to apply the initial layout to the elision_final_layout in the property set.
This commit generalizes the pass from just considering swap gates to all permutations (via the PermutationGate class). This enables the pass to elide additional circuit permutations, not just the special case of a swap gate. The pass and module are renamed accordingly to ElidePermutations and elide_permutations respectively.
Pull Request Test Coverage Report for Build 4577141342
💛 - Coveralls |
This commit fixes the recently added handling of the PermutationGate so that it correctly is mapping the qubits. The previous iteration of this messed up the mapping logic so it wasn't valid.
This is still failing one test locally for me, but it's a lot closer than it was when I left it ~6 months ago. The failure is in the case where we elide swaps but do not set a layout as part of the transpiler. In that case the EDIT: This has been fixed in: 5688293 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this LGTM, thanks for taking this over the finish line @alexanderivrii! I left a couple of inline suggestions that I'll apply. But from my side this looks good, I'm not able to approve or merge this as I'm the original author of the PR though.
if oq not in virtual_permutation_layout: | ||
virtual_permutation_layout[oq] = original_qubit_indices[oq] | ||
|
||
t_qubits = dag.qubits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the t_
prefix stand for here? For readability, can we write it out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what his intent was with the prefix either, but we can always clean it up in a follow up PR.
``final_layout`` property set based on ``layout`` and | ||
``virtual_permutation_layout``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From your discussion above @jakelishman , it sounds to me like we ought to consider keeping any new properties private until we i.e. refactor to storing final_permutation
.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine not documenting it explicitly as part of the API although at the end of the day the pass doesn't do much in the absence of a virtual permutation layout. So it's kind of hard to keep it private as part of the pass. I don't think it's adding an extra burden realistically to support it for the remainder of 1.x if we pivot to something else in the future we have to do this already with final_layout
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, part of us not being able to get the final_permutation
landed properly for 1.1 is that basically have to support this for the whole 1.x cycle now, but that was priced into the decision - it shouldn't be too painful, and we'd have needed a 2.0 release to make a truly clean break from external routing passes anyway.
self.assertEqual(res, expected) | ||
|
||
|
||
class TestElidePermutationsInTranspileFlow(QiskitTestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also test adding the pass in invalid positions (e.g. after layout), and having both it and FinalizeLayout
run twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a test for the after layout case. But did you have something in mind for the double case? It's a bit tricky in practice as the pass really isn't designed to be run more than once. The above todo you commented on in: #9523 (comment) is for the case where you have virtual_permutation_layout
already set and ensure the pass composes the layouts correctly. We could workaround that by calling FinalizeLayouts
before the second execution, but in practice there isn't anything to eliminate a second time unless we insert more swaps after the first time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the test case for after layout in: f5b6778
Fwiw (since I'm assigned) I read through this quickly and there's nothing I immediately wanted to interject beyond what Kevin/Matt already said. |
Actually, scratch that, I do have a comment - do we think it's definitely better to have |
FWIW, this is what I did in the original PR. I can adjust back to this, it makes sense to me to always handle this at the pass manager level with the other layout handling. |
This commit integrates the function that finalize layouts was performing into the passmanager harnesss. We'll always need to run the equivalent of finalize layout if any passes are setting a virtual permutation so using a standalone pass that can be forgotten is potentially error prone. This inlines the logic as part of the passmanager's output preparation stage so we always finalize the layout.
I integrated finalize layouts in the pass manager in: e755bcd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the discussions and previous reviews it looks like there are some points that need to be polished in the future, but I think the PR is in a good enough state to move forward, the test coverage is good and the tradeoffs of different design choices have been documented. I only have a couple of minor suggestions based on previous suggestions that weren't applied, other than that, LGTM.
- | | ||
Added a new optimization transpiler pass, :class:`~.ElideSwaps`, | ||
which is designed to run prior to the :ref:`layout_stage` and will | ||
optimize away any :class:`~.SwapGate`\s in a circuit by permuting virtual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optimize away any :class:`~.SwapGate`\s in a circuit by permuting virtual | |
optimize away any :class:`~.SwapGate`\s or :class:`~.PermutationGate`\s in a circuit by permuting virtual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has been updated already. The current release note text says:
Added a new optimization transpiler pass, :class:`~.ElidePermutations`,
which is designed to run prior to the :ref:`layout_stage` and will
optimize away any :class:`~.SwapGate`\s and
:class:`~qiskit.circuit.library.PermutationGate`\s
in a circuit by permuting virtual
qubits. For example, taking a circuit with :class:`~.SwapGate`\s:
qc.cx(1, 0) | ||
qc.measure_all() | ||
|
||
ElideSwaps()(qc).draw("mpl") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case you want to add this info:
ElideSwaps()(qc).draw("mpl") | |
ElideSwaps()(qc).draw("mpl") | |
This pass has been added to `optimization_level=3` in `generate_default_pass_manager()`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one isn't actually the case yet. We're doing this in #12111 , we split out from this PR to test it in isolation (ideally with more time before the release than we actually have now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sloppy reviewing on my side, or maybe I was tricked by GitHub :) Anyways, with those comments addressed LGTM. Thanks for the hard work to all the people involved!!
* Add ElideSwaps transpiler pass This commit adds a new transpiler pass ElideSwaps which is a logical optimization pass designed to run prior to layout or any other physical embedding steps in the transpiler pipeline. It traverses the DAG in topological order and when a swap gate is encountered it removes that gate and instead permutes the logical qubits for any subsequent gates in the DAG. This will eliminate any swaps in a circuit not caused by routing. Additionally, this pass is added to the preset pass manager for optimization level 3, we can consider adding it to other levels too if we think it makes sense (there is little overhead, and almost 0 if there are no swaps). One thing to note is that this pass does not recurse into control flow blocks at all, it treats them as black box operations. So if any control flow blocks contain swap gates those will not be optimized away. This change was made because mapping the permutation outside and inside any control flow block was larger in scope than what the intent for this pass was. That type of work is better suited for the routing passes which already have to reason about this. * Update tests with optimization level 3 * Pass final layout from ElideSwap to output The new ElideSwap pass is causing an output permutation just as a routing pass would. This information needs to be passed through to the output in case there is no layout or routing run. In those cases the information about the output permutation caused by the swap elision will be lost and doing layout aware operations like Operator.from_circuit() will not be able to reason about the permutation. This commit fixes this by inserting the original layout and qubit mapping into the property set along with the final layout. Then the base pass class and pass manager class are updated to use the original layout as the initial layout if one isn't set. In cases where we run layout and routing the output metadata from those passes will superscede these new metadata fields. * Move pass in opt level 3 earlier in stage and skip with explicit layout This commit fixes 2 issues in the execution of the new ElideSwaps pass as part of optimization level 3. First we were running it too late in the process both after high level synthesis (which isn't relavant yet, but will be soon when this is expanded to elide all permutations not just swaps) and also after reset diagonal before measurement. The second issue is that if the user is specifying to run with a manual layout set we should not run this pass, as it will interfere with the user intent. * Doc and copy paste fixes * Expand test coverage * Update permutation tracking There were 2 issues with the permutation tracking done in an earlier commit. First, it conflicted with the final_layout property set via routing (or internally by the routing done in the combined sabre layout). This was breaking conditional checks in the preset pass manager around embedding. To fix this a different property is used and set as the output final layout if no final layout is set. The second issue was the output layout object was not taking into account a set initial layout which will permute the qubits and cause the output to not be up to date. This is fixed by updating apply layout to apply the initial layout to the elision_final_layout in the property set. * Generalize pass to support PermutationGate too This commit generalizes the pass from just considering swap gates to all permutations (via the PermutationGate class). This enables the pass to elide additional circuit permutations, not just the special case of a swap gate. The pass and module are renamed accordingly to ElidePermutations and elide_permutations respectively. * Fix permutation handling This commit fixes the recently added handling of the PermutationGate so that it correctly is mapping the qubits. The previous iteration of this messed up the mapping logic so it wasn't valid. * Fix formatting * Fix final layout handling for no initial layout * Improve documentation and log a warning if run post layout * Fix final layout handling with no ElideSwaps being run * Fix docs build * Fix release note * Fix typo * Add test for routing and elide permutations * Apply suggestions from code review Co-authored-by: Jim Garrison <jim@garrison.cc> * Rename test file to test_elide_permutations.py * Apply suggestions from code review Co-authored-by: Kevin Hartman <kevin@hart.mn> * Fix test import after rebase * fixing failing test cases this should pass CI after merging Qiskit#12057 * addresses kehas comments - thx * Adding FinalyzeLayouts pass to pull the virtual circuit permutation from ElidePermutations to the final layout * formatting * partial rebase on top of 12057 + tests * also need test_operator for partial rebase * removing from transpiler flow for now; reworking elide tests * also adding permutation gate to the example * also temporarily reverting test_transpiler.py * update to release notes * minor fixes * Apply suggestions from code review * Fix lint * Update qiskit/transpiler/passes/optimization/elide_permutations.py * Add test to test we skip after layout * Integrate FinalizeLayouts into the PassManager harness This commit integrates the function that finalize layouts was performing into the passmanager harnesss. We'll always need to run the equivalent of finalize layout if any passes are setting a virtual permutation so using a standalone pass that can be forgotten is potentially error prone. This inlines the logic as part of the passmanager's output preparation stage so we always finalize the layout. * Compose a potential existing virtual_permutation_layout * Remove unused import --------- Co-authored-by: Jim Garrison <jim@garrison.cc> Co-authored-by: Kevin Hartman <kevin@hart.mn> Co-authored-by: Sebastian Brandhofer <148463728+sbrandhsn@users.noreply.github.com> Co-authored-by: AlexanderIvrii <alexi@il.ibm.com> Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Summary
This commit adds a new transpiler pass ElidePermutations which is a logical optimization pass designed to run prior to layout or any other physical embedding steps in the transpiler pipeline. It traverses the DAG in topological order and when a swap gate is encountered it removes that gate and instead permutes the logical qubits for any subsequent gates in the DAG. This will eliminate any swaps or permutation gates in a circuit not caused by routing.
Additionally, this pass is added to the preset pass manager for optimization level 3, we can consider adding it to other levels too if we think it makes sense (there is little overhead, and almost 0 if there are no swaps).edit: integrating the pas into the preset pass manager turned out a bit tricky and is delegated to a follow-up PR. in addition, when
ElidePermutations
is present, we do not needRemoveSwapsBeforeMeasure
.One thing to note is that this pass does not recurse into control flow blocks at all, it treats them as black box operations. So if any control flow blocks contain swap gates those will not be optimized away. This change was made because mapping the permutation outside and inside any control flow block was larger in scope than what the intent for this pass was. That type of work is better suited for the routing passes which already have to reason about this.
Update [by @alexanderivrii ]: This development ended up as a joined effort between @mtreinish, @sbrandhsn and myself. Here are a few additional details. This pass is meant to run on the virtual circuit, before layout and routing. Let$Q$ denote the original virtual circuit. The pass moves all the swaps and permutation gates to the end of the circuit, essentially representing $Q$ as $S - P$ , some circuit $S$ followed by a permutation $P$ (of the virtual qubits). The idea is that we can "save" P in the property set's "virtual_permutation_layout" attribute, "forget" about $P$ , i.e. transpile $S$ , and then incorporate $P$ at the very end by mapping it to a permutation of the physical circuit's qubits and combining (composing) it with the permutation coming from routing. The precise scheme is implemented as an additional $L: V\rightarrow P$ and $F: P\rightarrow P$ respectively denote the "layout permutation" and the "final permutation" for $S$ , then for $Q$ the layout permutation is still $S$ while the final permutation is $L^{-1} - P^{-1} - L - F$ . One point is that in order to do this update we need to know the layout permutation which is of course not available at the time of $P$ "on the side" and update "final permutation" in $L^{-1} - P^{-1} - L$ in " either
FinalizeLayouts
transpiler pass that is supposed to run at the very end of the transpilation flow. IfElidePermutations
pass. This is why we saveFinalizeLayouts
. One alternative was to constructSetLayout
orApplyLayout
passes, however they do not always run, depending on whether the coupling map is specified or not. So, after a discussion between the three of us, introducing an additional passFinalizeLayouts
made a lot of sense. In addition, it becomes possible to run multiple pre-layout "up to final permutation" passes, such as theStarPreRouting
pass followed byElidePermutations
, in which case one should compose the permutations in "virtual_permutation_layout".This should be rebased on top of #12057 (i.e. the code in
operator.py
andtest_operator.py
is not a part of this PR). HavingElidePermutations
pass andFinalizeLayouts
in separate PRs did not make much sense, so we did not split these into two.Let me also explain the "strange" update of the final layout above. Suppose that transpiling a virtual circuit$S$ produces the transpiled circuit $T$ , initial layout $L$ and final layout $F$ . These are related via $$T = L^{-1} - S - L - F.$$ $S - P$ , $S$ followed by a permutation $P$ .
$$T = L^{-1} - S - L - F = (L^{-1} - S - P - L) - (L^{-1} - P ^{-1} - L - F),$$ $T$ is still the resulting transpiled circuit for $S - P$ , $L$ is still the initial layout$, and
$(L^{-1} - P ^{-1} - L - F)$ is the new final layout, as claimed.
Suppose that we want to transpile
We have:
which can be interpreted as follows:
Details and comments
Co-authored-by: Alexander Ivrii alexi@il.ibm.com