-
Notifications
You must be signed in to change notification settings - Fork 37
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
0012-Pulse-Compiler-and-IR #45
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.
Thanks @TsafrirA. This proposal is well written. I need more clarification of class/instance members and class hierarchy for reviewers and future developers (so that they can understand design philosophy with this document). It would be great if you could start with the compiler and then introduce the channel syntax extension as a demonstration of capability.
The Pulse IR will provide the framework for the compiler to work on and perform the necessary compilation steps. The main components of the IR will be: | ||
|
||
- IR Block - A block of instructions (or nested blocks) with an alignment context. This structure is similar to the structure of ScheduleBlock and needed to support one of the main tasks of the compiler - scheduling. | ||
- IR Instruction - A pulse program instruction. Typically, it will be initialized without concrete timing, and will be scheduled during the compiler operation. |
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.
Please also define the members of these class. These would be something like
class PulseIR:
def __init__(self):
self.instructions # DAG, PulseIR can be nested into other instance?
self.metadata # maybe alignment is part of metadata?
class GenericInstruction:
def __init__(
self,
instruction_type: str,
duration: int,
logical_element: Optional[LogicalElement] = None,
frame: Optional[Frame] = None,
**operands,
):
self.t0 = None
self.instruction = instruction_type # opcode
self.duration = duration
self.logical_element = logical_element
self.frame = frame
self.operands = operands
Maybe we can use composite pattern to implement them?
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 instructions will be grouped in the IR objects, so I think that will make a composite pattern a bit redundant here?
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.
Could be. A good point of using composite pattern is that we can remove branching or dispaching which could make code more readable (originally we have implemented Schedule
and Instruction
by using this pattern). Maybe this doesn't fit in here because PulseIR
doesn't need time 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.
It seems to me like everything we can do with a composite pattern we can also do by acting on PulseIR
objects? We can revisit this later to see if we can improve code readability.
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.
Thanks @TsafrirA . This is almost in good shape. Can you also define the design of the MixedFrame
? Does this also have identifier in addition to the logical element and frame? -- likely having identifier helps IR -> target code conversion, i.e. a task to convert mixed frame into backend channel can be translated into assignment of one of identifiers that backend provides (lowering).
@nkanazawa1989 |
No. I meant identifier-aware mixed frame. I guess this makes handling of channel object much simpler. class MixedFrame:
def __init__(
*,
logical_element: LogicalElement | None = None,
frame: Frame | None = None,
identifier: str | None = None,
):
self.logical_element = logical_element
self.frame = frame
self.identifier = identifier When Play instruction is defined with the mixed_frame = MixedFrame(identifier=channel.name) When Play instruction is defined with mixed_frame = MixedFrame(logical_element=play.logical_element, frame=play.frame) If the backend doesn't allow (port, frame) payload and only accepts mixed frames, there must be another pass class IdentifyMixedFrame(GenericPass):
def run(passmanager_ir: PulseIR):
...
if instruction.mixed_frame.identifier is None:
# Consume mixed_frame.logical_element and mixed_frame.frame
# Assign obtained string to mixed_frame.identifier
else:
# Otherwise use default name, i.e. channel name Any thoughts? |
@nkanazawa1989 The question we should consider, is whether or not we actually want to allow mixing of mixed frames and channels at the IR level. If I have a channel which I can't map to a mixed frame (because I wasn't provided with a backend object, because the backend doesn't recognize that channel etc.), should I allow it in my IR? Wouldn't the safe bet be to raise an error when a channel can't be mapped to a mixed frame? |
I don't think backend provides the mapping from channel to mixed frame, because they as the same thing (and should have the same identifier). Indeed this structure allows us to support fake backends with minimum overhead -- they are necessary for unittest and Qiskit Dynamics simulation. Note that we stop adding new fake backends because of the package size issue, and existing fake backends don't have any convenient configuration object to convert channel into (logical element, frame) pair. So you should allow channels that backend cannot convert into mixed frame representation. Taking only name and considering it as a lowered mixed frame would minimize the mechanism for backward compatibility. (edit) IBM Backend can provide a plugin pass that converts conventional channel name into corresponding mixed frame name. |
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.
thanks @TsafrirA . I'm fine with the current proposal of searching for a frame and logical element for channels (probably this is better in terms of frame broadcasting). I'm really glad to finalize the design of the pulse compiler.
See RFC file for details.