Page MenuHomePhabricator

[MLIR][Python] Add capsule methods for pybind11 to PyValue.
ClosedPublic

Authored by mikeurbach on Apr 22 2021, 10:36 AM.

Details

Summary

Add the getCapsule() and createFromCapsule() methods to the
PyValue class, as well as the necessary interoperability.

Diff Detail

Event Timeline

mikeurbach created this revision.Apr 22 2021, 10:36 AM
mikeurbach requested review of this revision.Apr 22 2021, 10:36 AM

This is basically https://reviews.llvm.org/D99927 for Values instead of Operations. I've been testing it downstream in CIRCT. We have use-cases where we'd want a Python API that accepts a Value, or returns a Value, and this seemed to be missing for that.

Another thing we'll want is a nice casting helper, like this: https://github.com/llvm/circt/pull/940/commits/69f527952aa5acb2c73cc3b74e836483b5a5a5d8. It seems we have been adding this support downstream when needed. Does that kind of thing belong in the downstream project, or could I add it in MLIR? I don't see much pybind11 casting functionality in MLIR, so I'm curious if it's because it doesn't belong here or if we just haven't had the chance to move it upstream.

mlir/lib/Bindings/Python/IRModule.h
728

Frankly I'm in a little over my head here, is this the correct statement about ownership?

mlir/test/Bindings/Python/ir_value.py
25

Is this correct? It was confusing me last night... value2 is value is False, because value is an instance of OpResult not Value. But value2 == value is True, which I think is what we care about in this case.

jdd added a subscriber: jdd.Apr 22 2021, 3:41 PM

Another thing we'll want is a nice casting helper, like this: https://github.com/llvm/circt/pull/940/commits/69f527952aa5acb2c73cc3b74e836483b5a5a5d8. It seems we have been adding this support downstream when needed. Does that kind of thing belong in the downstream project, or could I add it in MLIR? I don't see much pybind11 casting functionality in MLIR, so I'm curious if it's because it doesn't belong here or if we just haven't had the chance to move it upstream.

I originally stole this from npcomp. I think @stellaraccident said it was a candidate for upstreaming, which makes sense to me.

Adding @ftynse for visibility.

Rebase main

mikeurbach marked 2 inline comments as not done.Apr 27 2021, 4:38 PM

@mehdi_amini since you approved https://reviews.llvm.org/D101398, do you mind taking a look at this one as well?

stellaraccident accepted this revision.Apr 27 2021, 6:21 PM
stellaraccident added inline comments.
mlir/lib/Bindings/Python/IRModule.h
728

Yes, the statement PyOperation::forOperation in the implementation is ensuring this.

mlir/test/Bindings/Python/ir_value.py
25

Yes, this is the best we can do here (values are just a wrapper and not uniqued between the portion/c side so eq vs is is all we can do).

This revision is now accepted and ready to land.Apr 27 2021, 6:21 PM

It would be nice to upstream the casting helpers once you all have them in a good state. Ideally, we have an out of tree sample that we use them in to avoid submitting dead code. One step at a time....

mikeurbach marked 2 inline comments as done.Apr 27 2021, 6:43 PM
This revision was landed with ongoing or failed builds.Apr 27 2021, 7:14 PM
This revision was automatically updated to reflect the committed changes.

Thanks Stella!

It would be nice to upstream the casting helpers once you all have them in a good state. Ideally, we have an out of tree sample that we use them in to avoid submitting dead code. One step at a time....

Sounds good to me.