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

Keep resets at beginning of circuits #10591

Merged
merged 17 commits into from
Aug 24, 2023

Conversation

nonhermitian
Copy link
Contributor

@nonhermitian nonhermitian commented Aug 8, 2023

Summary

Fixes #6943
Fixes #5921
Fix #5127

Details and comments

Disables the removal of reset instructions if performed at the beginning of a circuit. Previous behavior assumed that the input state of a circuit is zero, as is the case when executed on most (perhaps all) quantum hardware to date. However, the circuit object is abstract and exists without this context. Moreover, if one wanted to explicitly set the initial state to be the zeros state by adding resets, then the preset passes would remove this, invalidating the intention of the user.

This PR disables the pass that does this in the preset passmanagers. The tests that looked for removed resets are turned into tests that verify the resets are still there.

@nonhermitian nonhermitian requested a review from a team as a code owner August 8, 2023 17:05
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

I agree it's good to remove the assumption that all backends have initial zero states, however the only alternative now would be to manually run the RemoveResetInZeroState pass. Do we want to make this simpler for users, given that a reset can take a significant amount of time in shallow circuits?

test/python/compiler/test_transpiler.py Show resolved Hide resolved
qiskit/transpiler/preset_passmanagers/level3.py Outdated Show resolved Hide resolved
qiskit/algorithms/algorithm_result.py Outdated Show resolved Hide resolved
@nonhermitian
Copy link
Contributor Author

the only alternative now would be to manually run the RemoveResetInZeroState pass. Do we want to make this simpler for users, given that a reset can take a significant amount of time in shallow circuits?

If resets are present at the beginning of a circuit they are usually explicitly put there my the user with intention. The only example where this is not the case is the initialize that adds them as well. This is really the whole point of removing the pass here is to keep explicitly added resets in place.

In terms of hardware performance, there is not much to be lost here. Again, it is assumed that an explicitly added reset is desired here. But in cases like initialize it adds little to the overall cost. It is basically ~1us per reset on IBM hardware. Compare this, for example to the 200us rep_delay between circuits.

Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

I solved the conflicts and consolidate the tests into the regular file.

@@ -248,6 +248,7 @@
# Custom extensions
# ---------------------------------------------------------------------------------------


Copy link
Member

Choose a reason for hiding this comment

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

black on the docs/conf.py file introduced this.

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
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.

Given the discussions in all the linked threads and in internal Slack, this looks good to me. Imo it's better to sometimes miss an optimisation in order to never have unsound optimisations such as removing reset when the backend's initialisation isn't guaranteed to be great.

This also opens us up a bit more to the ideas of subroutine compilations, where the input states aren't guaranteed zero, though likely we'll want some way for a circuit to communicate that more thoroughly in the future.

@jakelishman jakelishman added Changelog: API Change Include in the "Changed" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Aug 24, 2023
@jakelishman jakelishman added this to the 0.45.0 milestone Aug 24, 2023
)
else:
pre_optimization = common.generate_pre_op_passmanager(remove_reset_in_zero=True)
pre_optimization = common.generate_pre_op_passmanager(remove_reset_in_zero=False)
Copy link
Member

Choose a reason for hiding this comment

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

Doing this is no-op, with the arguments set like this it returns an empty PassManager. It'd probably be more efficient to just default pre_optimization to None and only set it if we have a coupling map or target available, to run the gate direction passes.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't have thought that would have an effect on performance, though, and keeping it like this means potentially a bit less coupling between the components? It doesn't feel like the call site needs/ought to know that this combination of options returns an empty object.

Copy link
Member

Choose a reason for hiding this comment

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

It`ll save a few hundred ns at most. It's why I only left a comment and didn't block it because it's probably fine like this. I just wanted to call it out because this whole branch logic was written to support the pre optimization construction that was done prior to the introduction of stages. Where before the optimization loop the passmanagers either ran gate direction and optionally remove reset depending on optimization level or just remove reset in zero if there are no connectivity constraints. That all feels unnecessary now if we're never running remove resets anymore.

@1ucian0 1ucian0 added this pull request to the merge queue Aug 24, 2023
Merged via the queue into Qiskit:main with commit 676b329 Aug 24, 2023
12 checks passed
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
Projects
None yet
6 participants