Fully port Optimize1qGatesDecomposition to Rust #12959

mtreinish · 2024-08-14T16:46:08Z

Summary

This commit builds off of #12550 and the other data model in Rust
infrastructure and migrates the Optimize1qGatesDecomposition pass to
operate fully in Rust. The full path of the transpiler pass now never
leaves rust until it has finished modifying the DAGCircuit. There is
still some python interaction necessary to handle parts of the data
model that are still in Python, mainly calibrations and parameter
expressions (for global phase). But otherwise the entirety of the pass
operates in rust now.

This is just a first pass at the migration here, it moves the pass to be
a single for loop in rust. The next steps here are to look at operating
the pass in parallel. There is no data dependency between the
optimizations being done by the pass so we should be able to the
throughput of the pass by leveraging multithreading to handle each run
in parallel. This commit does not attempt this though, because of the
Python dependency and also the data structures around gates and the
dag aren't really setup for multithreading yet and there likely will
need to be some work to support that (this pass is a good candidate to
work through the bugs on that).

Details and comments

Part of #12208

This PR is based on top of #12550 and as such github shows the entire contents of #12550 in addition to the contents of this PR. To see the contents of this PR you can look at HEAD on this branch, or just look at:
0c8b668

TODO:

Rebase after [DAGCircuit Oxidation] Port DAGCircuit to Rust #12550 merges
Benchmark and tune the implementation
Investigate if there is any dead code in the euler_one_qubit_decomposer module

qiskit-bot · 2024-08-14T16:46:14Z

One or more of the following people are relevant to this code:

@enavarro51
@Qiskit/terra-core
@kevinhartman
@levbishop
@mtreinish
@nkanazawa1989

mtreinish · 2024-08-14T16:46:58Z

I ran some asv benchmarks on this and it's yielding very good looking performance improvements:

Benchmarks that have improved:

| Change   | Before [76eb568c] <use-dag-1q-decompose~9^2>   | After [7eaa9544] <use-dag-1q-decompose>   |   Ratio | Benchmark (Parameter)                                                                                         |
|----------|------------------------------------------------|-------------------------------------------|---------|---------------------------------------------------------------------------------------------------------------|
| -        | 5.46±0.02ms                                    | 1.23±0ms                                  |    0.23 | passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(5, 1024, ['rx', 'ry', 'rz', 'r', 'rxx', 'id'])  |
| -        | 13.9±0.06ms                                    | 2.96±0.04ms                               |    0.21 | passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(14, 1024, ['rx', 'ry', 'rz', 'r', 'rxx', 'id']) |
| -        | 25.4±0.2ms                                     | 4.02±0.02ms                               |    0.16 | passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(20, 1024, ['rx', 'ry', 'rz', 'r', 'rxx', 'id']) |
| -        | 4.11±0.01ms                                    | 595±20μs                                  |    0.14 | passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(5, 1024, ['u', 'cx', 'id'])                     |
| -        | 16.4±0.03ms                                    | 2.05±0.01ms                               |    0.13 | passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(20, 1024, ['u', 'cx', 'id'])                    |
| -        | 5.85±0.01ms                                    | 782±10μs                                  |    0.13 | passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(5, 1024, ['rz', 'x', 'sx', 'cx', 'id'])         |
| -        | 15.6±0.06ms                                    | 1.95±0.02ms                               |    0.12 | passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(14, 1024, ['rz', 'x', 'sx', 'cx', 'id'])        |
| -        | 11.9±0.04ms                                    | 1.47±0.02ms                               |    0.12 | passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(14, 1024, ['u', 'cx', 'id'])                    |
| -        | 30.4±0.3ms                                     | 2.65±0.04ms                               |    0.09 | passes.MultipleBasisPassBenchmarks.time_optimize_1q_decompose(20, 1024, ['rz', 'x', 'sx', 'cx', 'id'])        |

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

coveralls · 2024-08-14T17:13:46Z

Pull Request Test Coverage Report for Build 10634728662

Warning: 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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

306 of 351 (87.18%) changed or added relevant lines in 6 files are covered.
230 unchanged lines in 8 files lost coverage.
Overall coverage decreased (-0.06%) to 89.132%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/dag_circuit.rs	92	114	80.7%
crates/accelerate/src/euler_one_qubit_decomposer.rs	189	212	89.15%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	94.02%
crates/accelerate/src/two_qubit_decompose.rs	1	90.84%
qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py	2	94.17%
crates/qasm2/src/lex.rs	4	92.23%
crates/accelerate/src/euler_one_qubit_decomposer.rs	9	91.1%
crates/qasm2/src/parse.rs	12	97.15%
crates/circuit/src/circuit_data.rs	49	90.54%
crates/circuit/src/operations.rs	152	88.26%

