add controlflow handling to CheckMap transpiler pass. #8793
Conversation
|
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 3155923356
💛 - Coveralls |
There was a problem hiding this comment.
Thanks for splitting this off - it's easier to review, and hopefully to get this in first. I have a couple of concerns about using control-flow constructs with inner bits not in the outer circuits. It would also be good to test that nested control-flow constructs are also properly checked (both for valid and invalid routes).
| layout = Layout(dict(enumerate(cf_instr.qargs))) | ||
| for block in cf_instr.op.blocks: | ||
| dag_block = circuit_to_dag(block) | ||
| mapped_dag = dag_block.copy_empty_like() | ||
| order = layout.reorder_bits(mapped_dag.qubits) |
There was a problem hiding this comment.
The two uses of Layout here I think assume that the inner control-flow block and the outer circuit contain the same Qubit instances. The control-flow builders will do this, but it's not a requirement of the classes. It's possible that it'll be neater/easier to refactor this pass a little so that the current run method becomes an inner _run(self, dag, wire_map) method, which is what we use for the recursive calls, where wire_map maps inner-block virtual qubits up to the outer physical bits they are stored on.
There was a problem hiding this comment.
I just made the blocks map reference the root register so there was no need to divert run. It seems to accommodate the cases you mentioned in this review.
| def test_swap_mapped_cf_layout_change_true(self): | ||
| """Check control flow blocks with layout change are mapped.""" | ||
| num_qubits = 3 | ||
| coupling = CouplingMap([(i, i + 1) for i in range(num_qubits - 1)]) | ||
| qr = QuantumRegister(3) | ||
| cr = ClassicalRegister(3) | ||
| circuit = QuantumCircuit(qr, cr) | ||
| true_body = QuantumCircuit(qr) | ||
| true_body.cx(0, 2) | ||
| circuit.if_else((cr[0], 0), true_body, None, qr[[1, 0, 2]], cr) | ||
| dag = circuit_to_dag(circuit) | ||
| pass_ = CheckMap(coupling) | ||
| pass_.run(dag) | ||
| self.assertTrue(pass_.property_set["is_swap_mapped"]) |
There was a problem hiding this comment.
From the above comment: this test will fail if we instead did true_body = QuantumCircuit(3) (so used different Qubit instances), but it still should pass in that case. It would be good to have an explicit test that that works. Since we're looking to make this pass run after the layout analysis passes but before the "apply layout" stage, we can't guarantee that we'll be running this pass on a full physical circuit any more, so the control-flow blocks may not yet be normalised.
jakelishman
added
Changelog: New Feature
Summary
Adds control flow handling to the CheckMap transpiler pass.
Details and comments