This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][python bindings] Expose TypeIDs in python
ClosedPublic

Authored by makslevental on May 17 2023, 10:25 PM.

Details

Summary

This diff adds python bindings for MlirTypeID. It paves the way for returning accurately typed Types from python APIs (see D150927) and then further along building type "conscious" Value APIs (see D150413).

Diff Detail

Event Timeline

makslevental created this revision.May 17 2023, 10:25 PM
Herald added a project: Restricted Project. · View Herald Transcript

remove CAPI tests

update commit message

makslevental retitled this revision from expose typeids in python to [MLIR][python bindings] Expose typeids in python.May 18 2023, 12:59 PM
makslevental edited the summary of this revision. (Show Details)

add type_caster for MlirTypeID

makslevental edited the summary of this revision. (Show Details)May 18 2023, 6:48 PM
makslevental edited the summary of this revision. (Show Details)
makslevental published this revision for review.May 18 2023, 7:08 PM
makslevental retitled this revision from [MLIR][python bindings] Expose typeids in python to [MLIR][python bindings] Expose TypeIDs in python.
rkayaith added inline comments.May 18 2023, 10:25 PM
mlir/lib/Bindings/Python/IRCore.cpp
3303

In other places I see "cheap" functions like this are usually exposed as properties. There's also the class-level variant def_property_readonly_static.

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

Can you add this to mlir_type_subclass as well, and implement it for some non-builtin types as an example.

900

Instead of returning None, could we just skip defining the function? That would make it obvious when a class doesn't have it implemented.

mlir/test/python/ir/builtin_types.py
571–573

maybe just define these as a single list of tuples?

626

I would expect get_typeid() here to work the same as Type.get_typeid(), and return the concrete typeid (the id for vector in this case).
Maybe rename the static methods (static_typeid?) to disambiguate

makslevental marked 3 inline comments as done.

incorporate comments

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

I don't understand why (some pybind impl detail) but skipping defining on one such concrete leaves a property object descriptor there; i.e.,

if (DerivedTy::getTypeIdFunction)
  cls.def_property_readonly_static(
      "typeid", [](py::object & /*class*/) -> MlirTypeID {
        return DerivedTy::getTypeIdFunction();
      });

results in <property object at 0x7efe2dcebc40> being printed for ShapeType.typeid. Could do

if (DerivedTy::getTypeIdFunction)
  cls.def_property_readonly_static(
      "typeid", [](py::object & /*class*/) -> MlirTypeID {
        return DerivedTy::getTypeIdFunction();
      });
else
  cls.attr("typeid") = py::none();

but that just looks the same from the user side and is baroque (imho). Changed to throw a ValueError.

add static_typeid to mlir_type_subclass

makslevental marked 2 inline comments as done.May 19 2023, 8:48 AM
jpienaar added inline comments.May 19 2023, 8:50 AM
mlir/include/mlir-c/BuiltinTypes.h
63

OOC why not just

MlirTypeID mlirTypeGetTypeID(MlirType type);

?

remove unnecessary def_property_readonly_static

makslevental added inline comments.May 19 2023, 9:20 AM
mlir/include/mlir-c/BuiltinTypes.h
63

You mean in general why did I add all of these () -> TypeID functions? Because in the TypeCaster diff you need to eg build a map from TypeID -> TypeCaster without having a type on hand.

rkayaith added inline comments.May 19 2023, 9:36 AM
mlir/include/mlir-c/BuiltinTypes.h
63

if only we could generate all the C api boilerplate :)

ftynse accepted this revision.May 22 2023, 8:47 AM
ftynse added inline comments.
mlir/lib/Bindings/Python/IRCore.cpp
3309–3310

Let's use C++ API: throw py::value_error(...). Here and below.

This revision is now accepted and ready to land.May 22 2023, 8:47 AM
makslevental added inline comments.May 22 2023, 9:20 AM
mlir/lib/Bindings/Python/IRCore.cpp
3309–3310

Just clarifying where "below" refers to: there are ~50 instances of throw SetPyError in this directory - do you want me to replace all of them? If so, no problem, but that would be better in a separate diff? Otherwise ctrl+f in this UI, I see this one and the one in PyConcreteType::castFrom.

ftynse added inline comments.May 22 2023, 9:22 AM
mlir/lib/Bindings/Python/IRCore.cpp
3309–3310

I meant only the ones introduced in this diff. Though changing all the others in a separate diff is also much appreciated.

use C++ API throw py::value_error

makslevental marked an inline comment as done.May 22 2023, 9:26 AM
This revision was automatically updated to reflect the committed changes.
rkayaith added inline comments.May 24 2023, 6:42 PM
mlir/include/mlir-c/BuiltinTypes.h
76

nit, same for a bunch of others as well

mlir/test/python/ir/builtin_types.py
580–582

the keys of the dict here are the python *Type classes (IntegerType etc.), not PyTypeIDs like the comment says