Modify preset pass managers for control-flow support #8830

jakelishman · 2022-10-03T12:51:56Z

Summary

This adds control-flow support to the level 0 and level 1 pass managers directly, including neatly erroring out at runtime if the pass manager was constructed with options that are not yet compatible with control flow. In general, we assume that plugins from outside of Terra are safe for control flow - it's highly likely that this is not in fact the case, but it's better to do things this way so an external plugin can support control flow, as opposed to forbidding them from doing so.

Level 2 and level 3 both contain optimisations that are incompatible with control flow right now, and so immediately error if an input circuit has control flow. The failing optimisations are ones that involve a two-pass process of "block collection" followed by "block replacement"; we have no updated any of the "block collection" analysis passes to be safe for control flow, since we would also likely need to communicate the control-flow structure through the property set, which is not a path we're going down just yet.

In order to construct pass managers with nested flow control (e.g. the routing-pass stages contain conditionals, but the entire routing stage needs to be conditioned on the presence of control flow), there is a new PassManager method to_flow_controller. This produces a FlowControllerLinear with the same content as the pass manager, such that it can then itself be conditioned as a complete block, and embedded into other pass managers.

Details and comments

Depends (effectively) on #8418 - the modifications to level 1 assume that StochasticSwap will work with control flow.

I haven't yet written tests for this, as I'm not 100% certain this is the right direction and I don't want to waste time. Most of them likely wouldn't pass without #8418 anyway. I'll write tests once we've got a bit of an agreement in spirit.

I anticipate this PR being the one that will add all the release notes for the new control-flow support through the transpiler - I will also add those before merge.

Fix #8630. (This will be the last PR of that epic.)
Fix #8214.

This adds control-flow support to the level 0 and level 1 pass managers directly, including neatly erroring out at runtime if the pass manager was constructed with options that are not yet compatible with control flow. In general, we assume that plugins from outside of Terra are safe for control flow - it's highly likely that this is not in fact the case, but it's better to do things this way so an external plugin _can_ support control flow, as opposed to forbidding them from doing so. Level 2 and level 3 both contain optimisations that are incompatible with control flow right now, and so immediately error if an input circuit has control flow. The failing optimisations are ones that involve a two-pass process of "block collection" followed by "block replacement"; we have no updated any of the "block collection" analysis passes to be safe for control flow, since we would also likely need to communicate the control-flow structure through the property set, which is not a path we're going down just yet. In order to construct pass managers with nested flow control (e.g. the routing-pass stages contain conditionals, but the entire routing stage needs to be conditioned on the presence of control flow), there is a new `PassManager` method `to_flow_controller`. This produces a `FlowControllerLinear` with the same content as the pass manager, such that it can then itself be conditioned as a complete block, and embedded into other pass managers.

qiskit-bot · 2022-10-03T12:51:59Z

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

mtreinish · 2022-10-03T13:36:24Z

So I haven't looked at this in too much detail but we did add support for nested #6962. I was able to use that to execute a series of passes with a nested controller to enable GatesInBasis to trigger basis translation in the optimization loop without needing PassManger changes. For example: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/transpiler/preset_passmanagers/level2.py#L246-L249 (where unroll is built from a PassManager used for the translation stage here: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/transpiler/preset_passmanagers/level2.py#L240 ). I won't say it's the most ergonomic interface, but in the interest of keeping the API changes minimal at this stage in the release could you use something like that to do what you're doing with to_flow_controller?

jakelishman · 2022-10-03T13:50:59Z

The change I made to PassManager is basically a formalisation of what you've done there - you never construct an intermediate pass manager, but since I have to condition the output of get_pass_manager_stage (which is a PassManager), I don't have the option to just create a list like you've done. However, all the PassManager.to_flow_controller function does is just eagerly construct the pass_sets list into a list of FlowController instances (rather than leaving it in the weird semi-constructed state), which is the same as what you were able to do there.

That's only possible because of #6962 - I had to go through the old git history to find that PR, because the documented types are wrong (and control flow through PassManager is super confusing). What I did doesn't change how the pass manager actually runs or passes itself to RunningPassManager - it's just a conversion method.

The only change is these lines: https://github.com/Qiskit/qiskit-terra/blob/921139eb39c3ffa984c76949fe22a0b1d0afa72f/qiskit/transpiler/passmanager.py#L325-L336

