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.

Sign up for GitHub

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

Jump to bottom

Fix handling of holes in physical bits list in Layout #8767

Merged
merged 6 commits into from
Sep 20, 2022
Merged

Fix handling of holes in physical bits list in Layout #8767

merged 6 commits into from
Sep 20, 2022

Conversation

mtreinish
Copy link
Member

Summary

Previously the behavior of Layout.add(virtual) would attempt to select an available physical bit to pair for the layout. However, the manner in which this selection was done was buggy and it would potentially skip over available physical bits on the device and instead add a new physical bit to the layout. This had unintended consequences for layouts that added bits in higher numbers first. This commit fixes this behavior by first checking that we've exhausted all available physical bits from 0 to max bit in layout. Then if there are no holes in the physical bits in the layout it will add a bit to the top.

Details and comments

Fixes #8667

@mtreinish
Fix handling of holes in physical bits list in Layout
a473c5f 
Previously the behavior of `Layout.add(virtual)` would attempt to select
an available physical bit to pair for the layout. However, the manner in
which this selection was done was buggy and it would potentially skip
over available physical bits on the device and instead add a new
physical bit to the layout. This had unintended consequences for layouts
that added bits in higher numbers first. This commit fixes this behavior
by first checking that we've exhausted all available physical bits from
0 to max bit in layout. Then if there are no holes in the physical bits
in the layout it will add a bit to the top.

Fixes Qiskit#8667
@mtreinish mtreinish requested a review from a team as a code owner September 16, 2022 21:56
@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 mtreinish mentioned this pull request Sep 16, 2022
Closed
@mtreinish mtreinish added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Sep 16, 2022
@mtreinish mtreinish added this to the 0.22 milestone Sep 16, 2022
@coveralls
Copy link

coveralls commented Sep 16, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 3091500098

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 84.394%
Totals Coverage Status
Change from base Build 3090394614: 0.2%
Covered Lines: 59262
Relevant Lines: 70221
💛 - Coveralls

@mtreinish
Copy link
Member Author

I think that I should write an upgrade release note for this, it's potentially breaking behavior change. I view this as a bug and the previous behavior was ill defined, but it'll be better to document this in the release notes in case it catches someone off guard.

jakelishman
jakelishman reviewed Sep 20, 2022
View reviewed changes
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.

Agreed that highlighting this change in the "API changes" section is a good idea, despite the underlying behaviour being pretty buggy.

qiskit/transpiler/layout.py
Comment on lines +167 to +179
if len(self._p2v) == 0:
physical_bit = 0
else:
max_physical = max(self._p2v)
# Fill any gaps in the existing bits
for physical_candidate in range(max_physical):
if physical_candidate not in self._p2v:
physical_bit = physical_candidate
break
# If there are no free bits in the allocated physical bits add new ones
else:
physical_bit = max_physical + 1

Copy link
Member

@jakelishman jakelishman Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This feels iffy to me because adding a bit has linear cost, so it's quadratic to do in a loop even if there are no holes. This does only affect physical_bit=None, though, so it's not horrendous - does this have any appreciable impacts on any layout-pass benchmarks?

As an alternative, it's possible to remove the quadratic dependency (or at least restrict it to only those uses that are actually adding holes) if we always track a "next unallocated physical bit" integer in the class, and do this forwards search whenever the hole is filled in. The downside if that that adds a small constant cost to all additions, not just those with physical_bit=None. I don't know what the benchmarks are like, so I don't know what's most appropriate.

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 haven't run the benchmarks I'll spin up a run now to check. I wasn't super worried about it because while it does add a linear cost that's only for the physical_bit=None case and also I think the cases where you want to use that to extend the layout by 1 bit (the worst case performance-wise) is more uncommon. But lets see what the asv benchmarks show and we can see what makes the most sense here

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 just did a run of all the benchmarks that have layout in the name and it returned:

Benchmarks that have stayed the same:

       before           after         ratio
     [5c19a1f3]       [23544413]
                      <fix-add-register>
           failed           failed      n/a  mapping_passes.PassBenchmarks.time_apply_layout(14, 1024)
           failed           failed      n/a  mapping_passes.PassBenchmarks.time_apply_layout(5, 1024)
           failed           failed      n/a  mapping_passes.PassBenchmarks.time_layout_2q_distance(14, 1024)
           failed           failed      n/a  mapping_passes.PassBenchmarks.time_layout_2q_distance(5, 1024)
         749±90μs         865±60μs    ~1.16  mapping_passes.PassBenchmarks.time_dense_layout(5, 1024)
         811±80μs         842±70μs     1.04  mapping_passes.PassBenchmarks.time_dense_layout(20, 1024)
       28.9±0.1ms       29.5±0.2ms     1.02  mapping_passes.PassBenchmarks.time_noise_adaptive_layout(20, 1024)
          678±6ms          682±8ms     1.01  mapping_passes.PassBenchmarks.time_sabre_layout(5, 1024)
       18.5±0.1ms       18.6±0.2ms     1.00  mapping_passes.PassBenchmarks.time_noise_adaptive_layout(14, 1024)
      7.31±0.04ms      7.30±0.04ms     1.00  mapping_passes.PassBenchmarks.time_csp_layout(5, 1024)
       2.19±0.01s       2.19±0.02s     1.00  mapping_passes.PassBenchmarks.time_sabre_layout(14, 1024)
       30.4±0.2μs       30.3±0.1μs     0.99  mapping_passes.PassBenchmarks.time_set_layout(14, 1024)
       30.7±0.2μs       30.5±0.1μs     0.99  mapping_passes.PassBenchmarks.time_set_layout(5, 1024)
      5.53±0.02ms      5.50±0.03ms     0.99  mapping_passes.PassBenchmarks.time_noise_adaptive_layout(5, 1024)
        134±0.2ms          133±1ms     0.99  mapping_passes.PassBenchmarks.time_apply_layout(20, 1024)
       57.3±0.3ms       56.6±0.7ms     0.99  mapping_passes.PassBenchmarks.time_csp_layout(20, 1024)
       31.9±0.4ms       31.5±0.3ms     0.99  mapping_passes.PassBenchmarks.time_csp_layout(14, 1024)
       3.33±0.02s       3.26±0.02s     0.98  mapping_passes.PassBenchmarks.time_sabre_layout(20, 1024)
      31.2±0.09μs       30.5±0.1μs     0.98  mapping_passes.PassBenchmarks.time_set_layout(20, 1024)
       12.5±0.1ms       12.2±0.2ms     0.97  mapping_passes.PassBenchmarks.time_layout_2q_distance(20, 1024)
         867±60μs         670±20μs    ~0.77  mapping_passes.PassBenchmarks.time_dense_layout(14, 1024)

Benchmarks that have got worse:

       before           after         ratio
     [5c19a1f3]       [23544413]
                      <fix-add-register>
+      67.6±0.2μs       80.2±0.3μs     1.19  mapping_passes.PassBenchmarks.time_trivial_layout(14, 1024)
+      67.5±0.4μs       79.8±0.2μs     1.18  mapping_passes.PassBenchmarks.time_trivial_layout(5, 1024)
+     66.8±0.09μs       78.8±0.2μs     1.18  mapping_passes.PassBenchmarks.time_trivial_layout(20, 1024)

I'll do the full transpile level benchmarks now too

Copy link
Member Author

Choose a reason for hiding this comment

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

There was no real difference in the performance of the full path transpile benchmarks:

