Update scheduling passes to group measurements#391
Conversation
…e for the backend compiler to optimize.
Pull Request Test Coverage Report for Build 3078473168
💛 - Coveralls |
Pull Request Test Coverage Report for Build 3078473168
💛 - Coveralls |
jakelishman
left a comment
jakelishman
left a comment
There was a problem hiding this comment.
I can't particularly comment on the internals of the scheduler, but here's a couple of minor ones on the blocking algorithm. Nothing is major, and if I were responsible for merging I'd be pretty close to being ready to merge.
| TODO: The need for this should be mitigated when Qiskit adds better support for | ||
| blocks and walking them in its program representation. | ||
| """ | ||
| # Dictionary is used as an ordered set of nodes to process |
There was a problem hiding this comment.
I think this comment might be out of date - there's no dictionary here?
| next_nodes = dag.topological_op_nodes() | ||
| while next_nodes: | ||
| curr_nodes = next_nodes | ||
| next_nodes_set = set() | ||
| next_nodes = [] |
There was a problem hiding this comment.
This iterator confused me a bit at first. The main reason is, I think, just naming - next_nodes implied to me that nodes in there were in some sort of "layer" to be immediately placed. If it (and _set) was called something like deferred_nodes, I don't think I'd have done a double-take.
| def block_order_op_nodes(dag: DAGCircuit) -> Generator[DAGOpNode, None, None]: | ||
| """Yield nodes such that they are sorted into blocks that they minimize synchronization. | ||
|
|
||
| This should be used when iterating nodes in order to find blocks within the circuit | ||
| for IBM dynamic circuit hardware | ||
|
|
||
| TODO: The need for this should be mitigated when Qiskit adds better support for | ||
| blocks and walking them in its program representation. | ||
| """ |
There was a problem hiding this comment.
This function internally generates a kind of blocky structure, but the exposed iterator is flat - would we be better yielding blocks at a time to maintain the structure for consumers? I see it doesn't seem to be needed in the current PR, but I worry a little about us needing to re-encode the block-splitting logic in consumers.
| if isinstance(node.op, (Reset, Measure)): | ||
| next_nodes_set |= set( | ||
| node | ||
| for node in dag.descendants(node) | ||
| if isinstance(node, DAGOpNode) | ||
| ) |
There was a problem hiding this comment.
This algorithm feels quadratic in the number of nodes in the graph, since descendants is all descendants, not just immediate ones. If that's a concern (and it may well not be), I think it's possible to invert the "node-blocking" logic to track a "placeable frontier", rather than having all nodes start placeable, and having to remove ones that aren't repeatedly. We use a similar algorithm in the Sabre routing passes - I can write a summary if it's something you want to look at.
There was a problem hiding this comment.
FYI, It should be possible to accomplish the same via the key to kwarg to DAGCircuit.topological_op_nodes (for example, see
https://github.com/Qiskit/qiskit-terra/blob/4f650581f2395ecc4c46c9259bbf35fc82955a26/qiskit/transpiler/passes/optimization/collect_multiqubit_blocks.py#L108 )
| @@ -129,8 +138,18 @@ def _visit_conditional_node(self, node: DAGNode) -> None: | |||
| """ | |||
| # Special processing required to resolve conditional scheduling dependencies | |||
| if node.op.condition_bits: | |||
There was a problem hiding this comment.
Do we still need this if block? With change at this line the _visit_conditional_node will be never called when conditional bits are empty, so this is obvious.
|
|
||
| # If True we are coming from a conditional block. | ||
| # start a new block for the unconditional operations. | ||
| if self._conditional_block: |
There was a problem hiding this comment.
What happens if we input
qc = QuantumCircuit(3, 1)
qc.x(0).c_if(0, True)
qc.x(1) # <- seems like new block is created here
qc.x(2).c_if(0, True)It seems to me like having all three instructions in a single block should be fine. Perhaps starting new block is not a problem because this program doesn't have barrier.
|
|
||
| self._current_block_idx = block_idx | ||
|
|
||
| t1 = t0 + node.op.duration # pylint: disable=invalid-name |
There was a problem hiding this comment.
I noticed I had misunderstanding about conditional delay. This is likely not the scope of this PR, but for example a conditional X gate can be understood as (if my understanding is correct)
- condition = True: Apply X gate of duration 160 dt
- condition = False: Apply delay of duration 160 dt, e.g. turn off port output
and this is an implicit assumption in the current scheduling logic, i.e. conditional operation doesn't change instruction duration. However, in case of conditional delay,
- condition = True: Apply delay of duration 100 dt
- condition = False: Apply what? delay of duration 100 dt? eliminate itself from dag?
If we want to eliminate delay, the backend should dynamically reschedule instructions and update event table -- this is not currently supported I guess. So we should exclude Delay from CONDITIONAL_SUPPORTED.
| from qiskit.dagcircuit import DAGCircuit, DAGOpNode | ||
|
|
||
|
|
||
| def block_order_op_nodes(dag: DAGCircuit) -> Generator[DAGOpNode, None, None]: |
There was a problem hiding this comment.
Does this work in the same way also for ALAP? If not perhaps *_asap?
kdk
left a comment
kdk
left a comment
There was a problem hiding this comment.
This LGTM, and since this is new functionality that is opt-in, we can address the comments (none of which looked blocking) in follow-on issues.
5 participants
Summary
This addresses the comments from @ajavadia here.
This pass also updates the scheduling so as not to place a barrier between conditional operations to enable the backend to have the opportunity to optimize them.
FYI @kdk
Details and comments