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

[OM] Add om.list_create to create lists of objects #5418

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Conversation

nandor
Copy link
Contributor

@nandor nandor commented Jun 16, 2023

This PR adds the op to create lists of objects.

The Python bindings are not yet implemented as further discussion of the in-memory representation is required.

@nandor nandor requested a review from mikeurbach June 16, 2023 16:22
Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, I think we'll definitely need this. My only question is if it's possible to use ODS to specify the textual format. We'll start sorting out how to represent a list in the evaluator.

ListType:$result
);

let hasCustomAssemblyFormat = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to use the result type in the textual form and use the assembly format in ODS? Eh on second thought this is how hw.array_create works, so this seems fine. It would still be nice to use the assembly format string in ODS, if it's possible to extract the type of the first operand in ODS.

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 think we should keep this in sync with hw.array_create. I don't know how to get ODS to specify the element type and check. I am pretty sure we can build a constraint to validate that arguments match the element type of an array, but then we'd have to specify the full list type for the list.

lib/Dialect/OM/OMOps.cpp Outdated Show resolved Hide resolved
lib/Dialect/OM/OMOps.cpp Outdated Show resolved Hide resolved
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@darthscsi darthscsi left a comment

Choose a reason for hiding this comment

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

LGTM. @mikeurbach should give the final stamp of approval.

Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

Yeah, this looks great, and we can merge it whenever we want. It will not work in the evaluator, and we'll need to make some larger changes at that level. Those are changes we should probably get around to regardless, but we haven't prioritized that yet.

@nandor nandor merged commit fd7837e into main Aug 8, 2023
5 checks passed
@nandor nandor deleted the dev/nandor/om-list branch August 8, 2023 16:45
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.

None yet

4 participants