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

Don't run ElidePermutation if routing is disabled #13184

Merged
merged 4 commits into from
Sep 19, 2024

Conversation

mtreinish
Copy link
Member

Summary

We have a routing plugin named "none" which is used to assert that no routing should be performed (it errors if there is routing required). We were previously running the ElidePermutations pass in this case which is not the expected or correct behavior because it causes the same kind of permutation as routing and would eliminate and swap gates in the circuit. As the typical use case for routing_method='none' is when you've already routed a circuit (or constructed it by hand to match the connectivity constraints) we shouldn't be doing this. This commit fixes this behavior so if the routing_method is set to "none" we no longer run the ElidePermutations pass.

Details and comments

Fixes #13144

We have a routing plugin named "none" which is used to assert that no
routing should be performed (it errors if there is routing required).
We were previously running the ElidePermutations pass in this case which
is not the expected or correct behavior because it causes the same kind
of permutation as routing and would eliminate and swap gates in the
circuit. As the typical use case for routing_method='none' is when
you've already routed a circuit (or constructed it by hand to match the
connectivity constraints) we shouldn't be doing this. This commit fixes
this behavior so if the routing_method is set to "none" we no longer run
the ElidePermutations pass.

Fixes Qiskit#13144
@mtreinish mtreinish added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Sep 18, 2024
@mtreinish mtreinish added this to the 1.2.2 milestone Sep 18, 2024
@mtreinish mtreinish requested a review from a team as a code owner September 18, 2024 19:15
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Sep 18, 2024

Pull Request Test Coverage Report for Build 10946369059

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 11 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.007%) to 88.783%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.02%
crates/qasm2/src/lex.rs 4 92.48%
crates/qasm2/src/parse.rs 6 97.15%
Totals Coverage Status
Change from base Build 10946116755: -0.007%
Covered Lines: 73484
Relevant Lines: 82768

💛 - Coveralls

@jakelishman
Copy link
Member

I'm certainly fine with this case, but for minor discussion, an extension: should we also disable ElidePermutations in the cases of all-to-all connectivity, or if VF2 can find that the existing interaction graph is perfect?

(My rough initial feeling to both is "no, leave it", especially since setting routing_method="none" is a way to disable it, but I feel like I could be convinced.)

@mtreinish
Copy link
Member Author

mtreinish commented Sep 18, 2024

Yeah, I think for now we should just leave it, especially for a potential backport.

I agree it's not exactly clear what we should do in the case of a VF2Layout match (which will always be the case for all to all connectivity). I can go either way on it I tend to think having an explicit opt-out path makes sense. But I also think setting routing_method="none" is not the best UI or the most discoverable if you have this kind of edge case. Maybe this can be improved with more explicit documentation on how to do this?

If we did want to only run elide permutations when there isn't a vf2 match, we'd have to move ElidePermutations into the layout stage and run it conditionally after VF2Layout but before SabreLayout. Which is definitely doable, but it adds complications around how we run ConsolidateBlocks in the init stage too, because of this I think the only path that makes sense is to run VF2Layout during the init stage and the layout stage. We would run it before ElidePermutations in it's current place to ensure we don't elide an already routed circuit. But if there isn't a match we'd want to run all the init optimizations including ElidePermutations and ConsolidateBlocks. However doing that might result in the circuit having a perfect match so we would need to run VF2Layout again.

Writing all of that I think I've convinced myself we should just make it an opt-out thing with the routing_method plugin.

@alexanderivrii
Copy link
Contributor

Just a small thought: we probably should enable ElidePermutations for all-to-all connectivity, as removing swap/permutation gates keeps the circuit routable yet makes it smaller.

jakelishman
jakelishman previously approved these changes Sep 19, 2024
@jakelishman jakelishman added this pull request to the merge queue Sep 19, 2024
Merged via the queue into Qiskit:main with commit 9a896d3 Sep 19, 2024
15 checks passed
mergify bot pushed a commit that referenced this pull request Sep 19, 2024
* Don't run ElidePermutation if routing is disabled

We have a routing plugin named "none" which is used to assert that no
routing should be performed (it errors if there is routing required).
We were previously running the ElidePermutations pass in this case which
is not the expected or correct behavior because it causes the same kind
of permutation as routing and would eliminate and swap gates in the
circuit. As the typical use case for routing_method='none' is when
you've already routed a circuit (or constructed it by hand to match the
connectivity constraints) we shouldn't be doing this. This commit fixes
this behavior so if the routing_method is set to "none" we no longer run
the ElidePermutations pass.

Fixes #13144

* Move release note to the correct location

(cherry picked from commit 9a896d3)
github-merge-queue bot pushed a commit that referenced this pull request Sep 20, 2024
* Don't run ElidePermutation if routing is disabled

We have a routing plugin named "none" which is used to assert that no
routing should be performed (it errors if there is routing required).
We were previously running the ElidePermutations pass in this case which
is not the expected or correct behavior because it causes the same kind
of permutation as routing and would eliminate and swap gates in the
circuit. As the typical use case for routing_method='none' is when
you've already routed a circuit (or constructed it by hand to match the
connectivity constraints) we shouldn't be doing this. This commit fixes
this behavior so if the routing_method is set to "none" we no longer run
the ElidePermutations pass.

Fixes #13144

* Move release note to the correct location

(cherry picked from commit 9a896d3)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@mtreinish mtreinish deleted the no-elide-routing-none branch September 20, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: transpiler Issues and PRs related to Transpiler stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ElidePermutation undoes routing
5 participants