Add min_qubits kwarg to UnitarySynthesis pass #6349
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I ran the QV benchmarks with this commit applied: Based on this I'm thinking it might make sense to drop the extra |
|
I ran a larger set of benchmarks including more qv to see the impact for larger circuits: It looks like this reduces the output depth for the 50x20 model circuit but also significantly increases the run time. Not sure what the best path forward here is anymore |
This commit adds the UnitarySynthesis pass to the preset pass manager anywhere unrolling of custom or wide gates was done. Previously, we only ever called the UnitarySynthesis pass in the preset pass managers if the basis translation method was set to 'synthesis' or we were using level3 and then it was part of the optimization pass (as part of 2q peephole optimization). This was an issue in that we're implicitly calling the _define() method of the class whenever we're unrolling gates >= 3q or gates with a custom definition. The _define() method is basically identical to the UnitarySynthesis pass except it doesn't expose the options to set a basis gate or approximation degree, which would result in the output gates from unitary gates in the unroll steps are always in the U3 basis. This meant we would be converting unitary gates to u3 (and cx) which would result in a conversion to the target basis later, even if we could just go to the target basis directly. This is also future proofing for Qiskit#6124 where a plugin interface is added to the UnitarySynthesis pass and can potentially be used for arbitrary sized unitaries. At the same time this change caught an issue qith the SingleQubitUnitary gate where the name was duplicated with the UnitaryGate which would result in errors when UnitarySynthesis was called because the UnitarySynthesis pass looks for gate named 'unitary' to run on. This is fixed and the SingleQubitUnitary gate's name is changed to 'squ' to differentiate it from the UnitaryGate.
mtreinish
force-pushed
the
unitary-synthesis-unroll
branch
from
00a071d to
a501fce
Compare
|
I like the direction in which this is heading (I think |
|
This is still just decomposing unitaries, right? e.g. a Toffoli would not be expanded using UnitarySynthesis (which would be very inefficient). What do you mean by this @kdk ?
|
This commit adds a new option to the UnitarySynthesis pass constructor, min_qubits, which is used to specify a minimimum size unitary to synthesize. If the unitary is smaller than that it will be skipped. This is then used by the UnitarySynthesis instance in the unroll3q phase so we don't decompose 1 or 2q unitaries before routing.
Right, this keeps the same unitary synthesis approach, but tries to apply it in a more uniform and general way. Before this, a
For the places we use |
|
@mtreinish Do you have a set of updated benchmark numbers after 401aedf ? |
|
I just reran the benchmarks with the current state of the PR here are the results: |
In Qiskit#6124 which recently merged the UnitarySynthesis pass was added to preset passmanager to enable synthesis plugins that operate on > 2q from having an impact (otherwise they wouldn't be called). However that PR didn't have the option to avoid synthesizing 2q unitaries too early. This commit updates the base branch to the latest state of main which includes Qiskit#6124. This means this PR just adds a min_qubits option to UnitarySynthesis and leverages it for the unroll3q stage in the preset pass managers.
mtreinish
changed the title
|
So now that #6124 has merged I think this is more important. (mostly because I forgot about this until just now) Since the current state of main we'll be synthesizing unitaries too early without this PR. |
mtreinish
added
the
Changelog: New Feature
|
I ran numbers again comparing Although |
kdk
approved these changes
Sep 30, 2021
There was a problem hiding this comment.
This LGTM. It would be good to compare this to main to see if the earlier regression ( after 401aedf in #6349 (comment) ) was resolved, but only because as I understand it, this PR should be functionally identical to what had been on main for those cases, just with the synthesis occurring under a different pass. That can wait though until after we get the benchmarking results back and see where we stand.
|
Well the only places this will be different is because we added extra passes to the pipeline compared to the state prior to #6124 and even the min_qubits=3 only case that results in an extra dag iteration (from calling I do have it on my todo list to add a retworkx side topological order search function similar to: Qiskit/rustworkx#456 to try and accelerate these cases a bit |
|
I also ran larger QV comparisons just now and looked at depth too: |
|
So comparing it to After this merges I'll see what else I can come up with to fix that remaining ~10-20% overhead. |
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Oct 5, 2021
This commit tweaks the preset passmanager definitions to not include the UnitarySynthesis pass as part of basis translation if the 'unitary' gate is in the basis gates. Previously in Qiskit#6124 and Qiskit#6349 the preset passmanagers were updated to leverage the user specified unitary synthesis method instead of implicitly using the default as part of the unroll 3q and unroll custom definition passes. This however had the unintended side effect of always synthesizing unitary gates even if it was in the basis (which is the case on Aer). This commit fixes this by only running unitary synthesis as part of the basis translation step if it's not in the basis. A potential follow-on here is to make the unroll 3q transpiler pass basis aware (since right now it will implicitly run unitary synthesis internally) and add a similar logic around the use of the UnitarySynthesis pass for unrolling 3q or larger gates. The unroll 3q transpiler stage suffers from this same issue. However, this wasn't done here because the way 3q unrolling is used is to reduce the gates to be less than 3 qubits so the layout and routing phase can deal work with the gates in the circuit would likely cause issues if a unitary gate larger than 2 qubits was in the circuit being transpiled.
mtreinish
added a commit
that referenced
this pull request
Oct 6, 2021
) * Ensure UnitaryGate's are preserved by transpile if in basis gates This commit tweaks the preset passmanager definitions to not include the UnitarySynthesis pass as part of basis translation if the 'unitary' gate is in the basis gates. Previously in #6124 and #6349 the preset passmanagers were updated to leverage the user specified unitary synthesis method instead of implicitly using the default as part of the unroll 3q and unroll custom definition passes. This however had the unintended side effect of always synthesizing unitary gates even if it was in the basis (which is the case on Aer). This commit fixes this by only running unitary synthesis as part of the basis translation step if it's not in the basis. A potential follow-on here is to make the unroll 3q transpiler pass basis aware (since right now it will implicitly run unitary synthesis internally) and add a similar logic around the use of the UnitarySynthesis pass for unrolling 3q or larger gates. The unroll 3q transpiler stage suffers from this same issue. However, this wasn't done here because the way 3q unrolling is used is to reduce the gates to be less than 3 qubits so the layout and routing phase can deal work with the gates in the circuit would likely cause issues if a unitary gate larger than 2 qubits was in the circuit being transpiled. * Skip level 3 in tests The level 3 preset passmanager by default runs 2q block collection which is then synthesized into a unitary. As long as we're running that in the pass manager it's not really possible right now to preserve input unitaries through the transpilation. A potential follow on PR could potential skip the synthesis pass if 'unitary' is in the basis so the optimization pass would just collect 2q blocks into a unitary and pass that to the backend directly. However, that's not in scope for this PR (and it's not clear if we want that or not). * Revert to main preset passmanager logic As was pointed out by @jakelishman in code review. Prior to #6124 the unitary synthesis pass was checking the basis for the presence of unitary (and swap gates if pulse_optimize=True) in the basis set and skipping the gates in the pass directly instead of in the pass manager. This logic was lost when we added the plugin interface in #6124 (probably as part of a rebase as it was a long lived branch). Since we used to have the logic in the pass itself this commit changes the approach to not adjust the preset passmanager usage and just make the pass detect if unitary is in the basis and not synthesize. This simplifies the logic and makes it less error prone. At the same time the test coverage is expanded to ensure we preserve this behavior moving forward (as aer's testing requires it). * Add level 3 back to tests * Improve efficieny if gates to synthesize are in basis
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This commit adds the UnitarySynthesis pass to the preset pass manager
anywhere unrolling of custom or wide gates was done. Previously, we only
ever called the UnitarySynthesis pass in the preset pass managers if the
basis translation method was set to 'synthesis' or we were using level3
and then it was part of the optimization pass (as part of 2q peephole
optimization). This was an issue in that we're implicitly calling the
_define() method of the class whenever we're unrolling gates >= 3q or
gates with a custom definition. The _define() method is basically
identical to the UnitarySynthesis pass except it doesn't expose the
options to set a basis gate or approximation degree, which would result
in the output gates from unitary gates in the unroll steps are always in
the U3 basis. This meant we would be converting unitary gates to u3 (and
cx) which would result in a conversion to the target basis later, even
if we could just go to the target basis directly.
This is also future proofing for #6124 where a plugin interface is addedAs #6124 merged prior to this PR this commit adds ato the UnitarySynthesis pass and can potentially be used for arbitrary
sized unitaries.
min_qubitsoption to theUnitarySynthesispass to restrict the size ofunitary gates the pass will synthesize.
At the same time this change caught an issue with the SingleQubitUnitary
gate where the name was duplicated with the UnitaryGate which would
result in errors when UnitarySynthesis was called because the
UnitarySynthesis pass looks for gate named 'unitary' to run on. This is
fixed and the SingleQubitUnitary gate's name is changed to 'squ' to
differentiate it from the UnitaryGate.
Details and comments