Remove init peephole optimization discrete basis check (backport #12898) #12920
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![mergify[bot]](https://avatars.githubusercontent.com/in/10562?s=60&v=4){kind=small}
* Fix target handling in discrete basis check In #12727 a check was added to the default init stage's construction to avoid running 2q gate consolidation in the presence of targets with only discrete gates. However the way the target was being used in this check was incorrect. The name for an instruction in the target should be used as its identifier and then if we need the object representation that should query the target for that object based on the name. However the check was doing this backwards getting a list of operation objects and then using the name contained in the object. This will cause issues for instructions that use custom names such as when there are tuned variants or a custom gate instance with a unique name. While there is some question over whether we need this check as we will run the consolidate 2q blocks pass as part of the optimization stage which will have the same effect, this opts to just fix the target usage for it to minimize the diff. Also while not the explicit goal of this check it is protecting against some bugs in the consolidate blocks pass that occur when custom gates are used. So for the short term this check is retained, but in the future when these bugs in consolidate blocks are fixed we can revisit whether we want to remove the conditional logic. * Remove check and fix ConsolidateBlocks bug This commit pivots this PR branch to just remove the additional logic around skipping the optimization passes for discrete basis sets. The value the check was actually providing was not around a discrete basis set target and instead was to workaround a bug in the consolidate blocks pass. If a discrete basis set target was used this would still fail because we will unconditionally call `ConsolidateBlocks` during the optimization stage. This commit opts to just remove the extra complexity of the conditional execution of the peephole optimization passes and instead just fix the underlying bug in `ConsolidateBlocks` and remove the check. (cherry picked from commit 70c2f78)
|
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 following people are relevant to this code:
|
github-actions
bot
added
the
Changelog: Bugfix
jakelishman
approved these changes
Aug 8, 2024
Pull Request Test Coverage Report for Build 10294102649Details
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
In #12727 a check was added to the default init stage's construction to avoid running 2q gate consolidation in the presence of targets with only discrete gates. However the way the target was being used in this check was incorrect. The name for an instruction in the target should be used as its identifier and then if we need the object representation that should query the target for that object based on the name. However the check was doing this backwards getting a list of operation objects and then using the name contained in the object. This will cause issues for instructions that use custom names such as when there are tuned variants or a custom gate instance with a unique name.
While there is some question over whether we need this check as we will run the consolidate 2q blocks pass as part of the optimization stage which will have the same effect, this opts to just fix the target usage for it to minimize the diff. Also while not the explicit goal of this check it is protecting against some bugs in the consolidate blocks pass that occur when custom gates are used. So for the short term this check is retained, but in the future when these bugs in consolidate blocks are fixed we can revisit whether we want to remove the conditional logic.
Details and comments
This is an automatic backport of pull request #12898 done by Mergify.