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

add controlflow handling to Layout2qDistance transpiler pass #8733

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ewinston
Copy link
Contributor

Summary

This pr adds controlflow handling to the Layout2qDistance transpiler pass.

Details and comments

This implementation adds an init parameter weight_loops to indicate whether the number of blocks/loops should contribute to the overall 2q layout score. If True for while loops, NaN is returned.

@ewinston ewinston requested a review from a team as a code owner September 12, 2022 21:38
@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

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3040745492

  • 26 of 29 (89.66%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 84.309%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passes/layout/layout_2q_distance.py 26 29 89.66%
Totals Coverage Status
Change from base Build 3039251673: 0.005%
Covered Lines: 57854
Relevant Lines: 68621

💛 - Coveralls

@ewinston ewinston added this to the 0.22 milestone Sep 15, 2022
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

When I needed to make depth and size "estimates" for the optimisation-loop fixed-point-condition analysis passes (#8763), I treated while loops as having a single loop - for the purposes of what's being tested, it's sufficient.

This pass is a bit of a weird one. It's only used to evaluate if a layout is perfect or not - we don't need any sort of realistic estimate at all, just a "zero or not". The documentation of the class just says that the output number is the "score", rather than representing something physical, so it doesn't matter if it's not perfect. It still does feel a bit weird that it may return nan in this form; I feel like if we do commit to returning a number, we always ought to return a number.

After writing this, I'm really wondering if it would be more appropriate for us simply to swap the O1 "perfect layout" condition over to use CheckMap instead. That's more logically the operation we're trying to perform here, and it's slightly more sound anyway - this pass can't really be made Target-aware in the same way (because the concept of the distance matrix isn't clear once there's 3q+ gates or cx isn't the only 2q operation), so currently O1 is technically inconsistent with itself pre- and post-routing.

Perhaps we could carve the CheckMap changes out of #8418, swap the layout check to that, and deprecate this PR?

cc: @mtreinish, @kdk for your opinions, since I don't know the history behind the choice in the presets. Looks like it was added in #3657, and the initial Layout2qDistance was in #3321, and neither has substantially changed since those PRs.

@mtreinish
Copy link
Member

I tend to agree, the 2q distance does seem like overkill for determining that a trivial layout is perfect. CheckMap is more than enough to determine if the trivial layout is enough and should be faster. I'm not sure the history on the use of Layout2qDistance for this either, I never really questioned its usage when I touched the layout path code in the past, but from my perspective making that switch in level1 makes the most sense to me. (it's not used in level 2 or 3 because vf2layout does the checking for us)

@ewinston
Copy link
Contributor Author

On its own this pass proposes to score the quality of a layout in a reasonable way. Whether or not other passes use this analysis is a somewhat separate issue, although it doesn't seem like anything in terra currently use this information in a way that can't be replaced by CheckMap as pointed out.

In the case of level1 it seems fine to replace this pass with CheckMap, put this PR on hold, and perhaps deprecate the pass.

@mtreinish mtreinish removed this from the 0.22 milestone Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants