-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DAG based Pulse IR - Demo #11839
DAG based Pulse IR - Demo #11839
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked IR update part. This looks good to me. Maybe you can split the PR into two parts so that I can review the sequence pass logic separately.
qiskit/pulse/ir/ir.py
Outdated
return None | ||
else: | ||
return min(elements_initial_times, default=None) | ||
return min([self._time_table.get(node, None) for node in first_nodes], default=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need try-except for TypeError because comparison operators don't support None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default=None
takes care of that.
qiskit/pulse/ir/ir.py
Outdated
self.initial_time = initial_time | ||
self.time_offset = 0 | ||
self._time_table = {} | ||
self._children = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to implement has_children
method? Can be implemented later as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually never used the _children
attribute... So I am not sure it should even exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be convenient when we need to implement something like flattening pass, but you can remove for now.
qiskit/pulse/ir/ir.py
Outdated
|
||
class IrInstruction(IrElement): | ||
"""Pulse IR Instruction | ||
class IrBlock: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we give this a different name? I guess the original intention was "Intermediate representation of ScheduleBlock", and it makes me think this is still a collection of commands, i.e. code block + alignment. Since main purpose of this representation is sequencing, I prefer something like SequenceIR
considering the lowering of the program; context alignment (lexical) -> sequence (ordered) -> schedule (t0 assigned).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong preference here.
# that they have been altered from the originals. | ||
|
||
|
||
class Alignment: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an abstract base class, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I didn't want to put too much work into this until we decide how to deal with the two alignments hierarchies.
qiskit/pulse/ir/ir.py
Outdated
else: | ||
self.initial_time = initial_time | ||
self.time_offset = 0 | ||
self._time_table = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self._time_table = {} | |
self._time_table = defaultdict(lambda: None) |
Maybe? Then you don't need to write self._time_table.get(index, None)
everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was thinking the same thing.
qiskit/pulse/ir/ir.py
Outdated
return [ | ||
[self._time_table.get(ni, None), self._sequence.get_node_data(ni)] | ||
for ni in self._sequence.node_indices() | ||
if ni not in (0, 1) | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return [ | |
[self._time_table.get(ni, None), self._sequence.get_node_data(ni)] | |
for ni in self._sequence.node_indices() | |
if ni not in (0, 1) | |
] | |
def _to_time_inst_tuple(n_index): | |
instruction = self._sequence.get_node_data(ni) | |
if t0 := self._time_table.get(n_index, None) is not None: | |
return t0 + self.time_offset, instruction | |
return None, instruction | |
return list( | |
map( | |
_to_time_inst_tuple, | |
filter(lambda i: i not in (0, 1), self._sequence.node_indices()), | |
) | |
) |
You need to consider the time offset. I prefer tuple form for time instruction duo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opted not to do that, because it causes duplication where the initial time of sub blocks is stored in two places - the time table of the parent block, and the sub block itself. In general, I think it's better to have the initial time directly in the nodes. Eventually, this is where you want this the data to live - what is the instruction, and what time it should be applied.
(In the current implementation I took care of the offset in the flatten
method. I left the sub-blocks unaware of their relative timing until that point).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the initial time of sub blocks is stored in two places - the time table of the parent block, and the sub block itself.
Hmm it depends how we calculate initial time. I assume an outer block just calls sub block's schedule_elements
and extend, but if you assume sub block's element time t0 = outer block t0 + sub block element t0
, then current logic looks reasonable.
(In the current implementation I took care of the offset in the flatten method. I left the sub-blocks unaware of their relative timing until that point).
I prefer this schedule_elements
returns elements in flattened fashion, since lower level representation may not have notion of context and I don't think of any use case requiring time information but context must be preserved. You can also add flatten
argument to this method (I prefer it defaults to True).
I think it's better to have the initial time directly in the nodes
I'm against this design, based on the following rule:
Same command (i.e. Instruction tied to channel) should be equivalent object.
When you attach time information directly to the command, for example, two play instructions issued at different time must become different objects. To avoid this, we need a wrapper object, e.g. scheduled command, and adopt composite pattern as you first implemented. However, this results in complex class dependency and class instantiation overhead as the number of instruction increases.
Summary
This is an initial example for a DAG based PulseIR as an alternative to the option added in #11767.
This is based on a code example by @nkanazawa1989.
Details and comments
In #11767 we used a composite pattern and a list based tracking of the elements in the IR.
Here, we use instead a DAG representation. When initialized, the IR is comprised of graph of only nodes, and the edges are later added as part of a sequencing pass. This makes scheduling simple, as demonstrated by the scheduling pass implemented here.
As #11743 is still pending, the passes implemented here are kept in temporary files until the compiler will be added, and they could be sorted into their final homes.
Demo
A lot of the functionalities we need are already in place.
Quick set up:
And we can dive into a simple example with 3 play pulses - two of them sharing a mixed frame.
Initially, the IR is just a collection of nodes. To understand the dependencies, we first need to find all
MixedFrame
s associated with eachPulseTarget
andFrame
. We can use an analysis pass for that: (currently implemented as function, later we'll hook it into the compiler as a pass).Examining
property_set["target_frame_map"]
we'll see thatQubitFrame(1)
is associated with two mixed frames, while all other objects are associated with just one of the two existing in the IR.We can use this to sequence the instructions according to the alignment we choose.
Note how the instructions acting on the same mixed frame "block" each other.
If we repeat this procedure with
AlignSequential()
we'll get insteadThe previous example didn't require any mapping or broadcasting. Let's see one with broadcasting:
Note how the shift phase instruction is broadcasted to all mixed frames, thus "blocking" all of them.
Now we can take this and schedule it.
Note how
q1qf2
is pushed to the left because it's not "blocked" by the other instructions. If we swapped this for right alignment we'll have that instruction pushed to the right:And we can do the same thing with sub blocks.
Note, how the sub block "blocks" all instructions which correspond to any of the mixed frames in it (but not others).
If we flatten the representation we get:
Note how the instructions within the original sub-block now "block" the following instructions by themselves.
The PR is not in shape to be merged as is, and several issues still need addressing:
Frame
orPulseTarget
with no associatedMixedFrame
will cause an error currently)AlignmentKind
?The PR does, however, provide a solid basis for discussion.