8000 [OM] Add a string type by uenoku · Pull Request #5277 · llvm/circt · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[OM] Add a string type #5277

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 31, 2023
Merged

[OM] Add a string type #5277

merged 2 commits into from
May 31, 2023

Conversation

uenoku
Copy link
Member
@uenoku uenoku commented May 26, 2023

This PR adds a string type to OM dialect.

@uenoku uenoku requested review from mikeurbach and prithayan May 26, 2023 07:18
uenoku
uenoku commented May 26, 2023
View reviewed changes
include/circt/Dialect/OM/OMAttributes.td Outdated
Comment on lines 62 to 63
def OMStringAttr : AttrDef<OMDialect, "OMString", [TypedAttrInterface]> {
let summary = "An attribute that wraps a StringAttr with !om.string type";
Copy link
Member Author

Choose a reason for hiding this comment

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

A tricky issue to wrap StringAttr with OMStringAttr is it makes hard to use common var_to_attribute/attribute_to_var python utility as we have to convert python str into OMStringAttr.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm this is annoying. I think we can probably add support for the OM dialect attributes to those helpers.

mikeurbach
mikeurbach approved these changes May 30, 2023
View reviewed changes
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.

This looks good to me, thanks!

include/circt/Dialect/OM/OMAttributes.td Outdated
Comment on lines 62 to 63
def OMStringAttr : AttrDef<OMDialect, "OMString", [TypedAttrInterface]> {
let summary = "An attribute that wraps a StringAttr with !om.string type";
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm this is annoying. I think we can probably add support for the OM dialect attributes to those helpers.

@youngar
Copy link
Member
youngar commented May 30, 2023

What do we need the new OMStringAttr for? Can we just use StringAttr?

@uenoku uenoku force-pushed the dev/uenoku/add-string-type branch from b5a388b to dd307a2 Compare May 30, 2023 21:14
@uenoku
Copy link
Member Author
uenoku commented May 30, 2023 •
edited
Loading

What do we need the new OMStringAttr for? Can we just use StringAttr?

Yeah, good point. I didn't know that StringAttr could have a type. I changed to use TypedStrAttr, thanks!

mikeurbach
mikeurbach approved these changes May 30, 2023
View reviewed changes
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.

Oh wow, I didn't realize there was a typed StringAttr now. How does the round trip test work? Does it parse a string attribute as a typed string attribute in this context?

Regardless, if all we need is the string type, that sounds good to me.

@uenoku
Copy link
Member Author
uenoku commented May 31, 2023

How does the round trip test work? Does it parse a string attribute as a typed string attribute in this context?

Yes, I think so. This is a dump in generic format.

"builtin.module"() ({
  "om.class"() ({
    %0 = "om.constant"() {value = "foo" : !om.string} : () -> !om.string
  }) {formalParamNames = [], sym_name = "StringConstant"} : () -> ()
}) : () -> ()

@uenoku uenoku merged commit 7d8e7ce into main May 31, 2023
@uenoku uenoku deleted the dev/uenoku/add-string-type branch April 11, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikeurbach mikeurbach mikeurbach approved these changes

@prithayan prithayan Awaiting requested review from prithayan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@uenoku @youngar @mikeurbach
0