Add controlflow handling to stochastic swap pass

[ewinston]

Summary

Adds controlflow handling to StochasticSwap routing pass.

Depends on #8793

Details and comments

[qiskit-bot]

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 3200296423

• 181 of 195 (92.82%) changed or added relevant lines in 5 files are covered.
• 16 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.03%) to 84.684%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/utils/check_map.py	18	19	94.74%
qiskit/transpiler/passes/routing/stochastic_swap.py	125	129	96.9%
qiskit/transpiler/passes/routing/utils.py	19	23	82.61%
qiskit/dagcircuit/dagcircuit.py	18	23	78.26%

Files with Coverage Reduction	New Missed Lines	%
src/optimize_1q_gates.rs	1	95.16%
src/results/marginalization.rs	1	72.99%
src/sampled_exp_val.rs	14	63.93%

Totals
Change from base Build 3199713912:	0.03%
Covered Lines:	61798
Relevant Lines:	72975

---

💛 - Coveralls

[jakelishman]

I've left a few comments throughout. I think the general strategy of treating the interior of each block separately, and inverting the swap map back to the starting state at the end is fine for now, and in the future we can consider an approach where we choose the "best" output permutation in order to reduce unnecessary swaps.

I think we need much more stringent tests of the actual layouts in the tests - using Aer to test that the simulation works isn't really sufficient to test that the layout is both valid, and has the properties we expect.

We agreed in the dynamic circuits meeting that since break/continue handling is difficult (especially in the absence of some sort of true CFG), so let's just error out on them for now, and remove all handling of them.

(A couple of my review comments are probably a little out-of-date since Kevin's comments on the basic swap routing too.

[jakelishman]

I wrote quite a bit of text here, but then I realised that they're all symptoms of the same underlying thing: that the routing pass assumes all control-flow operations are full width of the circuit. I had thought we'd decided we didn't want to go that route? Certainly for our demo purposes, we don't want to go there - we want to be able to convert c_if to IfElseOp for single instructions (only 1q or 2q blocks), and have those scheduled at the same time. This form wouldn't permit that.

To have a working demo, we really need to have that working at not full width. I'm sorry I didn't see this sooner - I had thought it was already working with them.

Below is mostly what I wrote before I noticed this.

---

I had the same issues with this PR as the CheckMap one when using control-flow bodies that don't have the exact same bits as the outer circuit. The following bits are a few cases where that came up.

I found that this threw an error when I used a control-flow builder and tried to route the circuit, such as:

from qiskit import QuantumCircuit, QuantumRegister
from qiskit.transpiler import CouplingMap
from qiskit.transpiler.passes import StochasticSwap

cm = CouplingMap.from_line(2)
qr = QuantumRegister(2, "q")
qc = QuantumCircuit(qr)
with qc.for_loop((0,)):
    qc.cx(0, 1)

StochasticSwap(cm)(qc)

The same happened if I built a loop body as

loop_body = QuantumCircuit(2)
loop_body.cx(0, 1)
qc.for_loop((0,), None, loop_body, [0, 1], [])

It's possible that this isn't something we really need to care about because layout passes should always run before it, though if there's an easy way to have it work, I wouldn't be opposed to it. I don't know if it's the same issue, but I also tried this with a full layout pass, and got the same error message:

from qiskit import QuantumCircuit, QuantumRegister
from qiskit.transpiler import CouplingMap, PassManager
from qiskit.transpiler.passes import (
    StochasticSwap,
    TrivialLayout,
    FullAncillaAllocation,
    EnlargeWithAncilla,
    ApplyLayout,
)

cm = CouplingMap.from_line(2)

qr = QuantumRegister(2, "q")
qc = QuantumCircuit(qr)
with qc.for_loop((0,)):
    qc.cx(0, 1)

pm = PassManager(
    [
        TrivialLayout(cm),
        FullAncillaAllocation(cm),
        EnlargeWithAncilla(),
        ApplyLayout(),
        StochasticSwap(cm),
    ]
)
pm.run(qc)

That might suggest we need some further handling in ApplyLayout, perhaps?

Forcing through these two by using a QuantumRegister for the loop body with exactly the right bits and the name q throws a different error, which I think is related:

from qiskit import QuantumCircuit, QuantumRegister
from qiskit.transpiler import CouplingMap, PassManager
from qiskit.transpiler.passes import (
    StochasticSwap,
    TrivialLayout,
    FullAncillaAllocation,
    EnlargeWithAncilla,
    ApplyLayout,
)

cm = CouplingMap.from_line(5)

qc = QuantumCircuit(5)
qc.cx(0, 2)  # Not physical

for_loop = QuantumCircuit(QuantumRegister(name="q", bits=qc.qubits[3:]))
for_loop.cx(0, 1)
qc.for_loop((0,), None, for_loop, [3, 4], [])  # Physical

pm = PassManager(
    [
        TrivialLayout(cm),
        FullAncillaAllocation(cm),
        EnlargeWithAncilla(),
        ApplyLayout(),
        StochasticSwap(cm),
    ]
)
pm.run(qc)

This gives a key lookup error when using the layout in the internal routing of the control-flow operation.

[ewinston]

A couple of issues I noticed while allowing control flow instructions to be less than full width:

1. the usage of for node in dag.two_qubit_gates may return CF instructions since these should get through the unrolling which could cause exceptions
2. sometimes it seemed possible for a CF instruction's num_clbits to get out of sync with the DAGOpNode.cargs using public methods. I can't remember how it occurred but if I do I'll create an issue.

[jakelishman]

For 1: when this method has been used in other transpiler passes, I've put in an explicit if isinstance(node.op, ControlFlowOp): continue at the start of the loop to avoid issues.

For 2: that seems a bit worrying. At a guess, perhaps it's if a clbit that's in the condition of the argument is also in the body of the blocks becomes idle in the blocks? For IfElseOp, the two blocks also need to have the wires be idle in both blocks to be removed again, which seems like a vector for issues.

[ewinston]

For 1: when this method has been used in other transpiler passes, I've put in an explicit if isinstance(node.op, ControlFlowOp): continue at the start of the loop to avoid issues.
We could also set a flag on the DAGCircuit method which by default doesn't include two qubit CF instructions.

For 2: that seems a bit worrying. At a guess, perhaps it's if a clbit that's in the condition of the argument is also in the body of the blocks becomes idle in the blocks? For IfElseOp, the two blocks also need to have the wires be idle in both blocks to be removed again, which seems like a vector for issues.

That's a possibility. I usually didn't intend to remove idle classical bits but it's possible they slipped through somehere.

The pass only considers wires as idle as the intersection of idle wires of each block.

[jakelishman]

This form of removing idle wires really does look a lot better, thanks. My biggest user-facing concern is about the disappearing operations that come from the (imo incorrect) layer-based determination on whether something is "control-flow" or not.

Internally, though, I do think we could improve the code clarity throughout - the abbreviated variable names are quite hard to read in several places, and the utility functions are very dense with little explanation as to what's happening. I think with neater API boundaries, and perhaps a bit more unification of the two utility routing functions (and maybe one or two well done helper functions), it could really help. I've left some comments throughout with some suggestions.

[jakelishman]

I don't think this is necessary any more? (And if it is, I'm really not keen because it feels very magic, and the way it's used isn't thread-safe.)

[jakelishman]

(Also, it perhaps wouldn't be a bad idea to rebase this PR to get rid of the accidentally re-applied commits from main - it's not really the end of the world, but if we merge as-is, about 11 people will falsely get claimed as co-authors.)

[jakelishman]

Thanks for the latest changes - it's getting easier to read, and the layering bug I had seen does appear to have gone. In the interests of timings now, since we're quite far beyond the deadline for rc1, I'm actioning the things I've suggested in this review myself, and Matthew's also going on bug patrol to try and find failing cases.

Right now, it's probably best if you don't make changes here - I can either (force) push mine directly onto this PR, or I can open a new PR, but it'll be easier to keep in sync if only one of us is doing stuff.

I tried to run this (after merging in main) with:

import qiskit
from qiskit.transpiler import CouplingMap

cm = CouplingMap.from_line(6)

qc = qiskit.QuantumCircuit(6, 1)
qc.h(0)
with qc.for_loop((1,)):
    qc.cx(0, 1)
    qc.cx(0, 2)
    qc.cx(0, 3)
qc.measure(0, 0)
with qc.if_test((0, True)) as else_:
    qc.cx(1, 4)
    qc.cx(1, 5)
with else_:
    qc.cx(4, 5)
qc.cx(2, 3)

qiskit.transpile(
    qc,
    coupling_map=cm,
    routing_method="stochastic",
    basis_gates=["rz", "sx", "cx", "for_loop", "if_else", "while_loop"],
)

and go a later KeyError in the BasisTranslator:

[ ... snip transpile machinery ... ]

~/code/qiskit/terra/qiskit/transpiler/passes/basis/basis_translator.py in run(self, dag)
    240             return dag_updated
    241
--> 242         apply_translation(dag)
    243         replace_end_time = time.time()
    244         logger.info(

~/code/qiskit/terra/qiskit/transpiler/passes/basis/basis_translator.py in apply_translation(dag)
    215                         for block in node.op.blocks:
    216                             dag_block = circuit_to_dag(block)
--> 217                             dag_updated = apply_translation(dag_block)
    218                             if dag_updated:
    219                                 flow_circ_block = dag_to_circuit(dag_block)

~/code/qiskit/terra/qiskit/transpiler/passes/basis/basis_translator.py in apply_translation(dag)
    208             dag_updated = False
    209             for node in dag.op_nodes():
--> 210                 node_qargs = tuple(qarg_indices[bit] for bit in node.qargs)
    211                 qubit_set = frozenset(node_qargs)
    212                 if node.name in target_basis:

~/code/qiskit/terra/qiskit/transpiler/passes/basis/basis_translator.py in <genexpr>(.0)
    208             dag_updated = False
    209             for node in dag.op_nodes():
--> 210                 node_qargs = tuple(qarg_indices[bit] for bit in node.qargs)
    211                 qubit_set = frozenset(node_qargs)
    212                 if node.name in target_basis:

KeyError: Qubit(QuantumRegister(4, 'q'), 0)

It's not clear to me whether this pass produces an invalid circuit somewhere, or if it's a bug in the BasisTranslator, but it's something to find.

[jakelishman]

From my perspective we're at the point where we should merge this now, and get 0.22rc1 out the door. I've done quite a bit of coding on this PR, though, so it'd be good to get a second set of eyes on it to check for any issues I've become blind to.

Thanks Erick for all the work and all the changes!

[mtreinish]

I did a quick pass through the code a couple of small nits inline but nothing worth blocking over (one maybe something we want to explore in 0.23.0). Thanks for all the hard work @ewinston @jakelishman

[mtreinish]

What about cz? But I feel like this should be an operation property that the qarg order doesn't matter because it comes up in other places too like GateDirection (right now we hard code cz, but swap will have the same problem)

[jakelishman]

cz used to be in this list as well in an earlier iteration of this PR - I don't know why it got dropped. Was there a reason, @ewinston?

[mtreinish]

I guess there's not constant we could use for signed 32bit int max. Maybe a comment to explain it would be nice, but it was obvious to me at least.

[jakelishman]

oh yeah, I didn't really think. There's np.iinfo(np.int64).max, I suppose?