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 C API and Python bindings for IntegerAttr to string. #6787

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

mikeurbach
Copy link
Contributor

Both the upstream MLIR IntegerAttr and OM IntegerAttr are backed by an arbitrary precision integer. However, the upstream Python bindings don't have any mechanism to return an integer larger than 64 bits back to Python, even though Python ints are also arbitrary precision integers.

To support this, we can handle this where we explicitly convert OM IntegerAttrs to Python values. The simplest thing is to print a string representation of the arbitrary precision integer, and parse that to a Python int. This adds the necessary C API and Python binding for a "to string" method, and uses it in the attribute_to_var function.

There are smarter ways we can handle the conversion, but the "to string" API seems generally useful, so I'm using that in the conversion for now.

@mikeurbach
Copy link
Contributor Author

This solves the problem, but it's fairly hacky to do int(str(...))... any better ideas? We could define something smarter in the Python bindings for om::IntegerAttr, but we'd need to create C APIs to get the pieces of the underlying APInt.

There is another hack, that I'm not sure about. To make the string, we are currently creating a new StringAttr, and returning a StringRef that points to the newly interned string. I've tried various mechanisms to return a string, or accept a string reference as an argument to overwrite, but I wasn't able to make those work. Is there a better way to return/assign a string in a C API?

@mikeurbach mikeurbach force-pushed the mikeurbach/om-integer-attr-to-string branch from 363c10d to 7e4b8bd Compare March 5, 2024 22:51
Both the upstream MLIR IntegerAttr and OM IntegerAttr are backed by an
arbitrary precision integer. However, the upstream Python bindings
don't have any mechanism to return an integer larger than 64 bits back
to Python, even though Python ints are also arbitrary precision
integers.

To support this, we can handle this where we explicitly convert OM
IntegerAttrs to Python values. The simplest thing is to print a string
representation of the arbitrary precision integer, and parse that to a
Python int. This adds the necessary C API and Python binding for a "to
string" method, and uses it in the attribute_to_var function.

There are smarter ways we can handle the conversion, but the "to
string" API seems generally useful, so I'm using that in the
conversion for now.
@mikeurbach mikeurbach force-pushed the mikeurbach/om-integer-attr-to-string branch from 7e4b8bd to 25886dd Compare March 5, 2024 22:58
@uenoku
Copy link
Member

uenoku commented Mar 6, 2024

Yeah seems like there is no CAPI for bridging APInt (there was an old thread in the forum https://discourse.llvm.org/t/llvm-support-types-in-c-api/1751 but no progress). I think the proper solution is to add CPI for APInt to MLIR upstream but int(str(...)) looks fine since it's hidden in attribitue_to_var.

@mikeurbach
Copy link
Contributor Author

Interesting, I hadn't noticed that one. I agree it would be best to handle upstream. Let me start an issue with my real ask: I want arbitrary width attributes that have widths > 64 to be able to be passed to Python. I'm curious what the community thinks.

@mikeurbach
Copy link
Contributor Author

Seems like the general point upstream is we could take some time to think about the best way to do this for Python, but the easy thing of passing through a string is the most expedient way to get this done correctly. Any concern to merge this for now to get something working? I'm interested in designing the right thing here, but would also like to unblock for now.

Copy link
Contributor

@prithayan prithayan left a comment

Choose a reason for hiding this comment

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

This approach seems good enough, until we have a better API upstream.

@mikeurbach mikeurbach merged commit 1cc1069 into main Mar 8, 2024
4 checks passed
@mikeurbach mikeurbach deleted the mikeurbach/om-integer-attr-to-string branch March 8, 2024 01:46
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.

3 participants