Speedup the CommutationChecker and the CommutativeCancellation
#12859
Conversation
- list of pre-approved gates we know we support commutation on - less redirections in function calls - commutation analysis to only trigger search on gates that are actually cancelled
Cryoris
added
performance
Changelog: New Feature
|
One or more of the following people are relevant to this code:
|
releasenotes/notes/commutation-checker-gates-15349cfa13b40e50.yaml
Outdated
Show resolved
Hide resolved
qiskit/transpiler/passes/optimization/commutative_cancellation.py
Outdated
Show resolved
Hide resolved
-- these need updating only on the version with parameter support
|
|
||
| self._z_rotations = {"p", "z", "u1", "rz", "t", "s"} | ||
| self._x_rotations = {"x", "rx"} | ||
| self._gates = {"cx", "cy", "cz", "h", "y"} # Now the gates supported are hard-coded |
There was a problem hiding this comment.
That's a great PR!
Does this pass only checks self commuting gates? or also pairs of gates, for example:
- Any 1q gate in _z_rotations commutes with the control of CX
- Any 1q gate in _x_rotations commutes with the target of CX
- Any 1q gate in _z_rotations commutes with the control and target of CZ
There was a problem hiding this comment.
Regarding CX, the pass does the following:
┌───┐ ┌───┐ ┌─────────┐
q_0: ──■──┤ S ├───┤ T ├───┤ Rz(2.1) ├──■──
┌─┴─┐├───┤┌──┴───┴──┐└─────────┘┌─┴─┐
q_1: ┤ X ├┤ X ├┤ Rx(1.2) ├───────────┤ X ├
└───┘└───┘└─────────┘ └───┘
┌────────────┐
q_0: ┤ Rz(4.4562) ├
├────────────┤
q_1: ┤ Rx(4.3416) ├
└────────────┘
and then same for CZ. However, the pass could use some updating as it cannot handle e.g. Sdg or Tdg gates, meaning that the following circuit is unaffected by the pass
┌─────┐
q_0: ──■──┤ Sdg ├──■──
┌─┴─┐└─────┘┌─┴─┐
q_1: ┤ X ├───────┤ X ├
└───┘ └───┘
In practice that is caught by re-synthesizing 2q unitaries but we could do it cheaper with the commutative cancellation. Let's do that in a follow up and also add support for SX gates.
There was a problem hiding this comment.
why not to add sdg and tdg to self._z_rotations ?
There was a problem hiding this comment.
That's exactly what to do, but I'd rather do it in a follow up and not change the functionality so close to the 1.2 release (this PR should only affect runtime). I can open the follow-up the next days and we can discuss separately 🙂
Pull Request Test Coverage Report for Build 10194702496Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
mtreinish
added
Changelog: None
There was a problem hiding this comment.
Overall this LGTM, I have one inline question about a change to the tests. But I'm not super worried about it, feel free to enqueue for merge after answering it (unless it was a mistake). This is a good improvement, I think there might be some other low hanging fruit we can pick up on after merging this, in addition to the capabilities improvements discussed in the comments already.
| circuit.rz(-np.pi / 2, 0) | ||
|
|
||
| passmanager = PassManager(CommutativeInverseCancellation(matrix_based=matrix_based)) | ||
| passmanager = PassManager(CommutativeInverseCancellation()) |
There was a problem hiding this comment.
So I get why you did this because a parameterized circuit can't use matrix based inverse cancellation. But I'm wondering how it passed before, and what this PR did to change that.
There was a problem hiding this comment.
Good catch, this should not have been changed! That was a leftover from when we added parameter support and I forgot to revert this for 1.2 🙂 Reverted in 91850b8.
mtreinish
added
the
stable backport potential
…12859) * Faster commutation checker and analysis - list of pre-approved gates we know we support commutation on - less redirections in function calls - commutation analysis to only trigger search on gates that are actually cancelled * cleanup comments * add reno * review comments * revert accidentially changed tests -- these need updating only on the version with parameter support * revert changes in test_comm_inv_canc --------- Co-authored-by: MarcDrudis <MarcSanzDrudis@outlook.com> (cherry picked from commit 441925e)
…12859) (#12876) * Faster commutation checker and analysis - list of pre-approved gates we know we support commutation on - less redirections in function calls - commutation analysis to only trigger search on gates that are actually cancelled * cleanup comments * add reno * review comments * revert accidentially changed tests -- these need updating only on the version with parameter support * revert changes in test_comm_inv_canc --------- Co-authored-by: MarcDrudis <MarcSanzDrudis@outlook.com> (cherry picked from commit 441925e) Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>
…iskit#12859) * Faster commutation checker and analysis - list of pre-approved gates we know we support commutation on - less redirections in function calls - commutation analysis to only trigger search on gates that are actually cancelled * cleanup comments * add reno * review comments * revert accidentially changed tests -- these need updating only on the version with parameter support * revert changes in test_comm_inv_canc --------- Co-authored-by: MarcDrudis <MarcSanzDrudis@outlook.com>
Summary
Speedup the
CommutationCheckerandCommutativeCancellation. Note that this does not yet add support for parametrized gates.Details
In
CommutationChecker, we skip a set of checks by adding a set of pre-approved gates that we know we can compute the commutations of. We also add a feature to compute commutations only on a subset of gates, whichCommutativeCancellationcan use, as it only checks cancellations for certain gates.Speeeeeed data
To check I was running the compilation of a 100-qubit QFT, given by
and on my laptop the transpile time is reduced by a factor of 2 (averaged over 10 repetitions):
For more detailed information,
on main, the commutation analysis takes ~85% of the transpile time:
with this PR the time is still high, but reduced to ~66%:
Finally, asv is also happy, here testing the utility scale benchmarks (though I actually would've expected more improvement from the numbers above):