Add equivalence library entry for swap to ECR or CZ #12312
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
This commit adds two new equivalence library entries to cover the conversion from a SWAP gate to either ecr or cz directly. These are common 2q basis gates and without these entries in the equivalence library the path found from a lookup ends up with a much less efficient translation. This commit adds the two new entries so that the BasisTranslator will use a more efficient decomposition from the start when targeting these basis. This will hopefully result in less work for the optimization stage as the output will already be optimal and not require simplification. Testing for this PR is handled automatically by the built-in testing harness in test_gate_definitions.py that evaluates all the entries in the standard equivalence library for unitary equivalence.
mtreinish
added
performance
Changelog: None
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 8921883699Details
💛 - Coveralls |
ShellyGarion
reviewed
May 1, 2024
|
I ran the two qubit basis decomposer to generate these circuits: from qiskit.synthesis import TwoQubitBasisDecomposer
from qiskit.circuit.library import ECRGate, CZGate, SwapGate
decomp_ecr = TwoQubitBasisDecomposer(ECRGate(), euler_basis="ZSXX")
decomp_cz = TwoQubitBasisDecomposer(CZGate(), euler_basis="ZSXX")
decomp_ecr(SwapGate().to_matrix())
decomp_cz(SwapGate().to_matrix())and also went about it a different way to verify by using the basis translator and then ran @alexanderivrii was checking the results with this applied and found that doing this resulted in a modest reduction of gates when doing a full transpile in some 100 qubit workloads in the asv benchmarks. I'm about to kick off an asv run myself to see what effect this has on runtime. |
|
I ran some of the asv benchmarks with this PR: |
|
which 1-qubit gates could we use for the equivalence? here is another possible definition for a |
|
In general it shouldn't matter too much as long as there is a path from the gates in the circuit to the target. I picked the basis here specifically because it's what IBM backends use and it is the most efficient to give the direct output with the minimum gates as the basis translator will see it can do that with 1 transform and opt for that. This will avoid needing to clean it up later during the optimization stage. I like your definition though because it keeps everything in the clifford gates so people targeting the stabilizer simulator can use it (although I think aer recognizes that pi/2 angles for rz are cliffords now). We can add that definition to the library too, there isn't any harm in having multiple options available, the lookup should be fairly fast regardless and more options typically leads to better output. |
|
here is another simple definition for a |
|
Oh actually, I think we might already have these definitions effectively in the library just not explicit entries. Looking at https://github.com/Qiskit/qiskit/blob/main/qiskit/circuit/library/standard_gates/equivalence_library.py#L1288-L1302 we're already using that definition for cx -> cz. Also for cx -> ecr: https://github.com/Qiskit/qiskit/blob/main/qiskit/circuit/library/standard_gates/equivalence_library.py#L728-L745. So when the basis translator goes from swap -> ecr it can find this by doing swap -> cx -> ecr. I'm not sure we need explicit entries in this case because that path existed already. For example: >>> from qiskit import QuantumCircuit, transpile
>>> qc = QuantumCircuit(2)
>>> qc.swap(0, 1)
>>> print(transpile(qc, basis_gates=['h', 'cz'], optimization_level=0))
┌───┐ ┌───┐
q_0: ──────■─┤ H ├─■─┤ H ├─■──────
┌───┐ │ ├───┤ │ ├───┤ │ ┌───┐
q_1: ┤ H ├─■─┤ H ├─■─┤ H ├─■─┤ H ├
└───┘ └───┘ └───┘ └───┘
>>> print(transpile(qc, basis_gates=['sxdg', 'sdg', 'x', 'ecr'], optimization_level=0))
global phase: 3π/4
┌─────┐ ┌──────┐ ┌───┐ ┌──────┐┌──────┐┌─────┐ ┌──────┐┌───┐
q_0: ┤ Sdg ├─┤0 ├─┤ X ├─┤ √Xdg ├┤1 ├┤ Sdg ├────────┤0 ├┤ X ├
├─────┴┐│ Ecr │┌┴───┴┐└──────┘│ Ecr │└┬───┬┘┌──────┐│ Ecr │└───┘
q_1: ┤ √Xdg ├┤1 ├┤ Sdg ├────────┤0 ├─┤ X ├─┤ √Xdg ├┤1 ├─────
└──────┘└──────┘└─────┘ └──────┘ └───┘ └──────┘└──────┘ The extra cases help here because the synthesis output are not efficient when targeting |
As fallout from the addition of SingletonGate and SingletonControlledGate we were accidentally not running large portions of the unit tests which validate the default session equivalence library. This test dynamically runs based on all members of the standard gate library by looking at all defined subclasses of Gate and ControlledGate. But with the introduction of SingletonGate and SingletonControlledGate all the unparameterized gates in the library were not being run through the tests. This commit fixes this to catch that the swap definition added in the previous commit on this PR branch used an incorrect definition of SwapGate using ECRGate. The definition will be fixed in a follow up PR.
The previous definition of a swap gate using ECR rz and sx was incorrect and also not as efficient as possible. This was missed because the tests were accidently broken since Qiskit#10314 which was fixed in the previous commit. This commit updates the definition to use one that is actually correct and also more efficient with fewer 1 qubit gates. Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
|
And here is one more variant of converting the SWAP gate to 3 ECR + single-qubits gates: which is actually exactly the second example from #12312 (comment) after pushing all the Pauli (X) gates to the end of the circuit (where they happen to cancel out). |
Thanks, I updated the PR to use this in: 1a0521e and also updated the tests in 4e90c08 which were inadvertently broken by #10314 and it's follow-on PRs. The definition I was using before was incorrect because of typos on my part and the tests didn't flag that which has been corrected now. |
jlapeyre
reviewed
May 1, 2024
ShellyGarion
reviewed
May 2, 2024
|
Here is an even shorter decomposition of SWAP into CZ + SX that Shelly and I have just found (no need for RZ gate): The two circuits are equal (as operators). And here is a picture: P.S. We have used qiskit-sat-synthesis for the task. :) |
Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com> Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
|
Ok sure, updated in: d8d819f |
|
See https://algassert.com/post/1717 on why this works :-) I think it is awesome that qiskit-sat-synthesis generates a decomposition that is lower in depth than the approach in the link! :-) Is the decomposition optimal in number of gates/depth? |
|
I've rerun the asv benchmarks with the current state of this PR with the more efficient decompositions: |
alexanderivrii
approved these changes
May 2, 2024
ElePT
pushed a commit
to ElePT/qiskit
that referenced
this pull request
May 31, 2024
* Add equivalence library entry for swap to ECR or CZ This commit adds two new equivalence library entries to cover the conversion from a SWAP gate to either ecr or cz directly. These are common 2q basis gates and without these entries in the equivalence library the path found from a lookup ends up with a much less efficient translation. This commit adds the two new entries so that the BasisTranslator will use a more efficient decomposition from the start when targeting these basis. This will hopefully result in less work for the optimization stage as the output will already be optimal and not require simplification. Testing for this PR is handled automatically by the built-in testing harness in test_gate_definitions.py that evaluates all the entries in the standard equivalence library for unitary equivalence. * Add name to annotated gate circuit in qpy backwards compat tests * Fix equivalence library tests As fallout from the addition of SingletonGate and SingletonControlledGate we were accidentally not running large portions of the unit tests which validate the default session equivalence library. This test dynamically runs based on all members of the standard gate library by looking at all defined subclasses of Gate and ControlledGate. But with the introduction of SingletonGate and SingletonControlledGate all the unparameterized gates in the library were not being run through the tests. This commit fixes this to catch that the swap definition added in the previous commit on this PR branch used an incorrect definition of SwapGate using ECRGate. The definition will be fixed in a follow up PR. * Use a more efficient and actually correct circuit for ECR target The previous definition of a swap gate using ECR rz and sx was incorrect and also not as efficient as possible. This was missed because the tests were accidently broken since Qiskit#10314 which was fixed in the previous commit. This commit updates the definition to use one that is actually correct and also more efficient with fewer 1 qubit gates. Co-authored-by: Alexander Ivrii <alexi@il.ibm.com> * Update ECR circuit diagram in comment * Simplify cz equivalent circuit * Simplify cz circuit even more Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com> Co-authored-by: Alexander Ivrii <alexi@il.ibm.com> --------- Co-authored-by: Alexander Ivrii <alexi@il.ibm.com> Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com>
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 two new equivalence library entries to cover the conversion from a SWAP gate to either ecr or cz directly. These are common 2q basis gates and without these entries in the equivalence library the path found from a lookup ends up with a much less efficient translation. This commit adds the two new entries so that the BasisTranslator will use a more efficient decomposition from the start when targeting these basis. This will hopefully result in less work for the optimization stage as the output will already be optimal and not require simplification.
Testing for this PR is handled automatically by the built-in testing harness in test_gate_definitions.py that evaluates all the entries in the standard equivalence library for unitary equivalence.
Details and comments