Qiskitgh-6962 added support for nested `FlowController` instances. It did not, however, propagate any `DAGCircuit` output by its internal passes into the transpilation loop, which meant that if a pass returned a new DAG rather than mutating the input, the changes would be thrown away. Similar behaviour was occuring with the pass "normalisation" step; incorrect classes would raise an error, but if the normalisation needed to raise a single pass to a list, the result would not be re-bound into the nested controller.

jakelishman · 2022-10-03T15:07:59Z

The majority of the failing tests here transpired to be because of two reasons:

the is not None test in evaluating whether the trivial layout was perfect (which also decides whether VF2PostLayout should run) was important when no layout stage had run. Fixed by ff05b66.
Nesting FlowControllers to allow nested conditionals in pass manager #6962, which added support for nested FlowControllers didn't propagate the resultant DAGCircuit instances from its passes - they might "accidentally" work if the inner pass mutated its input rather than returning a new DAG, but this broke down for more complex passes. Fixed by a363da7.

coveralls · 2022-10-03T15:39:44Z

Pull Request Test Coverage Report for Build 3204437657

72 of 76 (94.74%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 84.756%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/preset_passmanagers/level0.py	3	4	75.0%
qiskit/transpiler/preset_passmanagers/level1.py	16	17	94.12%
qiskit/transpiler/preset_passmanagers/level2.py	3	4	75.0%
qiskit/transpiler/preset_passmanagers/level3.py	2	3	66.67%

Totals
Change from base Build 3202212994:	0.02%
Covered Lines:	61898
Relevant Lines:	73031

💛 - Coveralls

kdk

Overall approach LGTM and matches what we discussed offline.

qiskit/transpiler/preset_passmanagers/level1.py

mtreinish

Overall I'm happy with this direction it makes sense to me, I left a few inline comments from quick things that caught my eye on this first pass. After #8418 merges and this is rebased with tests verifying transpile() with control I'll circle back and do a more thorough review.

qiskit/transpiler/preset_passmanagers/common.py

mtreinish · 2022-10-03T16:42:46Z

qiskit/transpiler/preset_passmanagers/common.py

+        ("optimization", optimization_method),
+        ("scheduling", scheduling_method),
+    ]:
+        option = stage + "_method"


Couldn't we just use "layout_method" etc for L99-L103? Besides the _CONTROL_FLOW_STATES lookup this is just used for the error message and *_method works fine for the error message too.

I think I did this just because one of the possible error messages is "the entire {stage} stage is not supported", and that's weird with scheduling_method.

mtreinish · 2022-10-03T17:19:57Z

qiskit/transpiler/preset_passmanagers/level2.py

+    init = common.generate_error_on_control_flow(
+        "The optimizations in optimization_level=2 do not yet support control flow."
+    )


We technically could get a level2 pm that given the assumptions made in common.py is runnable with control flow. But it'd basically be layout_method=dense, routing_method=stochastic, optimization=plugin. It seems to be fairly unlikely in practice so this is fine I think.

I was treating us as "supporting" an optimisation level if the defaults will work - for O2, you have to override everything that actually makes it O2 back to what's in O1, so I think it's better to reject optimization_level=2 out-of-hand.

I guess at O1, now that routing and layout are both Sabre by default there's an argument that we're kind of just doing that internally too, I suppose. I was still thinking in terms of the old O1 that had dense/stochastic as its default.

I'm fine with this for now, to a certain degree if people want to get more complicated maybe they should just be using custom pass managers. We just will have to document clearly in the release note that optimization_level 2 and 3 will error regardless of options with a control flow circuit.

jakelishman · 2022-10-04T17:18:23Z

I've added the tests for the new functionality. They won't pass yet until this PR has #8418 as well, but they should pass immediately after that.

mtreinish

Overall this LGTM, as you predicted the only comments I had were on the release notes, just a few small ideas and suggestions inline. But not worth blocking over either, we can always work on these in the 0.22.0 final release prep when we edit the release notes too. So don't feel you have to update the release notes now.

releasenotes/notes/transpiler-control-flow-708896bfdb51961d.yaml

jakelishman · 2022-10-07T11:40:29Z

Merged with main after #8418 merged (and the bug fixes #8843 and #8844), and this is ready for final review and hopefully merge. I had to correct a couple of tests that I'd borked while writing them without an actual functional implementation, but it should be quite clear from the changeset in 66ed532 why it was the tests that were the problem.

mtreinish

LGTM, thanks for the updates to the release notes. Looking through the test changes all of those make sense to me. TBH, in my earlier reviews I didn't look too closely at the test code as we were waiting for #8418 still

