Oxidize commutative cancellation #13091
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 10797636548Warning: 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 |
There was a problem hiding this comment.
Thanks for doing this, this has the potential to improve performance quite a bit, not only by speeding up the cancellation, but also by avoiding all the python overhead for the analysis. I've done a quick first pass over the code. I left a few inline comments, besides those the big thing I think we want to be careful for here is making sure we don't panic needlessly and instead return Python exceptions when there is a possible error condition. Right now the code seems a bit too likely to just call panic!() which won't be use actionable or catchable and also exits hard.
| (0..dag.num_qubits() as u32).for_each(|qubit| { | ||
| let wire = Qubit(qubit); | ||
| if let Some(wire_commutation_set) = commutation_set.get(&Wire::Qubit(wire)) { | ||
| wire_commutation_set | ||
| .iter() | ||
| .enumerate() | ||
| .for_each(|(com_set_idx, com_set)| { | ||
| // This ensures that we only have DAGOPNodes in the current com_set, yuck... | ||
| if let NodeType::Operation(_node0) = &dag.dag[*com_set.first().unwrap()] { | ||
| com_set.iter().for_each(|node| { | ||
| let op = match &dag.dag[*node] { | ||
| NodeType::Operation(instr) => instr, | ||
| _ => panic!("Unexpected type in commutation set."), | ||
| }; | ||
| let num_qargs = dag.get_qargs(op.qubits).len(); | ||
| // no support for cancellation of parameterized gates | ||
| if op | ||
| .params_view() | ||
| .iter() | ||
| .all(|p| !matches!(p, Param::ParameterExpression(_))) | ||
| { | ||
| let op_name = op.op.name().to_string(); | ||
| if num_qargs == 1usize && _gates.contains(op_name.as_str()) { | ||
| single_q_cancellation_sets | ||
| .entry((op_name.clone(), wire, com_set_idx)) | ||
| .or_insert_with(Vec::new) | ||
| .push(*node); | ||
| } | ||
|
|
||
| if num_qargs == 1usize && _z_rotations.contains(op_name.as_str()) { | ||
| single_q_cancellation_sets | ||
| .entry((Z_ROTATION.to_string(), wire, com_set_idx)) | ||
| .or_insert_with(Vec::new) | ||
| .push(*node); | ||
| } | ||
| if num_qargs == 1usize && _x_rotations.contains(op_name.as_str()) { | ||
| single_q_cancellation_sets | ||
| .entry((X_ROTATION.to_string(), wire, com_set_idx)) | ||
| .or_insert_with(Vec::new) | ||
| .push(*node); | ||
| } | ||
| // Don't deal with Y rotation, because Y rotation doesn't commute with | ||
| // CNOT, so it should be dealt with by optimized1qgate pass | ||
| if num_qargs == 2usize | ||
| && dag.get_qargs(op.qubits).first().unwrap() == &wire | ||
| { | ||
| let second_qarg = dag.get_qargs(op.qubits)[1]; | ||
| let q2_key = ( | ||
| op_name, | ||
| wire, | ||
| second_qarg, | ||
| com_set_idx, | ||
| *node_indices | ||
| .get(&(*node, Wire::Qubit(second_qarg))) | ||
| .unwrap(), | ||
| ); | ||
| two_q_cancellation_sets | ||
| .entry(q2_key) | ||
| .or_insert_with(Vec::new) | ||
| .push(*node); | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| }) | ||
| } | ||
| }); |
There was a problem hiding this comment.
I was curious and sketched out using a parallel iterator chain here to build the cancellation sets hashmap using the above suggestion on changing the types:
let cancellation_sets: HashMap<CancellationSetKey, Vec<NodeIndex>> = (0..dag.num_qubits())
.into_par_iter()
.filter_map(|qubit| {
let wire = Qubit(qubit as u32);
if let Some(wire_commutation_set) = commutation_set.get(&Wire::Qubit(wire)) {
Some(wire_commutation_set
.iter()
.enumerate()
.filter_map(|(com_set_index, com_set)| {
// This ensures that we only have DAGOPNodes in the current com_set, yuck...
if let NodeType::Operation(_node0) = &dag.dag[*com_set.first().unwrap()] {
Some(com_set.iter().filter_map(|node| {
let op = match &dag.dag[*node] {
NodeType::Operation(instr) => instr,
_ => panic!("Unexpected type in commutation set."),
};
let num_qargs = dag.get_qargs(op.qubits).len();
// no support for cancellation of parameterized gates
if op
.params_view()
.iter()
.all(|p| !matches!(p, Param::ParameterExpression(_)))
{
let op_name = op.op.name().to_string();
let gate = op.op.standard_gate();
if num_qargs == 1usize && GATES.contains(&gate) {
let key = CancellationSetKey {
gate: GateOrRotation::Gate(gate),
qubits: smallvec![wire],
com_set_index,
second_index: None,
};
Some((key, *node))
} else if num_qargs == 1usize && _z_rotations.contains(op_name.as_str()) {
let key = CancellationSetKey {
gate: GateOrRotation::Z_ROTATION,
qubits: smallvec![wire],
com_set_index,
second_index: None,
};
Some((key, *node))
} else if num_qargs == 1usize && _x_rotations.contains(op_name.as_str()) {
let key = CancellationSetKey {
gate: GateOrRotation::X_ROTATION,
qubits: smallvec![wire],
com_set_index,
second_index: None,
};
Some((key, *node))
}
// Don't deal with Y rotation, because Y rotation doesn't commute with
// CNOT, so it should be dealt with by optimized1qgate pass
else if num_qargs == 2usize
&& dag.get_qargs(op.qubits).first().unwrap() == &wire
{
let second_qarg = dag.get_qargs(op.qubits)[1];
let key = CancellationSetKey {
gate: GateOrRotation::Gate(gate),
qubits: smallvec![wire, second_qarg],
com_set_index,
second_index: Some(*node_indices.get(&(*node, Wire::Qubit(second_qarg))).unwrap()),
};
Some((key, *node))
} else {
None
}
} else {
None
}
}).collect::<Vec<(CancellationSetKey, NodeIndex)>>())
} else {
None
}
})
.flat_map(|entries| entries.into_iter()))
} else {
None
}
})
.fold(|| HashMap::new(), |map, item| {
for (key, value) in item {
map.entry(key).and_modify(|indices: &mut Vec<NodeIndex>| indices.push(value)).or_insert_with(|| vec![value]);
}
map
})
.reduce(|| HashMap::new(), |outer_map, inner_map| {
for (key, value) in inner_map.iter_mut() {
outer_map.entry(*key).and_modify(|indices| indices.append(value)).or_insert_with(|| *value);
}
outer_map
});This doesn't fully compile yet because I haven't changed the logic below it to work with a single hashmap yet. But it at least gets passed the type checking (which probably means the borrow checker will hate it :D). I'm skeptical doing this in parallel actually helps performance here though because it results in several intermediate allocations that could be avoided. It was more just an exercise for me to see how it could be done. But this might be useful as a reference for other changes so I decided to post it even though it's not complete.
qiskit/transpiler/passes/optimization/commutative_cancellation.py
Outdated
Show resolved
Hide resolved
|
Oh also can you please rebase this on main so the commit history is just the local changes. While the final diff is ok, it makes the commit message and authors list for this PR the sum of all the commits on the branch, which is much larger than the actual work being done here. |
858ca8f to
8041ef7
Compare
There was a problem hiding this comment.
Thanks for the quick updates. I took another pass through the code after the last updates. This is looking a lot better now and much less error prone using an enum and rust types instead of string comparisons for everything. I'm still a bit concerned how many panic!() calls there are though, I feel like a fair number of them can be avoided though, and either eliminated with some logical changes or replaced with Python errors. I left some inline comments with suggestions on how to do this.
| total_phase += match definition.global_phase() {Param::Float(f) => f, _ => panic!("PackedInstruction with definition has no global phase set as floating point number")}; | ||
| } |
There was a problem hiding this comment.
As the logic is right now this should return a python error:
| total_phase += match definition.global_phase() {Param::Float(f) => f, _ => panic!("PackedInstruction with definition has no global phase set as floating point number")}; | |
| } | |
| total_phase += match definition.global_phase() { | |
| Param::Float(f) => f, | |
| _ => return Err(QiskitError::new_err(format!("PackedInstruction with definition has no global phase set as floating point number"))) | |
| }; | |
| } |
Although realistically if you make my above suggestion about how to get the definition directly from StandardGate I think you can change this to just use unreachable!() since the generator code for definitions of standard gates never has a parameterized phase, it's always a float 0.0. Something like:
| total_phase += match definition.global_phase() {Param::Float(f) => f, _ => panic!("PackedInstruction with definition has no global phase set as floating point number")}; | |
| } | |
| let Param::Float(new_phase) = definition.global_phase() else { unreachable!() }; | |
| total_phase += new_phase |
(although maybe set an error message saying standard gate phase is never parameterized)
would do the trick. But we can't make this change unless we're working with a StandardGate when we call definition().
There was a problem hiding this comment.
Unless I'm missing something: one of Rz or P (whichever isn't exactly equal to u1) can have a parametric phase if the angle is parametric. GlobalPhaseGate definitely can (if the angle is parametric).
There was a problem hiding this comment.
Ah good point, I thought there was one, but I scanned through the match statement yesterday and missed it on rzgate. But regardless this statement still holds since the angle is always a float at this point
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
There was a problem hiding this comment.
Sorry I glossed over the changes to dag_circuit.rs in my last review. We shouldn't change the interface to insert_1q_on_incoming_qubit unless we need to. Since nothing is using it anymore we should revert that piece. Also, I think we shouldn't make op_names public because that opens up the possibility of a transpiler pass corrupting the name counts accidentally by mutating it. We have the count_ops() method already, but we can also add a no-copy method to get an immutable reference to the inner mapping. These are so minor I'll push up a quick commit to fix both right now. Otherwise I think this is ready to merge.
Previously this PR was making the op_names field of the DAGCircuit struct public so it was accessible to the new transpiler pass code. However, this opened up the possibility of mutating the field by mistake, instead this makes the field private again and adds a no-copy method to get an immutable reference to the field. Additionally, there were some interface changes made to one method in DAGCircuit that were not needed anymore, which this reverts to minimize the diff.
Summary
fixes #12270
Details and comments
This replaces the Python implementation of
CommutativeCancellationwith a Rust implementation:PassManagerto run its prerequisiteCommutationAnalysis, the test case has beentest_callback_with_pass_requiresfixed. We might be able to remove that test case as the check is already covered inqiskit/test/python/transpiler/test_pass_scheduler.py
Line 111 in 52733ae
qiskit/qiskit/transpiler/passes/optimization/commutative_cancellation.py
Line 195 in f4ca088
Speedups (this branch vs Qiskit 1.2)
)
Top to bottom Clifford circuits, random circuits, random circuits with rotation gates only
Up to 5.5x speedup for Clifford circuits where the commutation relation of two gates is always determined by a look-up in an a priori constructed commutation library. Up to 7.0 speedup for random circuits as generated by
random_circuitand up to 8x speedup when restricting that random circuit to rotational gates only. Note this benchmark compares end-to-end performance as given byinstead of only comparing a python implementation of
CommutativeCancellationwith underlying Rust code vs a rust implementation ofCommutativeCancellationwith the same underlying code.