Fully port Depth and Size passes to Rust
#13040
Conversation
recursive depth and size calls.
kevinhartman
force-pushed
the
opt-dag-depth-size
branch
from
b859626 to
5b41efa
Compare
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 10586731786Warning: 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 |
crates/circuit/src/dag_circuit.rs
Outdated
| let blocks = raw_blocks.downcast::<PyTuple>()?; | ||
| let mut block_weights: Vec<usize> = Vec::with_capacity(blocks.len()); | ||
| for block in blocks.iter() { |
There was a problem hiding this comment.
Do we need the PyTuple downcast here? Should we be able to just call len() and iter() on a Bound<PyAny>. I think the only difference is their fallible in PyAnyMethods but not in PyTupleMethods.
There was a problem hiding this comment.
Good idea. Done in 80ff154.
crates/circuit/src/dag_circuit.rs
Outdated
| @@ -2239,34 +2240,33 @@ def _format(operand): | |||
| Ok(if recurse { | |||
There was a problem hiding this comment.
I wonder if we want an && CONTROL_FLOW_OP_NAMES.iter().any(|x| self.op_names.contains_key(&x.to_string())) on this condition so we don't bother with the for loop if we know there are no control flow instructions. The same question applies to size() as well.
There was a problem hiding this comment.
That's a good idea, and allowed me to make the code a bit cleaner across both methods. I added a helper as well, which is private to DAGCircuit for now.
Done in af244c2.
| return Ok(length); | ||
| } | ||
| if !recurse { | ||
| return Err(DAGCircuitError::new_err(concat!( |
There was a problem hiding this comment.
Oh, I forgot about concat! there are a couple of long error messages in this module we should probably break up using it.
Summary
This has a bit of a click-bait title since they're technically already ported (the calls to
DAGCircuit::depthandDAGCircuit::sizemade by the Python passes are already in Rust). But, this PR avoids anisinstancecall that was being done for every node in the DAG on calls to these methods when therecurseparameter was specified astrue, which should resolve the performance issues we were seeing with these passes.I've also done this for
DAGCircuit::count_ops.Details
Resolves #12272, #12273