jakelishman added on hold Can not fix yet priority: high Changelog: New Feature Include in the "Added" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Oct 3, 2022

jakelishman added this to the 0.22 milestone Oct 3, 2022

jakelishman mentioned this pull request Oct 3, 2022

Track adding control flow passes for optimization level 1 #8630

Closed

26 tasks

jakelishman assigned kdk and mtreinish Oct 3, 2022

Remove unused import

b935ae9

jakelishman added 2 commits October 3, 2022 16:01

Reinstate is not None in trivial_layout conditions

ff05b66

jakelishman added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Oct 3, 2022

Add release notes for transpiler control-flow support

3fc8cc6

jakelishman changed the title ~~Modify present pass managers for control-flow support~~ Modify preset pass managers for control-flow support Oct 3, 2022

kdk reviewed Oct 3, 2022

View reviewed changes

qiskit/transpiler/preset_passmanagers/level1.py Show resolved Hide resolved

mtreinish reviewed Oct 3, 2022

View reviewed changes

jakelishman added 4 commits October 3, 2022 18:27

Update out-of-date layout condition comments

f1a0bba

Make working/not-working split clearer in internal lookup

5e31bc5

Add tests of preset pass managers

097f63b

Merge remote-tracking branch 'ibm/main' into control-flow-preset

6160b16

jakelishman marked this pull request as ready for review October 4, 2022 17:18

jakelishman requested a review from a team as a code owner October 4, 2022 17:18

mtreinish previously approved these changes Oct 5, 2022

View reviewed changes

jakelishman added 3 commits October 6, 2022 20:40

Update release notes

a53a4a3

Merge remote-tracking branch 'ibm/main' into control-flow-preset

b500c57

Fix incorrect tests

66ed532

jakelishman dismissed mtreinish’s stale review via 66ed532 October 7, 2022 11:37

jakelishman removed the on hold Can not fix yet label Oct 7, 2022

jakelishman requested review from mtreinish and kdk October 7, 2022 11:40

mtreinish approved these changes Oct 7, 2022

View reviewed changes

mtreinish added the automerge label Oct 7, 2022

mergify bot merged commit 90b158c into Qiskit:main Oct 7, 2022

jakelishman deleted the control-flow-preset branch October 7, 2022 13:34

This was referenced Jan 23, 2023

Support control flow in SabreSwap #9419

Closed

Restore level 1 layout/routing default to SabreSwap #9422

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify preset pass managers for control-flow support #8830

Modify preset pass managers for control-flow support #8830

jakelishman commented Oct 3, 2022 •

edited

Loading

qiskit-bot commented Oct 3, 2022

mtreinish commented Oct 3, 2022

jakelishman commented Oct 3, 2022 •

edited

Loading

jakelishman commented Oct 3, 2022

coveralls commented Oct 3, 2022 •

edited

Loading

kdk left a comment

mtreinish left a comment

mtreinish Oct 3, 2022

jakelishman Oct 3, 2022

mtreinish Oct 3, 2022

jakelishman Oct 3, 2022

jakelishman Oct 3, 2022

mtreinish Oct 4, 2022

jakelishman commented Oct 4, 2022

mtreinish left a comment

jakelishman commented Oct 7, 2022

mtreinish left a comment

Modify preset pass managers for control-flow support #8830

Modify preset pass managers for control-flow support #8830

Conversation

jakelishman commented Oct 3, 2022 • edited Loading

Summary

Details and comments

qiskit-bot commented Oct 3, 2022

mtreinish commented Oct 3, 2022

jakelishman commented Oct 3, 2022 • edited Loading

jakelishman commented Oct 3, 2022

coveralls commented Oct 3, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3204437657

💛 - Coveralls

kdk left a comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Oct 3, 2022

Choose a reason for hiding this comment

jakelishman Oct 3, 2022

Choose a reason for hiding this comment

mtreinish Oct 3, 2022

Choose a reason for hiding this comment

jakelishman Oct 3, 2022

Choose a reason for hiding this comment

jakelishman Oct 3, 2022

Choose a reason for hiding this comment

mtreinish Oct 4, 2022

Choose a reason for hiding this comment

jakelishman commented Oct 4, 2022

mtreinish left a comment

Choose a reason for hiding this comment

jakelishman commented Oct 7, 2022

mtreinish left a comment

Choose a reason for hiding this comment

jakelishman commented Oct 3, 2022 •

edited

Loading

jakelishman commented Oct 3, 2022 •

edited

Loading

coveralls commented Oct 3, 2022 •

edited

Loading