Skip to content
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

[Encoding] Introduce "layouts" field to EncodingAttr. #19215

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hanhanW
Copy link
Contributor

@hanhanW hanhanW commented Nov 19, 2024

The revision introduces an optional "layouts" field to EncodingAttr. It is an array of attributes that describes the potential layouts on the device. It is an array because a device could have several executable targets. Note that it can be any attribute that implements EncodingLayoutAttrInterface. The expectation of the field is to bridge the logics between host codes and device codes. If an attribute does not implement the interface, it could be discarded anytime.

The revision also updates the TODO item for round_dims_to field. Because IREE is going to use the new "layouts" field and upcoming attribute interface to handle the allocation problem.

It is a step towards #17924

The revisions introduces a new attribute interface in the Encoding
dialect. The interface is used to query layout information needed to
materialize encoding attributes.

Any backend can implement the interface to interpret an encoding layout
based on their needs.

The current expectation of the interface is to propagate layout
information from backends to the host compliation or other targets. The
expectation can be adjusted as long as we identify additional needs for
encodings/layouts.

It is a step towards iree-org#17924

Signed-off-by: hanhanW <[email protected]>
OptionalParameter<"ArrayAttr", "An array of attributes that describes the "
"potential layouts on the device. It is an array because a device could "
"have several executable targets. Note that it can be any attribute with "
"encoding attribute interface implementation. The expectation of the field "
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably rebase this on top of #19216, so I can explicitly use the EncodingLayoutAttrInterface term here. Does it sound better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do fell like it will be great to combine the two (or rebase one on top of the other) and review them all together. The reason being one about the design and one is about implementation/application, so they share context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me rebase this on top of the other one. The PRs look relevant, but honestly I don't know how to write PR title and description if we combine them all together. I agree that the code is in a weird stage because there are no actual users. I should be able to add some unittests in upcoming PRs once I have them landed.

@hanhanW
Copy link
Contributor Author

hanhanW commented Nov 20, 2024

rebased and updated the comment, please review the last commit.

The revision introduces an optional "layouts" field to EncodingAttr. It
is an array of attributes that describes the potential layouts on the
device. It is an array because a device could have several executable
targets. Note that it can be any attribute with encoding attribute
interface implementation. The expectation of the field is to bridge the
logics between host codes and device codes. If an attribute does not
implement the interface, it could be discarded anytime.

It is a step towards iree-org#17924

Signed-off-by: hanhanW <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants