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.

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

Make circuit_to_dag convert blocks in control flow ops to DAGCircuit #10280

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jlapeyre
Copy link
Contributor

@jlapeyre jlapeyre commented Jun 14, 2023

The two main data structures for storing a circuit in Qiskit are QuantumCircuit and DAGCircuit. They support lossless conversion. Control flow operations include blocks of "code" as subcircuits. Currently these are stored only as QuantumCircuits. This necessitates an increasing amount of conversion between the two when manipulating circuits. In particular when doing transpilation.

This PR modifies circuit_to_dag and dag_to_circuit to recursively descend into blocks in control flow operations to convert them to the same type as the top-level structure.

There is a concomitant simplification of the code that (at present partially) supports control flow operations.

Pro arguments

  • We already have a good amount of technical debt due to conversion between the two structures. The rate of piling up this debt is increasing significantly as control-flow support is added to the transpiler. By committing to more debt, it starts to look like the proposed This PR slows the rate of increase of technical debt.

Contra arguments

I don't think these are sufficiently strong to warrant not merging this PR (or something similar).

  • This is too disruptive. It slows down adding new features. We will clean up this technical debt later.
  • Some code supporting control flow ops assumes QuantumCircuit. If you hit these code paths unintended errors will be raised.

Related issues

TODO

This PR will take a bit of work to merge even if the basic scheme is acceptable. Some items.

  • Two existing tests (tox -epy) fail. I guess these are not difficult to fix.
  • I'm pretty sure there remains previously existing code that needs to be modified to be consistent with the new behavior that is not covered in the test suite. So these code paths will be broken even if all tests pass. It needs to be fixed and tests added. (the tests should be added of course in any case.)
  • Tests, doc strings, release notes, etc.

@jakelishman
Copy link
Member

I totally agree that the conversion overheads are a problem in the DAG. We recognised it as a future problem in the rush to get the initial control-flow support into the transpiler and accepted it as debt at the time, but this was one of the solutions that we considered and rejected. Mixing the typing of the internals of ControlFlowOp will I think make it much harder for consumers of it to reason about - already in this PR, the amount of helper-function dispatch that's needed is quite large even within the classes themselves, and in practice, consumers will always need to work with the blocks directly at some points.

I think that this reduces the overhead (very very significantly!!), but it doesn't really address the complexities of handling control-flow from within the transpiler, so it'd need reverting in the near future. I think that the transpiler needs to reason about control flow as an inherent, first-class citizen of its IR. In classical IRs, this often takes the form of splitting program flow into a series of basic blocks, and then building an explicit control-flow graph out of those. That allows peephole-optimisation passes and quantum-only optimisations to be much simpler and efficient than our current structure - at the moment, we have to include utilities to iterate over each instruction separately and recurse through them, and often that needs to be mixed in with the actual pass code, or at least the pass code generally needs updating to explicitly ignore control-flow nodes (to not interfere with the trivial_recurse decorator doing the ad-hoc basic-block iteration).

The downside to the basic-block/CFG structure (and why we didn't immediately try to implement it) is that it's harder to marry the data-flow DAGCircuit format with it, which has significant implications for routing - one of our most important phases. If circuit_to_dag separates things out into basic blocks with data flow between them, it becomes a lot harder for routing passes to reason about the parallelisation possible between different strands (not that we do that well right now, but we do it better than we would). That's particularly important for the case of single 2q gates with a classical condition - if those are converted individually into basic blocks, and basic blocks tank our routing quality, that's a big quality regression we're leaving out. Our current system shouldn't have too much (if any) of a regression in that situation, because it will see the control-flow block as just a 2q gate to route. I don't think that's the direction we want long term, but that's the reason we didn't immediately move to it (and also it's a huge change to an IR that people rely on, so we didn't want to rush into it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants