Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support control flow in ConsolidateBlocks #10355

Merged
merged 30 commits into from
Jul 20, 2023
Merged

Support control flow in ConsolidateBlocks #10355

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
413aa02
Support control flow in ConsolidateBlocks
jlapeyre Jun 28, 2023
c882c6a
Add release note for support control flow in ConsolidateBlocks
jlapeyre Jun 28, 2023
d269db5
Move imports from inside function to top level
jlapeyre Jun 29, 2023
c32d69c
Make construction of ConsolidateBlocks for control flow ops more effi…
jlapeyre Jun 29, 2023
d4bc8bd
Do IfElseOp test without builder interface
jlapeyre Jul 10, 2023
d210512
Linting
jlapeyre Jul 10, 2023
eabaee5
Avoid cyclic import
jlapeyre Jul 10, 2023
c567d0d
Try to fix cyclic import (in lint tool only)
jlapeyre Jul 10, 2023
7a34941
Reuse top-level consolidation pass and add tests
jlapeyre Jul 11, 2023
3da4990
Remove argument `decomposer` from constructor of ConsolidateBlocks
jlapeyre Jul 11, 2023
e4a39fe
Merge branch 'main' into controlflow/consolidate_blocks
jlapeyre Jul 11, 2023
248479d
Remove cruft accidentally left
jlapeyre Jul 11, 2023
dc69654
Move function-level import to module level
jlapeyre Jul 11, 2023
60e27b4
Write loop more concisely
jlapeyre Jul 12, 2023
8bea1e7
Update releasenotes/notes/add-control-flow-to-consolidate-blocks-e013…
jlapeyre Jul 12, 2023
87b979e
Use assertion in tests with better diagnostics
jlapeyre Jul 12, 2023
bf0a1f4
Remove reference to decomposer from docstring to ConsolidateBlocks
jlapeyre Jul 12, 2023
d3d2925
Use more informative tests for ConsolidateBlocks with control flow
jlapeyre Jul 19, 2023
a5b57c1
Merge branch 'main' into controlflow/consolidate_blocks
jlapeyre Jul 19, 2023
c24f63e
Simplify test in test_consolidate_blocks and factor
jlapeyre Jul 19, 2023
2ae8558
Factor more code in test
jlapeyre Jul 19, 2023
c077e49
Use clbit in circuit as test bit when appending IfElse
jlapeyre Jul 19, 2023
7cef53b
In test, use bits in circuit as specifiers when appending gate to tha…
jlapeyre Jul 19, 2023
d4460a5
In a test, don't use qubits from unrelated circuit when constructing …
jlapeyre Jul 19, 2023
54bd958
Factor code in test to simplify test
jlapeyre Jul 19, 2023
be2a65f
Merge branch 'main' into controlflow/consolidate_blocks
jlapeyre Jul 19, 2023
8eae8c3
Simplify remaining control flow op tests for ConsolidateBlocks
jlapeyre Jul 19, 2023
b36a949
Run black
jlapeyre Jul 20, 2023
5dc5f85
Update test/python/transpiler/test_consolidate_blocks.py
jlapeyre Jul 20, 2023
196beb7
Merge branch 'main' into controlflow/consolidate_blocks
jlapeyre Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions qiskit/transpiler/passes/optimization/consolidate_blocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@
from qiskit.extensions import UnitaryGate
from qiskit.circuit.library.standard_gates import CXGate
from qiskit.transpiler.basepasses import TransformationPass
from qiskit.circuit.controlflow import ControlFlowOp
from qiskit.transpiler.passes.synthesis import unitary_synthesis
from .collect_1q_runs import Collect1qRuns
from .collect_2q_blocks import Collect2qBlocks


