Pulse Compiler - Scheduling pass #11981
Conversation
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
|
One or more of the the following people are requested to review this:
|
There was a problem hiding this comment.
This is very quick review. I'll review this again after #11980 is merged.
| @@ -77,6 +77,11 @@ def sequence(self) -> PyDAG: | |||
| """Return the DAG sequence of the SequenceIR""" | |||
| return self._sequence | |||
|
|
|||
| @property | |||
| def time_table(self) -> defaultdict: | |||
There was a problem hiding this comment.
I'm curious why this needs to be a defaultdict? Since the scheduler logic naively assumes all previous nodes are scheduled, it's safer to raise a key error (or particular compiler error) instead of silently substituting t0=0.
There was a problem hiding this comment.
the _time_table attribute is a defaultdict because we use it for initial_time, duration etc.
There was a problem hiding this comment.
Can we use internal _time_table for them and typecast to dict when the property returns?
There was a problem hiding this comment.
If you meant to do return dict(self._time_table) - that would return a different object and make _time_table it hard to mutate the base line dictionary. Did you have something else in mind?
There was a problem hiding this comment.
That's good point. Let's return same object as is.
| @@ -217,11 +222,19 @@ def _draw_nodes(n): | |||
| def flatten(self, inplace: bool = False) -> SequenceIR: | |||
| """Recursively flatten the SequenceIR. | |||
|
|
|||
| The flattening process includes breaking up nested IRs until only instructions remain. | |||
There was a problem hiding this comment.
Can we provide this functionality as a pass? Seems like no existing pass requires flatten. I'm okey with visualizing a graph with nested (unflatten) sequences, if this is only for visualization purpose. Some target might require flatten if it doesn't have notion of context, so having the flatten pass still makes sense to me.
There was a problem hiding this comment.
For the visualization purpose we can add a dedicated visualization function to the qiskit.visualization module, and call the flatten pass internally depending on the argument, e.g.
def visualize_sequence_ir(sequence_ir: SequenceIR, flat_nested: bool = True):
There was a problem hiding this comment.
I am not sure if this is something that should be a pass or a built-in method. I tend towards the latter because that's an operation which would probably be a stand alone - you want to visualize, or you want to flatten before sending to the backend. I don't think the user\developer should bother with a pass manager for something like that.
In other words - if the most likely scenario is a pass manager with only one pass, I think that's a good indication that it shouldn't be a pass.
There was a problem hiding this comment.
In case you submit the IR to backend, you know target code and whether flatten is required or not. For example, if you output Schedule like payload flatten is required, but something like qe-compiler Dialect or OpenPulse code, they have context and we can keep nested context. In this sense, a pass for flatten seems reasonable to me.
In my thoughts, only Transform pass can touch the graph structure. IR should NOT modify the structure of IR itself. Having this method obscures the responsibility, i.e. SRP
There was a problem hiding this comment.
That makes sense as well. I will split flatten out in a separate PR. (It wasn't introduced in this PR - only its tests)
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Summary
A scheduling pass is introduced. This PR depends on #11980.
Details and comments
The Scheduling Pass is introduced in a similar way to the
SetSequencepass of #11980. Scheduling logic is added to theAlignmentKindclasses.Some tests which depended on scheduling and were commented out before are restored.