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

Unit Attribute Handling #161

Merged
merged 5 commits into from
Aug 15, 2022
Merged

Unit Attribute Handling #161

merged 5 commits into from
Aug 15, 2022

Conversation

webmiche
Copy link
Collaborator

@webmiche webmiche commented Aug 12, 2022

This PR concerns UnitAttrs and how to handle them as I need it to add a llvm.emit_c_interface to a function operation.

Currently, this is primarily a hack that I used to get UnitAttrs through the MLIRConverter, but I think this nicely opens the can of worms that is design decisions around UnitAttrs.

The approach of this PR is an implicit one. Anything that maps a str to None is just a UnitAttr with that name.

Obviously, this does not add any functionality to AttributeDef and just hacks around in the attributes dictionary currently. However, I feel that this might not be too bad, at least not the spirit of it. In my understanding, UnitAttrs are not really something you want to define in an AttributeDef (unless it is as part of an OptAttr), but rather information you want to pass down by just adding some attribute.

On the user-side I have this code:

f = FuncOp.from_region("main", batches, [],
                         Region.from_block_list([body_block]))
f.attributes['llvm.emit_c_interface'] = None

@math-fehr
Copy link
Collaborator

Hmm, I'm not sure I really like this approach :/ But it's maybe because I don't understand what is the problem.
My main issue with this solution is that the dictionary can hold both an Attribute and a None. So you can't use dict.get(my_key) to get a key if it exists, since None would be mapped to the UnitAttr.

If we defined a UnitAttr (which has nothing in it), wouldn't that solve the problem? And we can of course have something in the printer to make it look better. That way, your convert_attribute should still work, right?
And of course, we would put the UnitAttr in an OptAttributeDef.

@webmiche
Copy link
Collaborator Author

Yeah, you are right, I overlooked that problem. I will draft a version, that adds a UnitAttr 👍

self.print("\"%s\" = " % attribute_list[0][0])
self.print_attribute(attribute_list[0][1])
self._print_attr_string(attribute_list[0][0], attribute_list[0][1],
True)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If anyone sees a way to make this nicer, please tell me!

Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be a print_list method in Printer that should do what you want ;)
That way, you can remove your if not first: self.print(", ")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow, that is way better!

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

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

Besides my comments, this looks good!

src/xdsl/printer.py Outdated Show resolved Hide resolved
src/xdsl/printer.py Outdated Show resolved Hide resolved
self.print("\"%s\" = " % attribute_list[0][0])
self.print_attribute(attribute_list[0][1])
self._print_attr_string(attribute_list[0][0], attribute_list[0][1],
True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be a print_list method in Printer that should do what you want ;)
That way, you can remove your if not first: self.print(", ")

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

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

LGTM, we can squash and merge it if you want!

@webmiche webmiche merged commit 2d9bf87 into main Aug 15, 2022
@math-fehr math-fehr deleted the unit_attr_hack branch August 15, 2022 13:57
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

2 participants