class ConsolidateBlocks(TransformationPass):
Expand All @@ -49,12 +52,16 @@ def __init__(
):
"""ConsolidateBlocks initializer.

The decomposer used is kak_basis_gate if it has non-None value. Otherwise
the decomposer is basis_gates if it has non-None value. If both are None,
then a default decomposer is used.

Args:
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
kak_basis_gate (Gate): Basis gate for KAK decomposition.
force_consolidate (bool): Force block consolidation
force_consolidate (bool): Force block consolidation.
basis_gates (List(str)): Basis gates from which to choose a KAK gate.
approximation_degree (float): a float between [0.0, 1.0]. Lower approximates more.
target (Target): The target object for the compilation target backend
target (Target): The target object for the compilation target backend.
"""
super().__init__()
self.basis_gates = None
Expand All @@ -71,6 +78,7 @@ def __init__(
)
else:
self.decomposer = TwoQubitBasisDecomposer(CXGate())
self._approximation_degree = approximation_degree
jakelishman marked this conversation as resolved.
Show resolved Hide resolved

def run(self, dag):
"""Run the ConsolidateBlocks pass on `dag`.
Expand Down Expand Up @@ -159,11 +167,37 @@ def run(self, dag):
dag.remove_op_node(node)
else:
dag.replace_block_with_op(run, unitary, {qubit: 0}, cycle_check=False)

dag = self._handle_control_flow_ops(dag)

# Clear collected blocks and runs as they are no longer valid after consolidation
if "run_list" in self.property_set:
del self.property_set["run_list"]
if "block_list" in self.property_set:
del self.property_set["block_list"]

return dag

def _handle_control_flow_ops(self, dag):
"""
This is similar to transpiler/passes/utils/control_flow.py except that the
collect blocks is redone for the control flow blocks.
"""
from qiskit.transpiler import PassManager
jakelishman marked this conversation as resolved.
Show resolved Hide resolved

pass_manager = PassManager()
if "run_list" in self.property_set:
pass_manager.append(Collect1qRuns())
if "block_list" in self.property_set:
pass_manager.append(Collect2qBlocks())
Comment on lines +187 to +191
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can likely make this PassManager just once up in __init__ and retrieve it from self each time, even during the recursion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. The cost should be negligible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After trying, I don't see an easy way to make this work. A ConsolidateBlocks instance is constructed before the top-level RunningPassManager injects the necessary information. The call PassManager on my machine takes < 250ns. Constructing it and checking a dict and appending passes takes 13us or 26us.

We could pass the required information when constructing ConsolidateBlocks. That would allow us to do this more cleanly in the future. Maybe populate requires based boolean-valued arguments. We'd support the status quo for a while.

In fact, doing this would be less fragile and better signal intent than the status quo. This:

https://github.com/Qiskit/qiskit-terra/blob/fb9d5d8bb41f1e289b5ee895ea087cc92e74a921/qiskit/transpiler/preset_passmanagers/level3.py#L174-L179

is not as transparent as

        ConsolidateBlocks(
            collect_2q_blocks=True, basis_gates=basis_gates, target=target, approximation_degree=approximation_degree
        ),
        UnitarySynthesis(
            basis_gates,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing the required information to ConsolidateBlocks would be my general choice so that the top level of the recursion and the recursive calls can all look more similar. At this stage in the 0.25 cycle, it might be better to leave what you've got and fix it in 0.45 instead so we've got more time.


pass_manager.append(self)
for node in dag.op_nodes(ControlFlowOp):
mapped_blocks = []
for block in node.op.blocks:
new_circ = pass_manager.run(block)
mapped_blocks.append(new_circ)
node.op = node.op.replace_blocks(mapped_blocks)
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
return dag

def _check_not_in_basis(self, gate_name, qargs, global_index_map):
Expand Down
9 changes: 9 additions & 0 deletions releasenotes/notes/add-control-flow-to-consolidate-blocks-e013e28007170377.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
features:
- |
Enabled performing the :class:`qiskit.transpiler.passes.ConsolidateBlocks` pass inside the
blocks of :class:`qiskit.circuit.ControlFlowOp`. This pass collects several sequences of gates
and replaces each sequence with the equivalent numeric unitary gate. This new feature enables
applying this pass recursively to the blocks in control flow operations. Note that the meaning
of "block" in `:class:`qiskit.transpiler.passes.ConsolidateBlocks` is unrelated to that in
:class:`qiskit.circuit.ControlFlowOp`.
jlapeyre marked this conversation as resolved.
Show resolved Hide resolved
90 changes: 88 additions & 2 deletions test/python/transpiler/test_consolidate_blocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@
"""

import unittest
import copy
import numpy as np

from qiskit.circuit import QuantumCircuit, QuantumRegister
from qiskit.circuit.library import U2Gate, SwapGate, CXGate
from qiskit.circuit import QuantumCircuit, QuantumRegister, IfElseOp
from qiskit.circuit.library import U2Gate, SwapGate, CXGate, CZGate
from qiskit.extensions import UnitaryGate
from qiskit.converters import circuit_to_dag
from qiskit.transpiler.passes import ConsolidateBlocks
Expand Down Expand Up @@ -428,6 +429,91 @@ def test_identity_1q_unitary_is_removed(self):
pm = PassManager([Collect2qBlocks(), Collect1qRuns(), ConsolidateBlocks()])
self.assertEqual(QuantumCircuit(5), pm.run(qc))

def test_descent_into_control_flow(self):
"""Test consolidation in blocks when control flow op is the same as at top level."""
qc = QuantumCircuit(2, 1)
u2gate1 = U2Gate(-1.2, np.pi)
u2gate2 = U2Gate(-3.4, np.pi)
qc.append(u2gate1, [0])
qc.append(u2gate2, [1])
qc.cx(0, 1)
qc.cx(1, 0)

pass_manager = PassManager()
pass_manager.append(Collect2qBlocks())
pass_manager.append(ConsolidateBlocks(force_consolidate=True))
result_top = pass_manager.run(qc)

qc_control_flow = QuantumCircuit(2, 1)
qc_block = copy.deepcopy(qc)

qc_block = QuantumCircuit(qc.qubits, qc.clbits)
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
qc_block.append(u2gate1, [0])
qc_block.append(u2gate2, [1])
qc_block.cx(0, 1)
qc_block.cx(1, 0)

ifop = IfElseOp((qc.clbits[0], False), qc_block, None)
qc_control_flow.append(ifop, qc.qubits, qc.clbits)

pass_manager = PassManager()
pass_manager.append(Collect2qBlocks())
pass_manager.append(ConsolidateBlocks(force_consolidate=True))
result_block = pass_manager.run(qc_control_flow)
gate_top = result_top[0].operation
gate_block = result_block[0].operation.blocks[0][0].operation
self.assertEqual(gate_top, gate_block)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this test is not inherently the builder interface, it's because the test compares the exact matrix representation of an inner-scope gate with an outer-scope gate without resolving the bit binding of the inner scope. That's a vital part of resolving control-flow scopes.

It would be good to have tests that explicitly use the builder interface to ensure that there is no problem with bit binding throughout, because that's how users will build control-flow circuits.

Canonicalizing the control-flow structure after the pass has run (which is the only place the test canonicalisation ought to be done, because the tested functionality should work whether or not the form is canonicalised) would not fix this test failure, because the problem is fundamentally that the test was requiring that A(0, 1) had the same matrix form as A(1, 0). Canonicalisation after the pass has run wouldn't change that the operator had been computed on (1, 0) (and suitably transposed) rather than (0, 1). Instead, the test could check the qubit binding, and swap the 2q operator if necessary:

test_matrix = ...
if <qubit-binding flips order>:
    test_matrix = swap @ test_matrix @ swap
    # or
    test_matrix = test_matrix.reshape(2, 2, 2, 2).transpose(1, 0, 3, 2).reshape(4, 4)

Fwiw, the pass is returning a correct answer in the flipped case from what I saw. It's just the test that's wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the builder interface the only interface? I mean, is constructing the object directly considered an internal detail? Is bit binding documented anywhere?
If there is a simple interface that is part of the API, it makes sense to me to use this when testing if possible. Unless it is much more difficult than the builder interface.... Also I recall that the builder interface was supposed to be more provisional than the lower-level design.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really about the builder interface, it's about bit-binding that can also be done with the low-level interface. It's a really important part of the Terra data model that things that interact with scopes (which includes Instruction.definition fields and matrices) need to know how to bind the CircuitInstruction.qubits to their counterparts in the inner scope. This existed before the control-flow instructions as well, it's just that it most frequently comes up with them.

The builder interface for control-flow isn't any more provisional than any part of Terra.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is "bit binding"? I don't find anything like that phrase in terra. I really need some documentation or reference to code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked a bit. Looks as if upon construction some objects, such as control flow ops, don't do any mapping of qubits, nor do they make any assumptions about mapping. QuantumCircuit.to_gate may do some kind of mapping, but I think not. And ConsolidateBlocks implements mapping qubits between the parent circuit and the blocks at a low level, throughout the code. By low level, I mean qiskit has no tools for mapping qubits between objects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By bit binding I mean the relationship between the ordering of CircuitInstruction.qubits and how the operation is applied. If you access operation.definition, or in any way get a matrix representation or inner circuit from the operation, you must ensure that you interpret the qubit ordering according to CircuitInstruction.qubits. Your test is not doing that - you're taking a 2q matrix without regard to the ordering of the qubits, and consequently it's sometimes got a flipped basis if the block happened to be defined on 1, 0 instead of 0, 1.

This isn't specific to control flow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And Qiskit does this everywhere - this isn't something new, it's been a fundamental part of the data since the very first versions.


def test_not_crossing_between_control_flow_block_and_parent(self):
"""Test that consolidation does not occur across the boundary between control flow
blocks and the parent circuit."""
qc = QuantumCircuit(2, 1)
qc.cx(0, 1)
qc_true = QuantumCircuit(qc.qubits, qc.clbits)
qc_false = QuantumCircuit(qc.qubits, qc.clbits)
qc_true.cx(0, 1)
qc_false.cz(0, 1)
ifop = IfElseOp((qc.clbits[0], True), qc_true, qc_false)
qc.append(ifop, qc.qubits, qc.clbits)

pass_manager = PassManager()
pass_manager.append(Collect2qBlocks())
pass_manager.append(ConsolidateBlocks(force_consolidate=True))
qc_out = pass_manager.run(qc)

self.assertIsInstance(qc_out[0].operation, UnitaryGate)
self.assertTrue(np.alltrue(CXGate().to_matrix() == qc_out[0].operation.to_matrix()))
op_true = qc_out[1].operation.blocks[0][0].operation
op_false = qc_out[1].operation.blocks[1][0].operation
self.assertTrue(np.alltrue(CXGate().to_matrix() == op_true.to_matrix()))
self.assertTrue(np.alltrue(CZGate().to_matrix() == op_false.to_matrix()))
jakelishman marked this conversation as resolved.
Show resolved Hide resolved

def test_not_crossing_between_control_flow_ops(self):
"""Test that consolidation does not occur between control flow ops."""
qc = QuantumCircuit(2, 1)
qc_true = QuantumCircuit(qc.qubits, qc.clbits)
qc_false = QuantumCircuit(qc.qubits, qc.clbits)
qc_true.cx(0, 1)
qc_false.cz(0, 1)
ifop1 = IfElseOp((qc.clbits[0], True), qc_true, qc_false)
qc.append(ifop1, qc.qubits, qc.clbits)
ifop2 = IfElseOp((qc.clbits[0], True), qc_true, qc_false)
qc.append(ifop2, qc.qubits, qc.clbits)

pass_manager = PassManager()
pass_manager.append(Collect2qBlocks())
pass_manager.append(ConsolidateBlocks(force_consolidate=True))
qc_out = pass_manager.run(qc)

op_true1 = qc_out[0].operation.blocks[0][0].operation
op_false1 = qc_out[0].operation.blocks[1][0].operation
op_true2 = qc_out[1].operation.blocks[0][0].operation
op_false2 = qc_out[1].operation.blocks[1][0].operation
self.assertTrue(np.alltrue(CXGate().to_matrix() == op_true1.to_matrix()))
self.assertTrue(np.alltrue(CZGate().to_matrix() == op_false1.to_matrix()))
self.assertTrue(np.alltrue(CXGate().to_matrix() == op_true2.to_matrix()))
self.assertTrue(np.alltrue(CZGate().to_matrix() == op_false2.to_matrix()))

jlapeyre marked this conversation as resolved.
Show resolved Hide resolved

if __name__ == "__main__":
unittest.main()