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 control flow handling to DenseLayout pass #8742

Merged
merged 2 commits into from
Sep 30, 2022

Conversation

ewinston
Copy link
Contributor

@ewinston ewinston commented Sep 14, 2022

Summary

Add control flow handling to dense layout pass.

Details and comments

depends on #8763

todo: may still need to update Target.operation_names_for_qargs when target object is supplied.

@ewinston ewinston requested a review from a team as a code owner September 14, 2022 06:22
@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

coveralls commented Sep 14, 2022

Pull Request Test Coverage Report for Build 3156699536

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.05%) to 84.646%

Files with Coverage Reduction New Missed Lines %
qiskit/pulse/library/waveform.py 3 91.49%
Totals Coverage Status
Change from base Build 3156697731: 0.05%
Covered Lines: 61419
Relevant Lines: 72560

💛 - Coveralls

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

FWIW, I'm not sure how much this is actually needed. The cx count and measurement count are only used as weighting factors when trying to estimate how noisy the qubits in a layout are (in the BackendV1 path only). For control flow circuits trying to adjust that count really doesn't provide much benefit over not doing it because it's just a heuristic to try and avoid super noisy qubits or 2q links being selected in the layout (the weighting factor is just to care more about 2q error vs measurements based on the circuit which is why we excluded it from the v2 path). This error based heuristic is kind of obsolete now anyway with VF2PostLayout which does a much better job at noise aware qubit selection post routing so we probably could remove the heuristic from the pass now.

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.

#8763 will automatically upgrade DenseLayout to recurse into control-flow when counting up the cx and measure gates, and that PR is perhaps a bit more logically the right location for such a change. I get the argument that DenseLayout's heuristics are going to be a bit rubbish for control flow, but I think having the explicit test, and just having them be automatically upgraded is fine - if we want to revisit the heuristic, it's better to do that in a non-control-flow specific PR anyway.

qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
test/python/transpiler/test_dense_layout.py Outdated Show resolved Hide resolved
@ewinston ewinston changed the title [WIP] add control flow handling to DenseLayout pass add control flow handling to DenseLayout pass Sep 28, 2022
Very little needs to be done for this pass, since the majority of its
logic is circuit-independent.  There is a heuristic based on the average
number of cx gates per qubit, but since `DAGCircuit.count_ops` is now
recursive itself, this is trivial to raise.

Co-authored-by: Jake Lishman <[email protected]>
@jakelishman jakelishman force-pushed the controlflow/dense_layout branch from bcf30ef to 90aa68e Compare September 29, 2022 21:37
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.

I've rebased this properly over #8763 now that it's merged. The principal remaining change is now mostly the test, and since this is just a simple pass, I think this is fine.

@jakelishman jakelishman added Changelog: New Feature Include in the "Added" section of the changelog automerge labels Sep 29, 2022
@jakelishman
Copy link
Member

The heuristic here is really pretty meaningless (and clearly intended for backends that have cx as their only 2+q gate), and the Target path doesn't even attempt to put a number on it. I'm not bothered at all about doing more work on the heuristic, since it's already not really doing anything.

@mergify mergify bot merged commit 3c9d8d5 into Qiskit:main Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants