Skip to content

Conversation

mtreinish
Copy link
Member

Summary

In the Unroll3qOrMore pass internally is quite simple it iterates over every operation in the circuit and for any operation that uses >= 3 qubits and recursively decompsing it into all 1 and 2 qubit operations, converting it to a DAGCircuit and substituting the >=3 qubit gates with the equivalent circuit. However, during this DAGCircuit conversion step we're spending a large amount deep copying the operation objects. However, we don't need to do this because nothing in the circuit will be reused with shared references so we can skip the copying and just pass the operation objects by reference onto the DAG (as that's all we need). This commit makes that change by using the copy_operations flag we introduced in #9848 on circuit_to_dag() to disable the internal copying.

Details and comments

In the Unroll3qOrMore pass internally is quite simple it iterates over
every operation in the circuit and for any operation that uses >= 3
qubits and recursively decompsing it into all 1 and 2 qubit operations,
converting it to a DAGCircuit and substituting the >=3 qubit gates with
the equivalent circuit. However, during this DAGCircuit conversion step
we're spending a large amount deep copying the operation objects.
However, we don't need to do this because nothing in the circuit will be
reused with shared references so we can skip the copying and just pass
the operation objects by reference onto the DAG (as that's all we need).
This commit makes that change by using the `copy_operations` flag we
introduced in Qiskit#9848 on circuit_to_dag() to disable the internal copying.
@mtreinish mtreinish added performance Changelog: None Do not include in changelog labels Aug 23, 2023
@mtreinish mtreinish requested a review from a team as a code owner August 23, 2023 19:49
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5955802704

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 17 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.02%) to 87.265%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 2 90.89%
qiskit/pulse/library/waveform.py 3 93.75%
crates/qasm2/src/parse.rs 12 97.13%
Totals Coverage Status
Change from base Build 5952293521: -0.02%
Covered Lines: 74274
Relevant Lines: 85113

💛 - Coveralls

mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Aug 24, 2023
This commit adds a cache for parameter free gate decompositions in
the Unroll3qOrMore transpiler pass. If there are repeated gates in a
circuit that don't have any parameters, then when we decompose the
circuit a single time that decomposition will be valid for all instances
of that gate. This doesn't hold true for parameterized gates because the
parameter value will determine the decomposition used. This greatly
improves the runtime performance of the pass when there are repeated >=3
qubit gates in the circuit.

In a future revision we can investigate adding the parameter values to
the cache lookup. But the hit rate would likely be fairly low in such
cases because it would only provide runtime improvements if there are
repeated >3 qubit gates with the same exact parameter values, which
seems fairly rare in practice.

This is based on top of Qiskit#10702 and will need to be rebased after Qiskit#10702
merges.
Copy link
Contributor

@jlapeyre jlapeyre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine.

@mtreinish mtreinish added this pull request to the merge queue Aug 25, 2023
Merged via the queue into Qiskit:main with commit 6f50483 Aug 25, 2023
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Aug 25, 2023
This commit adds a cache for parameter free gate decompositions in
the Unroll3qOrMore transpiler pass. If there are repeated gates in a
circuit that don't have any parameters, then when we decompose the
circuit a single time that decomposition will be valid for all instances
of that gate. This doesn't hold true for parameterized gates because the
parameter value will determine the decomposition used. This greatly
improves the runtime performance of the pass when there are repeated >=3
qubit gates in the circuit.

In a future revision we can investigate adding the parameter values to
the cache lookup. But the hit rate would likely be fairly low in such
cases because it would only provide runtime improvements if there are
repeated >3 qubit gates with the same exact parameter values, which
seems fairly rare in practice.

This is based on top of Qiskit#10702 and will need to be rebased after Qiskit#10702
merges.
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Aug 25, 2023
This commit adds a cache for parameter free gate decompositions in
the Unroll3qOrMore transpiler pass. If there are repeated gates in a
circuit that don't have any parameters, then when we decompose the
circuit a single time that decomposition will be valid for all instances
of that gate. This doesn't hold true for parameterized gates because the
parameter value will determine the decomposition used. This greatly
improves the runtime performance of the pass when there are repeated >=3
qubit gates in the circuit.

In a future revision we can investigate adding the parameter values to
the cache lookup. But the hit rate would likely be fairly low in such
cases because it would only provide runtime improvements if there are
repeated >3 qubit gates with the same exact parameter values, which
seems fairly rare in practice.

This is based on top of Qiskit#10702 and will need to be rebased after Qiskit#10702
merges.
SameerD-phys pushed a commit to SameerD-phys/qiskit-terra that referenced this pull request Sep 7, 2023
In the Unroll3qOrMore pass internally is quite simple it iterates over
every operation in the circuit and for any operation that uses >= 3
qubits and recursively decompsing it into all 1 and 2 qubit operations,
converting it to a DAGCircuit and substituting the >=3 qubit gates with
the equivalent circuit. However, during this DAGCircuit conversion step
we're spending a large amount deep copying the operation objects.
However, we don't need to do this because nothing in the circuit will be
reused with shared references so we can skip the copying and just pass
the operation objects by reference onto the DAG (as that's all we need).
This commit makes that change by using the `copy_operations` flag we
introduced in Qiskit#9848 on circuit_to_dag() to disable the internal copying.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants