Page MenuHomePhabricator

[mlir] Add Complex Type, Vector Type and Tuple Type subclasses to python bindings
ClosedPublic

Authored by zhanghb97 on Aug 28 2020, 7:44 AM.

Details

Summary

Based on the PyType and PyConcreteType classes, this patch implements the bindings of Complex Type, Vector Type and Tuple Type subclasses.
For the convenience of type checking, this patch defines a mlirTypeIsAIntegerOrFloat function to check whether the given type is an integer or float type.
These three subclasses in this patch have similar binding strategy:

  • The function pointer isaFunction points to mlirTypeIsA***.
  • The mlir***TypeGet C API is bound with the get_*** method in the python side.
  • The Complex Type and Vector Type check whether the given type is an integer or float type.

Diff Detail

Event Timeline

zhanghb97 created this revision.Aug 28 2020, 7:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2020, 7:44 AM
zhanghb97 requested review of this revision.Aug 28 2020, 7:44 AM
mehdi_amini added inline comments.Aug 29 2020, 10:50 AM
mlir/lib/Bindings/Python/IRModules.cpp
503

Can we be more explicit in the error message? Is it possible to print the type that was passed in and also mention the expected types?

506

Stella is out for the next week, something that isn't clear to me with the lifetime management in many places is that we keep_alive the provided PyType while it is only the associated PyMlirContext that should be kept alive, the PyType could be free'd immediately after this method completes I believe.

537

I am puzzled by what this keep_alive intends to achieve? Seems like it keeps alive the rank integer?
Maybe you intended py::keep_alive<0, 3>() here?

554

Can you use a SmallVector instead?

570

num_types?

stellaraccident requested changes to this revision.Aug 29 2020, 8:04 PM

I am indeed out and on mobile, so just let a couple of response comments.

mlir/lib/Bindings/Python/IRModules.cpp
506

It isn't ideal and there was a thread on it that I'd have to look up. I am not aware of a way to keep_alive through an intermediate parent. The facility only allowed extending the lifetime of an object passed as an argument. We can/should discuss it more, but I think that Alex and I concluded that this has the desired effect, and the cost seems manageable. Other ways of getting at the underlying context are not great and require carrying more things around.

The most obvious alternative would require passing the context separately here.

527

Why not take a vector of int64_t? (Which would allow a python sequence) seems to be mimicking the c API when it should be providing a normal python API.

This revision now requires changes to proceed.Aug 29 2020, 8:04 PM
mlir/lib/Bindings/Python/IRModules.cpp
551

I think you can just take a std::vector<PyType>. Separately, it would be nice to teach pybind about SmallVector and not use std::vector art these boundaries, but that can be done as a cleanup later.

bondhugula added inline comments.
mlir/lib/Bindings/Python/IRModules.cpp
500–501

No need of else.

532

No need of else.

zhanghb97 updated this revision to Diff 288889.Aug 30 2020, 9:36 PM
zhanghb97 marked 5 inline comments as done.
  • Add the invalid type and expected types in the error message.
  • get_vector method takes a vector of int64_t.
  • get_tuple method takes py::list and maps the py::list to SmallVector.
  • Fix some details (keep_alive and method name).
zhanghb97 added inline comments.Aug 30 2020, 9:37 PM
mlir/lib/Bindings/Python/IRModules.cpp
503

Done - Add the invalid type and expected types in the error message.

506

I don't understand why the PyType is freed immediately after this method completes. I think the PyType should be kept alive until the returned PyComplexType is freed.

527

Done - Take a vector of int64_t.

537

Done - Fix the "Patient" of the keep_alive.

551

I am wondering what “not use std::vector art these boundaries” means? Although we use the std::vector<PyType>, we still should map the std::vector<PyType> to std::vector<MlirType> or a SmallVector, so I keep taking the py::list and use a SmallVector here.

551

I think you can just take a std::vector<PyType>. Separately, it would be nice to teach pybind about SmallVector and not use std::vector art these boundaries, but that can be done as a cleanup later.

554

Done - Use a SmallVector.

570

Done - Use num_types.

zhanghb97 updated this revision to Diff 288890.Aug 30 2020, 9:46 PM
  • Remove else.
zhanghb97 marked 2 inline comments as done.Aug 30 2020, 9:49 PM
mehdi_amini added inline comments.Mon, Aug 31, 8:37 PM
mlir/lib/Bindings/Python/IRModules.cpp
506

I don't understand why the PyType is freed immediately after this method completes.

It isn't freed because of the "keep_alive" here. And this is the sense of my comment: this is wasteful to have all these Python objects kept alive around while they aren't needed anymore.

I think the PyType should be kept alive until the returned PyComplexType is freed.

There is nothing owned directly by the PyType that is used in the PyComplexType. The PyComplexType only needs the Context to stay alive.
As Stella mentioned, the problem is how to express a "keep_alive" on the context from here.

Seems like you need to rebase? The build couldn't apply the patch apparently.

Hi @mehdi_amini - If there is no problem with the code, could you please help me land them, I don't have commit access so far. Thanks!

mehdi_amini accepted this revision.Tue, Sep 1, 9:40 PM
This revision was not accepted when it landed; it landed in state Needs Review.Tue, Sep 1, 10:46 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.