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

Use Sabre by default for optimization levels 1 and 2 #8552

Merged
merged 12 commits into from
Sep 29, 2022

Conversation

mtreinish
Copy link
Member

Summary

This commit updates the preset pass manager construction to use the
SabreLayout and SabreSwap passes by default for optimization level 1 and
level 2. With the recently merged #7977 the performance of the sabre
swap pass has improved significantly enough to be considered for use by
default with optimization levels 1 and 2. While for small numbers of
target device qubits (< 30) the SabreLayout/SabreSwap pass doesn't quite
match the runtime performance of DenseLayout/StochasticSwap it typically
has better runtime performance for larger target devices. Additionally,
the runtime performance of Sabre should also improve further after #8388
is finished. However, the output quality from the sabre passes is
typically better resulting in fewer swap gates being inserted. With the
combination of better quality and comparable runtime performance it
makes sense to use sabre as the default for optimization levels 1 and 2.
For optimization level 0 stochastic swap is still used there because we
want to continue to leverage TrivialLayout for that level and to get
the full quality advantages SabreSwap and SabreLayout should be used
together.

Details and comments

This commit updates the preset pass manager construction to use the
SabreLayout and SabreSwap passes by default for optimization level 1 and
level 2. With the recently merged Qiskit#7977 the performance of the sabre
swap pass has improved significantly enough to be considered for use by
default with optimization levels 1 and 2. While for small numbers of
target device qubits (< 30) the SabreLayout/SabreSwap pass doesn't quite
match the runtime performance of DenseLayout/StochasticSwap it typically
has better runtime performance for larger target devices. Additionally,
the runtime performance of Sabre should also improve further after Qiskit#8388
is finished. However, the output quality from the sabre passes is
typically better resulting in fewer swap gates being inserted. With the
combination of better quality and comparable runtime performance it
makes sense to use sabre as the default for optimization levels 1 and 2.
For optimization level 0 stochastic swap is still used there because we
want to continue to leverage TrivialLayout for that level and to get
the full quality advantages SabreSwap and SabreLayout should be used
together.
@mtreinish mtreinish added performance Changelog: API Change Include in the "Changed" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Aug 16, 2022
@mtreinish mtreinish added this to the 0.22 milestone Aug 16, 2022
@mtreinish mtreinish requested a review from a team as a code owner August 16, 2022 13:47
@qiskit-bot
Copy link
Collaborator

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
Copy link
Member Author

For some data backing this change up, I ran these benchmarks: https://github.com/Qiskit/red-queen/tree/main/red_queen/games/mapping/benchmarks/misc#readme to compare SabreLayout/SabreSwap with DenseLayout/StochasticSwap which yielded the following plots:

For CX gate count post routing:

mapping_bench_kdk_view

For runtime performance of layout and routing:

mapping_bench_kdk_view_time

@nonhermitian
Copy link
Contributor

This is nice.

In Qiskit#7977 we moved to use compiled objects for part of the SabreSwap
compiler pass. However an unintended side effect of that PR was the use
of Rust objects stored in instance level variables which weren't
pickleable. This breaks multiprocessing at the PassManager level which
expects to be able to pickle and send a SabreSwap object to the
subprocess running on a circuit. This commit fixes this by making the
Rust NeighborTable object pickleable and switching to storing the
heuristic string at the instance level instead of the heuristic enum.
This commit updates a failing layout test which was assuming that level
1 and level 2 where still running DenseLayout. The test has been updated
to reflect the new default of SabreLayout.
@kdk
Copy link
Member

kdk commented Aug 16, 2022

💯 , can you add the scripts to generate these plots as an example somewhere in red-queen?

@coveralls
Copy link

coveralls commented Aug 16, 2022

Pull Request Test Coverage Report for Build 3127193931

  • 41 of 46 (89.13%) changed or added relevant lines in 4 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 84.427%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passes/routing/sabre_swap.py 7 8 87.5%
src/sabre_swap/neighbor_table.rs 30 34 88.24%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/layout/dense_layout.py 2 94.9%
Totals Coverage Status
Change from base Build 3126106140: 0.1%
Covered Lines: 59606
Relevant Lines: 70601

💛 - Coveralls

@gushuli
Copy link
Contributor

gushuli commented Aug 16, 2022

Thank you for your efforts in redeveloping Sabre in Rust! I really appreciate it~

Gushu

@mtreinish
Copy link
Member Author

Since #8388 is ready for review now I reran the benchmark script used above with #8388 applied to get a feel for the relative performance after it merges:

mapping_bench_kdk_view
mapping_bench_kdk_view_time

With #8388 it looks like Sabre is faster most of the time and definitely scales better for larger inputs and target devices.

I also added a graph showing the output circuit depth after running SabreLayout and SabreSwap on the input circuits:

mapping_bench_kdk_view_depth

The interesting thing there is that circuit depth seems to increase slightly when using sabre but the overall swap count decreases.

@kdk
Copy link
Member

kdk commented Aug 30, 2022

For the 0.22 release, there is a goal of having the preset pass managers support control flow, so before we can merge this we should have a plan or PR for integrating control flow support in sabre, similar to #8565 .

@kdk kdk added the on hold Can not fix yet label Aug 30, 2022
@jakelishman
Copy link
Member

@kdk: or we can cheat, and use Sabre by default if there's no control flow, and whatever we've got if there is.

@kdk kdk removed the on hold Can not fix yet label Aug 30, 2022
@kdk
Copy link
Member

kdk commented Aug 30, 2022

Following some discussion with the team, the current plan is to start with Dense+StochasticSwap as default for circuits with control flow (as part of #8630 and #8418 ), and implement control flow handling with SabreSwap/SabreLayout post 0.22 release.

@kdk kdk self-assigned this Sep 20, 2022
@@ -218,7 +218,7 @@ def pass_manager(self, pass_manager_config, optimization_level=None) -> PassMana
)
if optimization_level == 1:
routing_pass = SabreSwap(
coupling_map, heuristic="lookahead", seed=seed_transpiler, trials=5
coupling_map, heuristic="decay", seed=seed_transpiler, trials=5
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this here, because the additional cost for the more thorough heuristic is basically zero over lookahead and in theory should provide better results. This way makes the optimization levels all run the same heuristic and just differ with the number of trials.

@kdk kdk added the automerge label Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog mod: transpiler Issues and PRs related to Transpiler performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants