-
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
Introduce Pulse IR skeleton #11767
Introduce Pulse IR skeleton #11767
Conversation
One or more of the the following people are requested to review this:
|
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 great starting point of discussion. I like the use of composite pattern. This really fits in with our pulse model. However, I prefer simpler implementation and want to delegate all complexity to passes unless the method is used by multiple passes (so I prefer starting with minimum implementation and add methods later if we find it's necessary).
pass | ||
|
||
@abstractmethod | ||
def shift_initial_time(self, value: int): |
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 this must be implemented by a pass.
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.
Let's leave it in at the moment, and remove it later if we decide it's better to have it as a pass. I think during scheduling this is something we might have to do often, so it might make sense to have it as a method.
@property | ||
def initial_time(self) -> int | None: | ||
"""Return the initial time ``initial_time`` of the object""" | ||
elements_initial_times = [element.initial_time for element in self._elements] |
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 is O(N) lookup and expensive. I'd prefer turning the data structure into (rustworkx based) DAG and then return the first node to calculate the initial time. Alternatively we can use dict keyed on the initial time, but this doesn't work because None
shouldn't be a key.
@property | ||
def final_time(self) -> int | None: | ||
"""Return the final time of the ``IrBlock``object""" | ||
elements_final_times = [element.final_time for element in self._elements] |
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.
With the same reason I prefer DAG.
"""Return the length of the IR, defined as the number of elements in it""" | ||
return len(self._elements) | ||
|
||
def __repr__(self) -> str: |
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.
In my experience repr for sequence (e.g. ScheduleBlock) is not really useful, because it's super lengthy. If we prefer LLVM IR like, probably we can support .dump
method that generates parsable code. This also helps debugging (unittest); i.e. the reference is always text, which is very robust.
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.
(note that I don't suggest implementing the code in this PR, since you need to start from defining AST...)
Thanks @nkanazawa1989 for the quick review. With the exception of About |
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.
Let's merge the PR as a skeleton. I'll do some followup for internal machinery.
Summary
This PR continues the work on the new Pulse Compiler and IR (RFC) by introducing a skeleton for the IR representation.
Details and comments
The implementation follows the form first presented in #11234. Only basic functionalities were introduced at this point, because #11726 is still pending, and the work on the compiler (following #11743) will better reveal what functionality is missing.