Use BasisTranslator for unroll 3q or more #10776
Merged
Conversation
This file contains hidden or 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
This commit modifies the the preset pass manager construction to by default use the UnrollCustomDefinitions and BasisTranslator passes to perform the unrolling of operations that operate on >= 3 qubits. This is done by leveraging the min_qubits argument added to those passes in this commit which lets you set a minimum number of qubit arguments to translate operations for. Previously, this was handled by the Unroll3qOrMore pass which works by iterating over the DAG and finding all operations with >= 3 qubits and recursively unrolling them until the output is all in terms of 1 or 2 qubit operations. This works fine, but has undesireable runtime performance characteristics because standard gates repeatedly have to build new DAGs to substitute in. For example, if you had a circuit with 50k CCXGates that would result in 50k new DAGs being created from scratch for each instance of CCXGate. A previous attempt was made to add a translation cache to that pass in Qiskit#10703 but as was discussed in code review caching the circuit is unsound as it's possible for a custom gate object to be constructed such that it would be cached and return an invalid substitution circuit. By leveraging the basis translator we only build as many substition DAGs as needed.
|
One or more of the the following people are requested to review this:
|
jakelishman
approved these changes
Sep 5, 2023
There was a problem hiding this comment.
This generally looks correct to me, though it makes me wonder if we'll see any performance regressions in present transpile that have very few custom operations or 3+q gates, since we're now going to run two passes rather than one. It probably shouldn't be very much if so.
This is totally beside the point, but the behaviour described by the release notes is actually still true even if min_qubits is set to zero haha.
|
I left it untagged for merge in case we wanted to look at timings before merge - if not, I'm happy for somebody just to add it to the queue. |
|
Sure, I'll run some asv benchmarks to compare a bunch of different circuits and also do some manual testing with circuits that have large numbers of 3q gates just to get some data for the PR history. |
|
I ran a subset of transpilation asv benchmarks and it was basically some showed a bit of a speed improvement, but one QV test showed a runtime regression (I'm not sure exactly what was going on there but it's also only a 14ms regression so it's not a huge deal from my perspective). The |
|
As for a place where this makes a much bigger impact is running: with On main this takes ~43.695 sec and with this PR it took ~31.078 sec. |
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 modifies the the preset pass manager construction to by default use the UnrollCustomDefinitions and BasisTranslator passes to perform the unrolling of operations that operate on >= 3 qubits. This is done by leveraging the min_qubits argument added to those passes in this commit which lets you set a minimum number of qubit arguments to translate operations for.
Previously, this was handled by the Unroll3qOrMore pass which works by iterating over the DAG and finding all operations with >= 3 qubits and recursively unrolling them until the output is all in terms of 1 or 2 qubit operations. This works fine, but has undesireable runtime performance characteristics because standard gates repeatedly have to build new DAGs to substitute in. For example, if you had a circuit with 50k CCXGates that would result in 50k new DAGs being created from scratch for each instance of CCXGate. A previous attempt was made to add a translation cache to that pass in #10703 but as was discussed in code review caching the circuit is unsound as it's possible for a custom gate object to be constructed such that it would be cached and return an invalid substitution circuit. By leveraging the basis translator we only build as many substition DAGs as needed.
Details and comments