Fully port CheckMap to Rust #13030
Fully port CheckMap to Rust #13030
Conversation
mtreinish
added
performance
Rust
|
One or more of the following people are relevant to this code:
|
|
I was inspired to quickly do this transpiler pass because asv showed a runtime regression in this pass after #12550 merged. Comparing this PR against main (with that regression) shows a huge speedup: (I'm not sure I've ever seen asv report a ratio of |
|
Comparing against 1.2.0 without the regression caused by #12550: |
Pull Request Test Coverage Report for Build 10744272901Details
💛 - Coveralls |
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
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.
mtreinish
force-pushed
the
rust-check-map
branch
from
6924be4
to
341a65b
Compare
Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
| pub fn check_map( | ||
| py: Python, | ||
| dag: &DAGCircuit, | ||
| edge_set: HashSet<[u32; 2]>, |
There was a problem hiding this comment.
Besides dropping circuit_to_dag the other potential performance optimization here is building the hash set during init as a pyclass so we avoid converting from a pyset at call time. We can probably do this in a follow up because realistically this probably won't be noticeable except for the cases when there are a lot of edges in the connectivity graph and minimal gates in the circuit (also CheckMap is only typically called once during a transpile, for determining whether to run SabreSwap or not).
| None => edge_set.contains(&[qubits[0].into(), qubits[1].into()]), | ||
| } | ||
| }; | ||
| for node in dag.op_nodes(false) { |
There was a problem hiding this comment.
Not for this PR, but I wonder if eventually we should add methods like DAGCircuit::op_node_references and have that return an iterator of some node reference type which holds the NodeId and a &PackedInstruction so that users don't need to unwrap. Sort of like what petgraph has for their IntoNodeReferences trait.
There was a problem hiding this comment.
I like the sound of that. Currently a lot of the rust DAGCircuit API is limited, and everyone seems to be adding stuff in their transpiler passes PRs to save time. We could discuss this further in its own issue if you're interested in working on it :)
Summary
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 twoproperty 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).
Details and comments
This commit is based on top of #12959 and will need to be rebased afterRebasedthat merges. In the meantime you can see the contents of this PR at:
41fe453
Closes #12251
Part of #12208