Benchmarks that have stayed the same:

       before           after         ratio
     [5c19a1f3]       [23544413]
                      <fix-add-register>
       4.99±0.02s       5.09±0.04s     1.02  transpiler_levels.TranspilerLevelBenchmarks.time_quantum_volume_transpile_50_x_20(1)
       1.44±0.01s       1.47±0.01s     1.02  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_qv_14_x_14(2)
        385±0.4ms          390±3ms     1.01  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_from_large_qasm_backend_with_prop(3)
          705±5ms          715±2ms     1.01  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_from_large_qasm_backend_with_prop(1)
       2.56±0.01s       2.59±0.01s     1.01  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_qv_14_x_14(3)
          370±2ms        375±0.9ms     1.01  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_from_large_qasm(3)
          409±2ms          414±2ms     1.01  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_from_large_qasm(1)
       12.3±0.03s       12.5±0.02s     1.01  transpiler_levels.TranspilerLevelBenchmarks.time_quantum_volume_transpile_50_x_20(3)
          514±4ms          519±5ms     1.01  transpiler_levels.TranspilerLevelBenchmarks.time_schedule_qv_14_x_14(0)
       1.04±0.01s       1.05±0.01s     1.01  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_qv_14_x_14(1)
       7.33±0.01s       7.39±0.01s     1.01  transpiler_levels.TranspilerLevelBenchmarks.time_quantum_volume_transpile_50_x_20(2)
        516±0.6ms          518±1ms     1.00  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_from_large_qasm_backend_with_prop(2)
       1.11±0.01s       1.11±0.01s     1.00  transpiler_levels.TranspilerLevelBenchmarks.time_schedule_qv_14_x_14(1)
       3.16±0.07s       3.17±0.03s     1.00  transpiler_levels.TranspilerLevelBenchmarks.time_quantum_volume_transpile_50_x_20(0)
        504±0.7ms          504±1ms     1.00  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_from_large_qasm(2)
        221±0.5ms        221±0.2ms     1.00  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_from_large_qasm_backend_with_prop(0)
             2565             2565     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_quantum_volume_transpile_50_x_20(0)
             2628             2628     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_quantum_volume_transpile_50_x_20(1)
             2628             2628     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_quantum_volume_transpile_50_x_20(2)
             1453             1453     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_quantum_volume_transpile_50_x_20(3)
             2705             2705     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_transpile_from_large_qasm(0)
             2005             2005     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_transpile_from_large_qasm(1)
             2005             2005     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_transpile_from_large_qasm(2)
                7                7     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_transpile_from_large_qasm(3)
             4705             4705     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_transpile_from_large_qasm_backend_with_prop(0)
             2005             2005     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_transpile_from_large_qasm_backend_with_prop(1)
             2005             2005     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_transpile_from_large_qasm_backend_with_prop(2)
                7                7     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_transpile_from_large_qasm_backend_with_prop(3)
              630              630     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_transpile_qv_14_x_14(0)
              644              644     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_transpile_qv_14_x_14(1)
              644              644     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_transpile_qv_14_x_14(2)
              391              391     1.00  transpiler_levels.TranspilerLevelBenchmarks.track_depth_transpile_qv_14_x_14(3)
       97.8±0.7ms       97.7±0.2ms     1.00  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_from_large_qasm(0)
          449±4ms          440±6ms     0.98  transpiler_levels.TranspilerLevelBenchmarks.time_transpile_qv_14_x_14(0)

BENCHMARKS NOT SIGNIFICANTLY CHANGED.

I think I can live with the mild regression in absolute terms of microseconds.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's not worry about that.

releasenotes/notes/register-add-fix-e29fa2ee47aa6d05.yaml Outdated Show resolved Hide resolved
releasenotes/notes/register-add-fix-e29fa2ee47aa6d05.yaml Outdated Show resolved Hide resolved
@jakelishman jakelishman added the Changelog: API Change Include in the "Changed" section of the changelog label Sep 20, 2022
@mtreinish @jakelishman
Apply suggestions from code review
2354441 
Co-authored-by: Jake Lishman <jake@binhbar.com>
@mergify mergify bot merged commit c0f5ae0 into Qiskit:main Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakelishman jakelishman jakelishman approved these changes

Assignees
No one assigned
Labels
Changelog: API Change Include in the "Changed" section of the changelog Changelog: Bugfix Include in the "Fixed" section of the changelog
Projects
None yet
Milestone
0.22
Development

Successfully merging this pull request may close these issues.

VF2Layout transpiler pass inserts extra qubits (more than are in the device)
4 participants
@mtreinish @qiskit-bot @coveralls @jakelishman