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

Full support for control flow in transpile #9417

Closed
8 of 10 tasks
jakelishman opened this issue Jan 23, 2023 · 1 comment · Fixed by #10372
Closed
8 of 10 tasks

Full support for control flow in transpile #9417

jakelishman opened this issue Jan 23, 2023 · 1 comment · Fixed by #10372
Assignees
Labels
mod: transpiler Issues and PRs related to Transpiler priority: high type: epic A theme of work that contain sub-tasks type: feature request New feature or request
Milestone

Comments

@jakelishman
Copy link
Member

jakelishman commented Jan 23, 2023

What should we add?

Over the course of the epic:

we added support for control-flow to the transpiler at optimisation levels 0 and 1. For full support, we need to bring the missing passes up-to-speed as well. This epic tracks the other components to achieve that.

For the purposes of this issue, we particularly need SabreSwap upgrading. For optimisation level 2, we need the optimisation passes CommutationAnalysis and CommutativeCancellation, and optimisation level 3 (and some less common transpiler options) make up the rest. We do not need to add support for control-flow to all of the scheduling passes right now, because Qiskit control-flow is only supported by IBM hardware at the moment, and scheduling this is handled by the IBM provider.

When these are updated, we should also make sure that the corresponding gated checks in the preset pass-manager constructors are corrected.

Sub-epics:

Tracked issues:

Low priority related issues - these should not be prioritised, and are not required for this epic:

@jlapeyre
Copy link
Contributor

jlapeyre commented Jun 13, 2023

EDIT: #10355 uses a completely different approach. So the comment here is irrelevant to this epic.

The current implementation of ConsolidateBlocks requires that information regarding nodes on the DAG persists across passes, using the usual properties mechanism. But the blocks of ops inside control flow ops (need a good name for that!) are QuantumCircuits. If you convert them to DAGCircuits each time you need to analyze or modify them in a pass, the relevant node ids will not be the same across passes. Trying to refer to the node ids from a previous pass would result in an error. There's also the issue of performance degradation from repeated conversion between circuits and dags. But that is a secondary concern.

We might try something like this:

  1. Allow the bodies of control flow ops to be DAGCircuits, rather than restricting them to QuantumCircuit as is currently the case. Maybe making IfElseOp a parametric type is reasonable. It seems Python seems to be evolving toward better support for this. But, I imagine there are some downsides. Or more simply, allow the two types in the bodies and do isinstance checks. This will add a bit of complexity and bug-proneness.
  2. Modify circuit_to_dag to recursively convert control flow ops to use DAGCircuit.

Then these lower-level DAGs will have a persistent identity. And we can manipulate them at a lower level. Eg. within DAGCircuit. This should be cleaner than doing conversion and recursion into bodies repeatedly at higher levels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod: transpiler Issues and PRs related to Transpiler priority: high type: epic A theme of work that contain sub-tasks type: feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants