Update GateDirection and checker for control flow #8822
Conversation
These are largely straightforwards, but since each pass has essentially two separate versions, it became simpler to refactor them out when doing the recursion, rather than using internal closures. As part of the refactor, we also swap to using `Target.instruction_supported` to check for validity - this method was added after the passes were originally made target-aware.
jakelishman
added
Changelog: New Feature
|
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:
|
Pull Request Test Coverage Report for Build 3161855539
💛 - Coveralls |
There was a problem hiding this comment.
Overall this LGTM, I really like the new code structure it's much easier to follow, especially in gate_direction. The only thing that sticks out are the inline comments I left on check_gate_direction. But the same comment applies to gate_direction. Basically we're making these passes operate in O(2n) when we can do it in O(n). I think after we combine the loops this should be good to go.
| elif node.name == "rzx": | ||
| if self.target.instruction_supported( | ||
| qargs=qargs, operation_class=RZXGate, parameters=node.op.params | ||
| ): | ||
| continue | ||
| if self.target.instruction_supported( | ||
| qargs=swapped, operation_class=RZXGate, parameters=node.op.params | ||
| ): | ||
| dag.substitute_node_with_dag(node, self._rzx_dag(*node.op.params)) | ||
| else: | ||
| raise TranspilerError( | ||
| f"The circuit requires a connection between physical qubits {qargs}" | ||
| f" for {node.name}" |
There was a problem hiding this comment.
Ooh, this is nice, thanks for fixing the TODO there! @ajavadia will be glad to see this because it pushes us closer to being able to enable https://arxiv.org/abs/2111.02535 by default in transpile() if the backend supports tuned variants of rzx.
There was a problem hiding this comment.
Oh yeah - since Target.instruction_supported got added, the todo became very easy to action. I think we're probably leaving some efficiency on the table here when it's called in a loop with operation_class set, but that's for another day most likely.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Summary
These are largely straightforwards, but since each pass has essentially two separate versions, it became simpler to refactor them out when doing the recursion, rather than using internal closures. As part of the refactor, we also swap to using
Target.instruction_supportedto check for validity - this method was added after the passes were originally made target-aware.Details and comments