-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Support control flow in ConsolidateBlocks #10355
Merged
jakelishman
merged 30 commits into
Qiskit:main
from
jlapeyre:controlflow/consolidate_blocks
Jul 20, 2023
Merged
Changes from all commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
413aa02
Support control flow in ConsolidateBlocks
jlapeyre c882c6a
Add release note for support control flow in ConsolidateBlocks
jlapeyre d269db5
Move imports from inside function to top level
jlapeyre c32d69c
Make construction of ConsolidateBlocks for control flow ops more effi…
jlapeyre d4bc8bd
Do IfElseOp test without builder interface
jlapeyre d210512
Linting
jlapeyre eabaee5
Avoid cyclic import
jlapeyre c567d0d
Try to fix cyclic import (in lint tool only)
jlapeyre 7a34941
Reuse top-level consolidation pass and add tests
jlapeyre 3da4990
Remove argument `decomposer` from constructor of ConsolidateBlocks
jlapeyre e4a39fe
Merge branch 'main' into controlflow/consolidate_blocks
jlapeyre 248479d
Remove cruft accidentally left
jlapeyre dc69654
Move function-level import to module level
jlapeyre 60e27b4
Write loop more concisely
jlapeyre 8bea1e7
Update releasenotes/notes/add-control-flow-to-consolidate-blocks-e013…
jlapeyre 87b979e
Use assertion in tests with better diagnostics
jlapeyre bf0a1f4
Remove reference to decomposer from docstring to ConsolidateBlocks
jlapeyre d3d2925
Use more informative tests for ConsolidateBlocks with control flow
jlapeyre a5b57c1
Merge branch 'main' into controlflow/consolidate_blocks
jlapeyre c24f63e
Simplify test in test_consolidate_blocks and factor
jlapeyre 2ae8558
Factor more code in test
jlapeyre c077e49
Use clbit in circuit as test bit when appending IfElse
jlapeyre 7cef53b
In test, use bits in circuit as specifiers when appending gate to tha…
jlapeyre d4460a5
In a test, don't use qubits from unrelated circuit when constructing …
jlapeyre 54bd958
Factor code in test to simplify test
jlapeyre be2a65f
Merge branch 'main' into controlflow/consolidate_blocks
jlapeyre 8eae8c3
Simplify remaining control flow op tests for ConsolidateBlocks
jlapeyre b36a949
Run black
jlapeyre 5dc5f85
Update test/python/transpiler/test_consolidate_blocks.py
jlapeyre 196beb7
Merge branch 'main' into controlflow/consolidate_blocks
jlapeyre File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 changes: 9 additions & 0 deletions
9
releasenotes/notes/add-control-flow-to-consolidate-blocks-e013e28007170377.yaml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
--- | ||
features: | ||
- | | ||
Enabled performing the :class:`.ConsolidateBlocks` pass inside the | ||
blocks of :class:`.ControlFlowOp`. This pass collects several sequences of gates | ||
and replaces each sequence with the equivalent numeric unitary gate. This new feature enables | ||
applying this pass recursively to the blocks in control flow operations. Note that the meaning | ||
of "block" in :class:`.ConsolidateBlocks` is unrelated to that in | ||
:class:`.ControlFlowOp`. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can likely make this
PassManager
just once up in__init__
and retrieve it fromself
each time, even during the recursion.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. The cost should be negligible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After trying, I don't see an easy way to make this work. A
ConsolidateBlocks
instance is constructed before the top-levelRunningPassManager
injects the necessary information. The callPassManager
on my machine takes < 250ns. Constructing it and checking adict
and appending passes takes 13us or 26us.We could pass the required information when constructing
ConsolidateBlocks
. That would allow us to do this more cleanly in the future. Maybe populaterequires
based boolean-valued arguments. We'd support the status quo for a while.In fact, doing this would be less fragile and better signal intent than the status quo. This:
https://github.com/Qiskit/qiskit-terra/blob/fb9d5d8bb41f1e289b5ee895ea087cc92e74a921/qiskit/transpiler/preset_passmanagers/level3.py#L174-L179
is not as transparent as
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the required information to
ConsolidateBlocks
would be my general choice so that the top level of the recursion and the recursive calls can all look more similar. At this stage in the 0.25 cycle, it might be better to leave what you've got and fix it in 0.45 instead so we've got more time.