Fix SabreSwap with classically conditioned gates

[jakelishman]

Summary

The calculation of the expected number of predecessors for a gate to
be considered "resolved" in SabreSwap was not accounting for wires
stemming from classical conditions. The tracking of the actual number
of predecessor requirements satisfied did correctly account for this, so
in certain circumstances the actual count could jump from "too low" to
"too high" without passing through "just right", and the gate would
never get added to the circuit.

Details and comments

Fix #8040.

[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 2315102409

• 15 of 15 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.005%) to 84.38%

---

Totals
Change from base Build 2309830910:	0.005%
Covered Lines:	54505
Relevant Lines:	64595

---

💛 - Coveralls

[jakelishman]

Oh wait, I didn't test my method with more than one bit in the condition register - it might not be fully correct yet. I'll look again tomorrow.

[jakelishman]

I've fixed up the pass so it now uses the same SabreSwap._successors function to calculate both the required number of predecessors and the actual number of predecessors. Now, instead of counting up to the required number, we count down to zero. This means that a change to SabreSwap._successors is sufficient to change all the assumptions about the input wires, and those wires are read from the DAG directly, rather than being inferred (incorrectly) from a DAGOpNode.

[mtreinish]

Overall LGTM, thanks for fixing this. Just a typo fix inline and a small question (not critical).

[mtreinish]

I'm wondering if doing something like:

Suggested change

	for node in dag.op_nodes():
	for successor in self._successors(node, dag):
	out[successor] += 1
	for node in dag.op_nodes():
	out_successor[node] += dag._multi_graph.in_degree(node)

would work and be faster. Not that I expect this to be a bottleneck. It just took me a sec to grok this.
I guess there might be an issue because of the input nodes as we'd be including the first layer nodes and they'd have a required predecessor count for the inputs. That's not too different than what this outputs now because we'll end up with output nodes in the out dict but that should be ok because we'll never do a lookup on an output wire.

[jakelishman]

I think this will count DAGInNodes too? We ought to discount those to do things properly, but we could just loop through the original front_layer and automatically set those to zero.

[jakelishman]

Oh, I clearly didn't read your comment properly sorry!

[mtreinish]

Lets just leave this as is. We can revisit this after #7977 and see if we're actually seeing this in profiles or not.

[jakelishman]

Our asv benchmarks claim that this PR introduced a severe performance regression for large circuits. This is a false positive - the benchmarks were actually being completely invalidated by the bug this PR fixes. For example, constructing the exact 14-qubit 1024-depth circuit used in the benchmark suite, before this PR we saw:

In [1]: cmap = ...
   ...: dag_14q_1024 = ...
   ...: swap = SabreSwap(cmap)
   ...: ops_before = dag_to_circuit(dag_14q_1024).count_ops()
   ...: ops_after = dag_to_circuit(swap.run(dag_14q_1024)).count_ops()
   ...:
   ...: # Should be all zeros, except for swap which should be positive.
   ...: {k: ops_after[k] - ops_before[k] for k in ops_before}
Out[1]:
{'cy': -886,
 'cx': -484,
 'ch': -456,
 'rzz': -454,
 'cu3': -459,
 'cz': -436,
 'swap': 474,
 'crz': -399,
 'u1': -260,
 'tdg': -248,
 'sdg': -245,
 'rz': -241,
 'u3': -236,
 'x': -257,
 'u2': -251,
 'z': -232,
 'ry': -228,
 's': -231,
 'rx': -244,
 'y': -249,
 'reset': -232,
 't': -223,
 'id': -230,
 'h': -224,
 'measure': -13}

Ignoring swap gates (hard to count the true difference), the pass was throwing away literally 80% of the instructions in the input. After this patch, the dictionary is correctly:

{'cy': 0,
 'cx': 0,
 'ch': 0,
 'rzz': 0,
 'cu3': 0,
 'cz': 0,
 'swap': 6329,
 'crz': 0,
 'u1': 0,
 'tdg': 0,
 'sdg': 0,
 'rz': 0,
 'u3': 0,
 'x': 0,
 'u2': 0,
 'z': 0,
 'ry': 0,
 's': 0,
 'rx': 0,
 'y': 0,
 'reset': 0,
 't': 0,
 'id': 0,
 'h': 0,
 'measure': 0}

Since we're now (correctly) doing 5x the amount of work, the 400% increase in run-time matches up with what is expected.