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

Jump to bottom

Nested flow control for Passmanagers #6830

Closed
mtreinish opened this issue Jul 29, 2021 · 2 comments · Fixed by #6962
Closed

Nested flow control for Passmanagers #6830

mtreinish opened this issue Jul 29, 2021 · 2 comments · Fixed by #6962
Labels
type: enhancement It's working, but needs polishing

Comments

@mtreinish
Copy link
Member

What is the expected enhancement?

Skimming through the pass manager code and reading the documentation (which is quite lacking) for building pass managers I do not see an option to nest flow control. The FlowController class is only attached to a set of passes which makes it impossible to apply an additional flow controller on a subset of those passes. For example the use case I have in mind is to have a condition in a do while loop, for example something like:

while not fixed_point:
    pass_a()
    pass_b()
    check_pass()
    if not check_pass_result:
        pass_d()
    pass_e()
pass_f()

which at least I couldn't find a way to represent in the current pass manager structure. It would be very useful as we make the pass manager more sophisticated to have nested flow control like this. If I just missed it we should improve the documentation around this (which honestly we need regardless).
@mtreinish mtreinish added the type: enhancement It's working, but needs polishing label Jul 29, 2021
@mtreinish mtreinish mentioned this issue Jul 29, 2021
Merged
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Jul 29, 2021
@mtreinish
Add GatesInBasis analysis pass
195e4e1 
This commit adds a new analysis pass, GatesInBasis, which is used to
check if all the gates in the circuit are currently in the basis set or
not. The intent for this pass is to use this to conditionally trigger
the execution of basis translation (normally the BasisTranslator) in the
main optimization loop of the preset pass managers, however this is
blocked on Qiskit#6830. So for now this just adds the pass so we can use it in
the future.
@dhruvbhq
Copy link
Contributor

Do you think this will be a good issue to get familiar with the transpiler code? If yes, I would like to try it.

@mtreinish mtreinish mentioned this issue Aug 22, 2021
Merged
@1ucian0
Copy link
Member

1ucian0 commented Aug 24, 2021

This seems related to #5772 . Let me have a look first, it might be more complicated than expected.

1ucian0 added a commit to 1ucian0/qiskit-terra that referenced this issue Aug 24, 2021
@1ucian0
fix Qiskit#6830
c267ebe
@1ucian0 1ucian0 mentioned this issue Aug 24, 2021
Closed
This was referenced Aug 24, 2021
Merged
Merged
mergify bot pushed a commit that referenced this issue Oct 13, 2021
@mtreinish @jlapeyre @itoko
Add GatesInBasis analysis pass (#6832)
7fb30ba 
* Add GatesInBasis analysis pass

This commit adds a new analysis pass, GatesInBasis, which is used to
check if all the gates in the circuit are currently in the basis set or
not. The intent for this pass is to use this to conditionally trigger
the execution of basis translation (normally the BasisTranslator) in the
main optimization loop of the preset pass managers, however this is
blocked on #6830. So for now this just adds the pass so we can use it in
the future.

* Track instruction op counts in DAGCircuit

This commit reworks the GatesInBasis pass to keep track of the
instruction counts in the DAGCircuit object whenever we add or remove
an op node from the DAG. Then the gates in basis pass can simply lookup
against that counts dictionary whether or not all the gates are in the
basis or not. This way we avoid a full dag traversal to determine the
gates in the dag.

* Docstring fixes

* Fix lint

* Update qiskit/transpiler/passes/utils/gates_basis.py

Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>

* Consolidate DAGCircuit._ops record keeping to helper methods

* Rename to DAGCircuit._ops to DAGCircuit._op_names

* Ensure op_counts is checked in dagcircuit validity checking in tests

* Update qiskit/dagcircuit/dagcircuit.py

Co-authored-by: itoko <15028342+itoko@users.noreply.github.com>

* Use DAGCircuit._op_names for DAGCircuit.count_ops() too

Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
Co-authored-by: itoko <15028342+itoko@users.noreply.github.com>
@mergify mergify bot closed this as completed in #6962 Jan 14, 2022
mergify bot added a commit that referenced this issue Sep 2, 2022
@irajput @mtreinish
@jlapeyre @jakelishman @mergify
Only run translation stage in optimization loop if necessary (#6940)
8a3e760 
* Add GatesInBasis analysis pass

This commit adds a new analysis pass, GatesInBasis, which is used to
check if all the gates in the circuit are currently in the basis set or
not. The intent for this pass is to use this to conditionally trigger
the execution of basis translation (normally the BasisTranslator) in the
main optimization loop of the preset pass managers, however this is
blocked on #6830. So for now this just adds the pass so we can use it in
the future.

* adding to runningpass and level3

* formatted

* rename

* minor format

* edit

* fixed make errors with gates_in_basis

* added reno

* changes

* added suggested changes

* format

* Update qiskit/transpiler/passes/utils/gates_basis.py

Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>

* Update qiskit/transpiler/runningpassmanager.py

Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>

* fixed raise

* added tests

* added tests for nested do_while

* Remove some tests that are added in PR #6962

These tests were in #6962. One has been modified there, and the others
removed. PR #6962 should be merged to main first. Then main will be
merged to this PR #6940

* Remove newline added accidentally

* Add test for running _unroll if not GatesInBasis in level3

This also requires changing GatesInBasis to handle basis_set == None.

* Black reformat

* Fix pylint complaints

* Format file with black

* Build one list rather than concatenating the same two lists repeatedly

* Update optimization level 1 and 2 too

This commit expands the conditional unrolling so that it occurs in every
optimization level that has an optimization loop. Since this PR branch
was originally created we've expanded the use of the unroller stage in
the optimization loop to account for other issues caused by new
developments in the transpiler since this was originally developed. This
commit updates level 1 and level 2 to avoid unecessary basis translation
if the circuit is already in the target basis.

* Update gates in basis to return True if no constraints are present

For the gates in basis pass it previously was raising an error if you
initialized the pass without a target or basis gates constraint set.
This was because the pass didn't have any constraints to enforce so
there wasn't anything to validate. However, with the integration of the
pass into the preset pass managers this behavior wasn't desireable
because people run transpile() without a backend, target, or basis_gates
set. To account for this the pass logic is changed to just return True
if there are no instruction contstraints because all the gates are in
the basis since there is no basis.

* Return early to avoid unnecessary nesting

* Dummy commit to retrigger CI

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type: enhancement It's working, but needs polishing
Projects
None yet
Milestone
No milestone
3 participants
@1ucian0 @mtreinish @dhruvbhq