Totals
Change from base Build 10598013395:	-0.06%
Covered Lines:	71802
Relevant Lines:	80557

💛 - Coveralls

This commit builds off of Qiskit#12959 and the other data model in Rust infrastructure and migrates the InverseCancellation pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. There is still some python interaction necessary to handle parts of the data model that are still in Python, mainly for handling parameter expressions. But otherwise the entirety of the pass operates in rust now. This is just a first pass at the migration here, it moves the pass to use loops in rust. The next steps here are to look at operating the pass in parallel. There is no data dependency between the optimizations being done for different inverse gates/pairs so we should be able to the throughput of the pass by leveraging multithreading to handle each inverse option in parallel. This commit does not attempt this though, because of the Python dependency and also the data structures around gates and the dag aren't really setup for multithreading yet and there likely will need to be some work to support that. Fixes Qiskit#12271 Part of Qiskit#12208

This commit builds off of Qiskit#12550 and the other data model in Rust infrastructure and migrates the Optimize1qGatesDecomposition pass to operate fully in Rust. The full path of the transpiler pass now never leaves rust until it has finished modifying the DAGCircuit. There is still some python interaction necessary to handle parts of the data model that are still in Python, mainly calibrations and parameter expressions (for global phase). But otherwise the entirety of the pass operates in rust now. This is just a first pass at the migration here, it moves the pass to be a single for loop in rust. The next steps here are to look at operating the pass in parallel. There is no data dependency between the optimizations being done by the pass so we should be able to the throughput of the pass by leveraging multithreading to handle each run in parallel. This commit does not attempt this though, because of the Python dependency and also the data structures around gates and the dag aren't really setup for multithreading yet and there likely will need to be some work to support that (this pass is a good candidate to work through the bugs on that). Part of Qiskit#12208

mtreinish · 2024-08-23T17:46:54Z

Now that #12550 has merged I've rebased this and it's ready for review.

This commit builds off of Qiskit#12959 and the other data model in Rust infrastructure and migrates the InverseCancellation pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. There is still some python interaction necessary to handle parts of the data model that are still in Python, mainly for handling parameter expressions. But otherwise the entirety of the pass operates in rust now. This is just a first pass at the migration here, it moves the pass to use loops in rust. The next steps here are to look at operating the pass in parallel. There is no data dependency between the optimizations being done for different inverse gates/pairs so we should be able to the throughput of the pass by leveraging multithreading to handle each inverse option in parallel. This commit does not attempt this though, because of the Python dependency and also the data structures around gates and the dag aren't really setup for multithreading yet and there likely will need to be some work to support that. Fixes Qiskit#12271 Part of Qiskit#12208

Since we only are storing 12 enum fields (which are a single byte) using any heap allocated collection is completely overkill and will have more overhead that storing a statically sized array for all 12 variants. This commit adds a new struct that wraps a `[bool; 12]` to track which basis are supported and an API for tracking this. This simplifies the tracking of which qubit supports which EulerBasis, it also means other internal users of the 1q decomposition have a simplified API for working with the euler basis.

crates/accelerate/src/nlayout.rs

ShellyGarion

I think that euler_one_qubit_decomposer.rs should be moved to crates/accelerate/src/synthesis/one_qubit to match with the structure of the current synthesis library.
Similarly, it would be good if two_qubit_decompose.rs would be moved to crates/accelerate/src/synthesis/two_qubits

mtreinish · 2024-08-26T10:32:02Z

I think that euler_one_qubit_decomposer.rs should be moved to crates/accelerate/src/synthesis/one_qubit to match with the structure of the current synthesis library.
Similarly, it would be good if two_qubit_decompose.rs would be moved to crates/accelerate/src/synthesis/two_qubits

We can reorganize these modules in a follow up PR. When I created these modules there wasn't any other synthesis in rust so there wasn't anywhere else to put them. I think it'll probably be best to do this at the same time we create a transpiler crate so the Optimize1qGatesDecomposition function that I modify here is separate from the base synthesis code.

…method

ElePT

Thanks a lot @mtreinish!! Apart from the cool optimizations, the PR also includes a lot of handy dag circuit utilities, and I think we would all benefit from having it merged soon (I definitely will). I left a few questions (just for my info) and a tiny non-blocking reno suggestion, but LGMT.

crates/accelerate/src/euler_one_qubit_decomposer.rs

crates/accelerate/src/nlayout.rs

crates/accelerate/src/target_transpiler/mod.rs

releasenotes/notes/optimize-1q-gates-decomposition-ce111961b6782ee0.yaml

…82ee0.yaml Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

This commit builds off of Qiskit#12959 and the other data model in Rust infrastructure and migrates the InverseCancellation pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. There is still some python interaction necessary to handle parts of the data model that are still in Python, mainly for handling parameter expressions. But otherwise the entirety of the pass operates in rust now. This is just a first pass at the migration here, it moves the pass to use loops in rust. The next steps here are to look at operating the pass in parallel. There is no data dependency between the optimizations being done for different inverse gates/pairs so we should be able to the throughput of the pass by leveraging multithreading to handle each inverse option in parallel. This commit does not attempt this though, because of the Python dependency and also the data structures around gates and the dag aren't really setup for multithreading yet and there likely will need to be some work to support that. Fixes Qiskit#12271 Part of Qiskit#12208

This commit migrates the entirety of the CheckMap analysis pass to Rust. The pass operates solely in the rust domain and returns an `Option<(String, [u32; 2])>` to Python which is used to set the two property set fields appropriately. All the analysis of the dag is done in Rust. There is still Python interaction required though because control flow operations are only defined in Python. However the interaction is minimal and only to get the circuits for control flow blocks and converting them into DAGs (at least until Qiskit#13001 is complete). This commit is based on top of Qiskit#12959 and will need to be rebased after that merges. Closes Qiskit#12251 Part of Qiskit#12208

* Fully port CheckMap to Rust This commit migrates the entirety of the CheckMap analysis pass to Rust. The pass operates solely in the rust domain and returns an `Option<(String, [u32; 2])>` to Python which is used to set the two property set fields appropriately. All the analysis of the dag is done in Rust. There is still Python interaction required though because control flow operations are only defined in Python. However the interaction is minimal and only to get the circuits for control flow blocks and converting them into DAGs (at least until #13001 is complete). This commit is based on top of #12959 and will need to be rebased after that merges. Closes #12251 Part of #12208 * Use a Vec<Qubit> for wire_map instead of a HashMap This commit switches to using a Vec<Qubit> for the internal wire_map used to map control flow qubits. A HashMap was originally used because in Python a dictionary is used. However, in the rust domain the inner qubits are contiguous integers starting from 0 so a Vec can be used for better performance in the case we have control flow. * Update crates/accelerate/src/check_map.rs Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> --------- Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>

* Fully port InverseCancellation to Rust This commit builds off of #12959 and the other data model in Rust infrastructure and migrates the InverseCancellation pass to operate fully in Rust. The full path of the transpiler pass now never leaves Rust until it has finished modifying the DAGCircuit. There is still some python interaction necessary to handle parts of the data model that are still in Python, mainly for handling parameter expressions. But otherwise the entirety of the pass operates in rust now. This is just a first pass at the migration here, it moves the pass to use loops in rust. The next steps here are to look at operating the pass in parallel. There is no data dependency between the optimizations being done for different inverse gates/pairs so we should be able to the throughput of the pass by leveraging multithreading to handle each inverse option in parallel. This commit does not attempt this though, because of the Python dependency and also the data structures around gates and the dag aren't really setup for multithreading yet and there likely will need to be some work to support that. Fixes #12271 Part of #12208 * Remove temporary variable for chunk empty check * Destructure gate pairs * Rework short circuit logic

Since 8e5fab6 (Qiskitgh-12959), this branch has been dead, and the Rust method alluded to in it (`compute_error_one_qubit_sequence`) was removed. This is occasionally - but for some reason not always - detected by `pylint`, and it's a true error so should be removed. In fact, almost all of the Python code in `Optimize1qDecomposition` is dead from the perspective of that transpiler pass. However, `Optimize1qCommutation` has no concept of safe API boundaries, and is in fact still using all of these methods (and the default `UnitarySynthesis` plugin does a little too). A follow-up commit can reorganise the code to fix this, but this commit is intended to be mergeable faster, to unblock a couple of PRs that are triggering the pylint failure.

…13188) Since 8e5fab6 (gh-12959), this branch has been dead, and the Rust method alluded to in it (`compute_error_one_qubit_sequence`) was removed. This is occasionally - but for some reason not always - detected by `pylint`, and it's a true error so should be removed. In fact, almost all of the Python code in `Optimize1qDecomposition` is dead from the perspective of that transpiler pass. However, `Optimize1qCommutation` has no concept of safe API boundaries, and is in fact still using all of these methods (and the default `UnitarySynthesis` plugin does a little too). A follow-up commit can reorganise the code to fix this, but this commit is intended to be mergeable faster, to unblock a couple of PRs that are triggering the pylint failure.

mtreinish added performance Changelog: New Feature Include in the "Added" section of the changelog Rust This PR or issue is related to Rust code in the repository mod: transpiler Issues and PRs related to Transpiler labels Aug 14, 2024

mtreinish added this to the 1.3 beta milestone Aug 14, 2024

mtreinish requested review from nonhermitian, a team, alexanderivrii and ShellyGarion as code owners August 14, 2024 16:46

mtreinish force-pushed the use-dag-1q-decompose branch 2 times, most recently from 6ebfff0 to 5c9d285 Compare August 15, 2024 11:36

jlapeyre self-requested a review August 15, 2024 12:08

mtreinish force-pushed the use-dag-1q-decompose branch from 5c9d285 to 621e60c Compare August 15, 2024 18:26

mtreinish mentioned this pull request Aug 15, 2024

[DAGCircuit Oxidation] Port DAGCircuit to Rust #12550

Merged

9 tasks

mtreinish force-pushed the use-dag-1q-decompose branch 3 times, most recently from 8569121 to 7eaa954 Compare August 17, 2024 11:51

sbrandhsn mentioned this pull request Aug 20, 2024

Oxidize Commutation Analysis #12995

Merged

mtreinish force-pushed the use-dag-1q-decompose branch from 7eaa954 to 18eeab7 Compare August 20, 2024 19:03

mtreinish mentioned this pull request Aug 21, 2024

Fully port InverseCancellation to Rust #13013

Merged

2 tasks

mtreinish force-pushed the use-dag-1q-decompose branch from 18eeab7 to 0c8b668 Compare August 22, 2024 16:46

mtreinish force-pushed the use-dag-1q-decompose branch from 0c8b668 to 317a057 Compare August 23, 2024 17:45

mtreinish changed the title ~~[WIP] Fully port Optimize1qGatesDecomposition to Rust~~ Fully port Optimize1qGatesDecomposition to Rust Aug 23, 2024

jakelishman reviewed Aug 25, 2024

View reviewed changes

crates/accelerate/src/nlayout.rs Outdated Show resolved Hide resolved

ShellyGarion reviewed Aug 26, 2024

View reviewed changes

mtreinish added 7 commits August 26, 2024 06:52

Remove From trait for Qubit->PhysicalQubit conversion

9b43ea7

Merge branch 'main' into use-dag-1q-decompose

0f12c27

Merge remote-tracking branch 'origin/main' into use-dag-1q-decompose

a0e4592

Fix merge conflict

d759381

Merge remote-tracking branch 'origin/main' into use-dag-1q-decompose

5ae3269

Use new DAGCircuit::has_control_flow() for control_flow_op_nodes() py…

9828e37

…method

Move _basis_gates set creation to __init__

21ccc8f

ElePT self-assigned this Aug 29, 2024

ElePT previously approved these changes Aug 30, 2024

View reviewed changes

mtreinish dismissed ElePT’s stale review via e371cfc August 30, 2024 14:49

Update releasenotes/notes/optimize-1q-gates-decomposition-ce111961b67…

e371cfc

…82ee0.yaml Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

ElePT approved these changes Aug 30, 2024

View reviewed changes

ElePT enabled auto-merge August 30, 2024 15:03

ElePT added this pull request to the merge queue Aug 30, 2024

Merged via the queue into Qiskit:main with commit 8e5fab6 Aug 30, 2024
15 checks passed

jakelishman mentioned this pull request Sep 13, 2024

Reformat pyext to avoid merge conflicts #13147

Merged

jakelishman mentioned this pull request Sep 19, 2024

Remove dead code branch from Optimize1qDecomposition Python code #13188

Merged

mtreinish deleted the use-dag-1q-decompose branch February 19, 2025 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully port Optimize1qGatesDecomposition to Rust #12959

Fully port Optimize1qGatesDecomposition to Rust #12959

mtreinish commented Aug 14, 2024 •

edited

Loading

qiskit-bot commented Aug 14, 2024

mtreinish commented Aug 14, 2024 •

edited

Loading

coveralls commented Aug 14, 2024 •

edited

Loading

mtreinish commented Aug 23, 2024

ShellyGarion left a comment •

edited

Loading

mtreinish commented Aug 26, 2024

ElePT left a comment

Fully port Optimize1qGatesDecomposition to Rust #12959

Fully port Optimize1qGatesDecomposition to Rust #12959

Conversation

mtreinish commented Aug 14, 2024 • edited Loading

Summary

Details and comments

qiskit-bot commented Aug 14, 2024

mtreinish commented Aug 14, 2024 • edited Loading

coveralls commented Aug 14, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10634728662

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

mtreinish commented Aug 23, 2024

ShellyGarion left a comment • edited Loading

Choose a reason for hiding this comment

mtreinish commented Aug 26, 2024

ElePT left a comment

Choose a reason for hiding this comment

mtreinish commented Aug 14, 2024 •

edited

Loading

mtreinish commented Aug 14, 2024 •

edited

Loading

coveralls commented Aug 14, 2024 •

edited

Loading

ShellyGarion left a comment •

edited

Loading