From fa774b3e3d493d9b04cb98166e40673301abae10 Mon Sep 17 00:00:00 2001 From: Matthew Treinish Date: Sat, 29 Jun 2024 11:00:28 -0400 Subject: [PATCH] Avoid Python op creation in commutative cancellation This commit updates the commutative cancellation and commutation analysis transpiler pass. It builds off of #12692 to adjust access patterns in the python transpiler path to avoid eagerly creating a Python space operation object. The goal of this PR is to mitigate the performance regression on these passes introduced by the extra conversion cost of #12459. --- crates/circuit/src/circuit_instruction.rs | 43 +++++++++++- crates/circuit/src/dag_node.rs | 68 ++++++++++++++++++- crates/circuit/src/operations.rs | 34 +++++++++- qiskit/circuit/commutation_checker.py | 18 +++++ qiskit/quantum_info/operators/operator.py | 1 + .../optimization/commutation_analysis.py | 9 +-- .../optimization/commutative_cancellation.py | 14 ++-- 7 files changed, 169 insertions(+), 18 deletions(-) diff --git a/crates/circuit/src/circuit_instruction.rs b/crates/circuit/src/circuit_instruction.rs index d6516722fbac..84f867498121 100644 --- a/crates/circuit/src/circuit_instruction.rs +++ b/crates/circuit/src/circuit_instruction.rs @@ -15,7 +15,7 @@ use std::cell::RefCell; use numpy::IntoPyArray; use pyo3::basic::CompareOp; -use pyo3::exceptions::{PyDeprecationWarning, PyValueError}; +use pyo3::exceptions::{PyDeprecationWarning, PyTypeError, PyValueError}; use pyo3::prelude::*; use pyo3::types::{IntoPyDict, PyList, PyTuple, PyType}; use pyo3::{intern, IntoPy, PyObject, PyResult}; @@ -432,6 +432,41 @@ impl CircuitInstruction { matrix.map(|mat| mat.into_pyarray_bound(py).into()) } + fn to_matrix(&self, py: Python) -> Option { + self.matrix(py) + } + + fn __array__( + &self, + py: Python, + dtype: Option, + copy: Option, + ) -> PyResult { + if copy == Some(false) { + return Err(PyValueError::new_err( + "A copy is needed to return an array from this object.", + )); + } + let res = + match self.to_matrix(py) { + Some(res) => res, + None => return Err(PyTypeError::new_err( + "ParameterExpression with unbound parameters cannot be represented as an array", + )), + }; + Ok(match dtype { + Some(dtype) => { + let numpy_mod = py.import_bound("numpy")?; + let args = (res,); + let kwargs = [("dtype", dtype)].into_py_dict_bound(py); + numpy_mod + .call_method("asarray", args, Some(&kwargs))? + .into() + } + None => res, + }) + } + #[getter] fn label(&self) -> Option<&str> { self.extra_attrs @@ -460,6 +495,12 @@ impl CircuitInstruction { .and_then(|attrs| attrs.unit.as_deref()) } + pub fn is_parameterized(&self) -> bool { + self.params + .iter() + .any(|x| matches!(x, Param::ParameterExpression(_))) + } + /// Creates a shallow copy with the given fields replaced. /// /// Returns: diff --git a/crates/circuit/src/dag_node.rs b/crates/circuit/src/dag_node.rs index ffd7920a36fd..b7e475f50980 100644 --- a/crates/circuit/src/dag_node.rs +++ b/crates/circuit/src/dag_node.rs @@ -14,10 +14,12 @@ use crate::circuit_instruction::{ convert_py_to_operation_type, operation_type_to_py, CircuitInstruction, ExtraInstructionAttributes, }; +use crate::imports::QUANTUM_CIRCUIT; use crate::operations::Operation; use numpy::IntoPyArray; +use pyo3::exceptions::{PyTypeError, PyValueError}; use pyo3::prelude::*; -use pyo3::types::{PyDict, PyList, PySequence, PyString, PyTuple}; +use pyo3::types::{IntoPyDict, PyDict, PyList, PySequence, PyString, PyTuple}; use pyo3::{intern, IntoPy, PyObject, PyResult}; use smallvec::smallvec; @@ -184,6 +186,16 @@ impl DAGOpNode { Ok(()) } + #[getter] + fn num_qubits(&self) -> u32 { + self.instruction.operation.num_qubits() + } + + #[getter] + fn num_clbits(&self) -> u32 { + self.instruction.operation.num_clbits() + } + #[getter] fn get_qargs(&self, py: Python) -> Py { self.instruction.qubits.clone_ref(py) @@ -215,12 +227,51 @@ impl DAGOpNode { self.instruction.params.to_object(py) } + pub fn is_parameterized(&self) -> bool { + self.instruction.is_parameterized() + } + #[getter] fn matrix(&self, py: Python) -> Option { let matrix = self.instruction.operation.matrix(&self.instruction.params); matrix.map(|mat| mat.into_pyarray_bound(py).into()) } + fn to_matrix(&self, py: Python) -> Option { + self.matrix(py) + } + + fn __array__( + &self, + py: Python, + dtype: Option, + copy: Option, + ) -> PyResult { + if copy == Some(false) { + return Err(PyValueError::new_err( + "A copy is needed to return an array from this object.", + )); + } + let res = + match self.to_matrix(py) { + Some(res) => res, + None => return Err(PyTypeError::new_err( + "ParameterExpression with unbound parameters cannot be represented as an array", + )), + }; + Ok(match dtype { + Some(dtype) => { + let numpy_mod = py.import_bound("numpy")?; + let args = (res,); + let kwargs = [("dtype", dtype)].into_py_dict_bound(py); + numpy_mod + .call_method("asarray", args, Some(&kwargs))? + .into() + } + None => res, + }) + } + #[getter] fn label(&self) -> Option<&str> { self.instruction @@ -281,6 +332,21 @@ impl DAGOpNode { } } + #[getter] + fn definition<'py>(&self, py: Python<'py>) -> PyResult>> { + let definition = self + .instruction + .operation + .definition(&self.instruction.params); + definition + .map(|data| { + QUANTUM_CIRCUIT + .get_bound(py) + .call_method1(intern!(py, "_from_circuit_data"), (data,)) + }) + .transpose() + } + /// Sets the Instruction name corresponding to the op for this node #[setter] fn set_name(&mut self, py: Python, new_name: PyObject) -> PyResult<()> { diff --git a/crates/circuit/src/operations.rs b/crates/circuit/src/operations.rs index 8935e72e0ad5..78f69f50ee49 100644 --- a/crates/circuit/src/operations.rs +++ b/crates/circuit/src/operations.rs @@ -709,8 +709,38 @@ impl Operation for StandardGate { .expect("Unexpected Qiskit python bug"), ) }), - Self::RXGate => todo!("Add when we have R"), - Self::RYGate => todo!("Add when we have R"), + Self::RXGate => Python::with_gil(|py| -> Option { + let theta = ¶ms[0]; + Some( + CircuitData::from_standard_gates( + py, + 1, + [( + Self::RGate, + smallvec![theta.clone(), FLOAT_ZERO], + smallvec![Qubit(0)], + )], + FLOAT_ZERO, + ) + .expect("Unexpected Qiskit python bug"), + ) + }), + Self::RYGate => Python::with_gil(|py| -> Option { + let theta = ¶ms[0]; + Some( + CircuitData::from_standard_gates( + py, + 1, + [( + Self::RGate, + smallvec![theta.clone(), Param::Float(PI / 2.0)], + smallvec![Qubit(0)], + )], + FLOAT_ZERO, + ) + .expect("Unexpected Qiskit python bug"), + ) + }), Self::RZGate => Python::with_gil(|py| -> Option { let theta = ¶ms[0]; Some( diff --git a/qiskit/circuit/commutation_checker.py b/qiskit/circuit/commutation_checker.py index e758674829f8..209890f0c358 100644 --- a/qiskit/circuit/commutation_checker.py +++ b/qiskit/circuit/commutation_checker.py @@ -21,6 +21,7 @@ from qiskit.circuit.operation import Operation from qiskit.circuit.controlflow import CONTROL_FLOW_OP_NAMES from qiskit.quantum_info.operators import Operator +from qiskit._accelerate.circuit import StandardGate _skipped_op_names = {"measure", "reset", "delay", "initialize"} _no_cache_op_names = {"annotated"} @@ -57,6 +58,23 @@ def __init__(self, standard_gate_commutations: dict = None, cache_max_entries: i self._cache_miss = 0 self._cache_hit = 0 + def commute_nodes( + self, + op1, + op2, + max_num_qubits: int = 3, + ) -> bool: + """Checks if two DAGOpNodes commute.""" + qargs1 = op1.qargs + cargs1 = op2.cargs + if not isinstance(op1._raw_op, StandardGate): + op1 = op1.op + qargs2 = op2.qargs + cargs2 = op2.cargs + if not isinstance(op2._raw_op, StandardGate): + op2 = op2.op + return self.commute(op1, qargs1, cargs1, op2, qargs2, cargs2, max_num_qubits) + def commute( self, op1: Operation, diff --git a/qiskit/quantum_info/operators/operator.py b/qiskit/quantum_info/operators/operator.py index a4e93f364809..c9d8740fea7c 100644 --- a/qiskit/quantum_info/operators/operator.py +++ b/qiskit/quantum_info/operators/operator.py @@ -112,6 +112,7 @@ def __init__( # a complex numpy matrix. self._data = np.asarray(data.to_matrix(), dtype=complex) else: + print(data) raise QiskitError("Invalid input data format for Operator") super().__init__( diff --git a/qiskit/transpiler/passes/optimization/commutation_analysis.py b/qiskit/transpiler/passes/optimization/commutation_analysis.py index eddb659f0a25..61c77de552b9 100644 --- a/qiskit/transpiler/passes/optimization/commutation_analysis.py +++ b/qiskit/transpiler/passes/optimization/commutation_analysis.py @@ -72,14 +72,7 @@ def run(self, dag): does_commute = ( isinstance(current_gate, DAGOpNode) and isinstance(prev_gate, DAGOpNode) - and self.comm_checker.commute( - current_gate.op, - current_gate.qargs, - current_gate.cargs, - prev_gate.op, - prev_gate.qargs, - prev_gate.cargs, - ) + and self.comm_checker.commute_nodes(current_gate, prev_gate) ) if not does_commute: break diff --git a/qiskit/transpiler/passes/optimization/commutative_cancellation.py b/qiskit/transpiler/passes/optimization/commutative_cancellation.py index 4c6c487a0ea3..5c0b7317aabd 100644 --- a/qiskit/transpiler/passes/optimization/commutative_cancellation.py +++ b/qiskit/transpiler/passes/optimization/commutative_cancellation.py @@ -24,7 +24,7 @@ from qiskit.circuit.library.standard_gates.rx import RXGate from qiskit.circuit.library.standard_gates.p import PhaseGate from qiskit.circuit.library.standard_gates.rz import RZGate -from qiskit.circuit import ControlFlowOp +from qiskit.circuit.controlflow import CONTROL_FLOW_OP_NAMES _CUTOFF_PRECISION = 1e-5 @@ -138,14 +138,14 @@ def run(self, dag): total_phase = 0.0 for current_node in run: if ( - getattr(current_node.op, "condition", None) is not None + current_node.condition is not None or len(current_node.qargs) != 1 or current_node.qargs[0] != run_qarg ): raise RuntimeError("internal error") if current_node.name in ["p", "u1", "rz", "rx"]: - current_angle = float(current_node.op.params[0]) + current_angle = float(current_node.params[0]) elif current_node.name in ["z", "x"]: current_angle = np.pi elif current_node.name == "t": @@ -159,8 +159,8 @@ def run(self, dag): # Compose gates total_angle = current_angle + total_angle - if current_node.op.definition: - total_phase += current_node.op.definition.global_phase + if current_node.definition: + total_phase += current_node.definition.global_phase # Replace the data of the first node in the run if cancel_set_key[0] == "z_rotation": @@ -200,7 +200,9 @@ def _handle_control_flow_ops(self, dag): """ pass_manager = PassManager([CommutationAnalysis(), self]) - for node in dag.op_nodes(ControlFlowOp): + for node in dag.op_nodes(): + if node.name not in CONTROL_FLOW_OP_NAMES: + continue mapped_blocks = [] for block in node.op.blocks: new_circ = pass_manager.run(block)