This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][python bindings] Add TypeCaster for returning refined types from python APIs
ClosedPublic

Authored by makslevental on May 18 2023, 6:42 PM.

Details

Summary

depends on D150839

This diff uses MlirTypeID to register TypeCasters (i.e., [](PyType pyType) -> DerivedTy { return pyType; }) for all concrete types (i.e., PyConcrete<...>) that are then queried for (by MlirTypeID) and called in struct type_caster<MlirType>::cast. The result is that anywhere an MlirType mlirType is returned from a python binding, that mlirType is automatically cast to the correct concrete type. For example:

c0 = arith.ConstantOp(f32, 0.0)
# CHECK: F32Type(f32)
print(repr(c0.result.type))

unranked_tensor_type = UnrankedTensorType.get(f32)
unranked_tensor = tensor.FromElementsOp(unranked_tensor_type, [c0]).result

# CHECK: UnrankedTensorType
print(type(unranked_tensor.type).__name__)
# CHECK: UnrankedTensorType(tensor<*xf32>)
print(repr(unranked_tensor.type))

This functionality immediately extends to typed attributes (i.e., attr.type).

The diff also implements similar functionality for mlir_type_subclasses but in a slightly different way - for such types (which have no cpp corresponding class or struct) the user must provide a type caster in python (similar to how AttrBuilder works) or in cpp as a py::cpp_function.

Diff Detail

Event Timeline

makslevental created this revision.May 18 2023, 6:42 PM
Herald added a project: Restricted Project. · View Herald Transcript
makslevental retitled this revision from [MLIR][python bindings] Add TypeCaster for returning refined types from python APIs depends on D150839 to [MLIR][python bindings] Add TypeCaster for returning refined types from python APIs.May 18 2023, 6:42 PM
makslevental edited the summary of this revision. (Show Details)

edit commit message

add register_type_caster

makslevental edited the summary of this revision. (Show Details)May 18 2023, 7:07 PM

add comments

makslevental published this revision for review.May 18 2023, 7:37 PM
rkayaith added inline comments.May 18 2023, 10:48 PM
mlir/python/mlir/ir.py
134

Where would the registration for non-builtin types go? If they go in this file then it'd mean ir.py will be pulling in a bunch of dialects, and if they go somewhere else users would have to know to run the registration somehow (unless you do what PyGlobals::lookupOperationClass does, and automatically import the dialect module when looking up the caster).

mlir/test/python/dialects/python_test.py
461–463

I don't think replace is a good API to expose, what happens if you do print(repr(Type.parse("tensor<f32>"))) after this is registered?

To support this kind of usecase maybe there should be a stack of casters, with the newest registered casters tried first? But in general I'm not sure how this pattern would work, it seems to me like it's only safe for the dialect who "own"s the typeid to register casters for it, otherwise you could end up with competing casters, and no clear order to try them in.

jpienaar added inline comments.May 18 2023, 10:54 PM
mlir/python/mlir/ir.py
134

If you put this at filescope of the dialect you are registering it for, then wouldn't importing that dialect would run the registration?

rkayaith added inline comments.May 18 2023, 11:08 PM
mlir/python/mlir/ir.py
134

The case I'm thinking of is if you get an object through some generic APIs, e.g.

from mlir import ir
t = ir.Type.parse("!pdl.type", ir.Context())
print(type(t))  # could we make this be 'pdl.TypeType',
                # without ever importing 'pdl'?
jpienaar added inline comments.May 19 2023, 7:57 AM
mlir/python/mlir/ir.py
134

Not without hardcoding it/adding dependency somewhere general from what I can see. I mean you could do it C++ side along with dialect registration... But Python side it's unclear as the loading is purely in MLIRContext. Mmm, well there could be something like loadHook which Python side could listen to. I was actually thinking allowing downstream users customization is nice, it means you could reuse MLIR in memory while creating a form that is more similar to your python conventions/DSLs, and then yes we have a set of convenience ones that folks could opt in to. Not sure if too many levels of freedom, but the thinking was it allowed for using more convenient libraries without making core depend on them.

rkayaith added inline comments.May 19 2023, 8:52 AM
mlir/python/mlir/ir.py
134

Not without hardcoding it/adding dependency somewhere general from what I can see.

The reason I bring it up is because the ir.Operation equivalent of this case works (I think, can't actually try at the moment):
https://github.com/llvm/llvm-project/blob/main/mlir/lib/Bindings/Python/IRModule.cpp#L142-L145
So couldn't the same approach be used here?

mlir/test/python/dialects/python_test.py
461–463

Or if this is more a "hold my beer" kind of API for the DSL usecase like Jacques mentioned, maybe at least some warning/guidance somewhere on where replace should/shouldn't be used?

implement dialect loading for type casters

makslevental marked an inline comment as done.May 23 2023, 3:19 PM
makslevental added inline comments.
mlir/python/mlir/ir.py
134

implemented

mlir/test/python/dialects/python_test.py
513–517

ask and ye shall receive @rkayaith

makslevental added inline comments.May 23 2023, 3:28 PM
mlir/test/python/dialects/python_test.py
461–463

I'm definitely a "hold my beer" kind of dev. Also a stack of casters will inevitably wind up with us (me?) dealing re-entrancy. It's up to you - I definitely want the ability to overrule upstream casters in order to add utility functions to those types (e.g., stuff like dtype and shape for RankedTensorType) but I'm not married to it (and mlir_type_subclass functionality works without it).

makslevental added inline comments.May 23 2023, 3:29 PM
mlir/include/mlir-c/Support.h
145–147 ↗(On Diff #524904)

I believe this is correct C vernacular for forward decls but clang complains with

'mlirTypeIDGetDialect' has C-linkage specified, but returns incomplete type 'struct MlirDialect' which could be incompatible with C [-Wreturn-type-c-linkage]

The reason you need a forward decl is because these are defined in mlir-c/IR.h which includes this header. Let me know if there's a way to do this that silences the clang warnings.

makslevental edited the summary of this revision. (Show Details)May 23 2023, 3:31 PM

fix name of typeid gettor for transformoperation

typedef correctly

implement caster for mlir_type_subclass correctly

rkayaith added inline comments.May 23 2023, 11:26 PM
mlir/include/mlir/Bindings/Python/PybindAdaptors.h
278–299

I would move this logic into a method on ir.Type, something like py::object PyType::downcastUsingTypeCaster() (not sure what to name it). That way you can access PyGlobals directly, and having a straightforward way to invoke the cast should simplify the tests (can do Type(derived_type).downcast() instead of tunnelling through arith.ConstantOp)

483

Having to use regex here doesn't seem great, could you use __str__ or mlirTypePrint directly to avoid that?

mlir/lib/Bindings/Python/IRModule.cpp
83

seems odd for typeName to be an argument here when the only thing it's used for is the exception message, I would remove it but not a strong opinion.

137

This assumes that the mlirTypeID is the TypeID of an mlir::Type, but we'd want to extend this to work for attributes as well. The caller should know what kind of thing they're looking up, so I would make the MlirDialect an argument to this function.

140

StringRef should be fine here

mlir/test/python/dialects/python_test.py
479–482

I don't think you need the wrapping function here

485–486

should print and check the repr here

513–517

nice! could you add a comment here about what's going on, something like "we got a transform.OperationType without explicitly importing transform"

also not sure this is the right place to have this test, since it's not related to the python_test dialect

mlir/test/python/ir/attributes.py
542–551

are all the cases in this test needed? seems like there's overlap with the tests in builtin_types.py

mlir/test/python/ir/builtin_types.py
381–382

the rename here seems unnecessary?

646–651

these seem like they should be removed from this test

fix typecaster cache

makslevental added inline comments.May 24 2023, 7:29 AM
mlir/include/mlir/Bindings/Python/PybindAdaptors.h
278–299

good idea!

483

you can use __str__ but i thought that would be weird. mlirTypePrint requires a whole dance because PyPrintAccumulator isn't exposed. So I'll do __str__.

mlir/lib/Bindings/Python/IRModule.cpp
83

It's a debugging aid but okay I'll take it out.

mlir/test/python/dialects/python_test.py
479–482

yup that's correct but i wrote it long-hand so it's clear as an example - anyone that knows python well enough will realize you can skip the single hop

mlir/test/python/ir/attributes.py
542–551

yea you're right - got a little overzealous

mlir/test/python/ir/builtin_types.py
381–382

it's because i import the memref dialect up top now

fix typecaster cache

makslevental marked an inline comment as done.

move typecaster to PyType as maybe_downcast; remove superfluous tests; move MlirTypeID (and MlirTypeIDAllocator) to silence C linkage warnings

makslevental marked 7 inline comments as done.

move tests around

makslevental marked an inline comment as done.May 24 2023, 9:58 AM

add named MLIR_PYTHON_CAPI_MAYBE_DOWNCAST_ATTR macro

add named MLIR_PYTHON_CAPI_MAYBE_DOWNCAST_ATTR macro

change register_type_caster as top-level function; restore old mlirTypeID API and get dialect directly from type

stray comments removed

Thanks for all the iteration on this! Mostly minor comments here, LGTM once they're addressed but I'd like @ftynse / others to take a look as well.

mlir/include/mlir-c/Bindings/Python/Interop.h
110

nit, wording here seems a bit awkward

mlir/include/mlir-c/BuiltinTypes.h
317–318 ↗(On Diff #525287)

there's a typo here, but I don't think you actually need this function anyways so it could just be dropped

(also the existing mlirShapedTypeGetElementType could be used instead of exposing mlirRankedTensorTypeGetElementType)

mlir/include/mlir/Bindings/Python/PybindAdaptors.h
432–439

can use a default arg here too

486

huring?

488–493

nit: the reinterpret_borrow looks kind of scary to my untrained eye, when you "only" want to capture by-value

495

shouldn't need to replace here, as far as I can tell

mlir/lib/Bindings/Python/Globals.h
66

nit: I would use lowercase type here and in the other comments, since in theory the same registry could be used for Attributes and other things as well

67
mlir/lib/Bindings/Python/IRCore.cpp
3287–3288

I don't think the returned typeid can ever be null, so this check could be removed/turned into an assert

mlir/lib/Bindings/Python/IRModule.cpp
80–82
209–210

typeCasterMapCache should be cleared here to be consistent with the other caches

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

extra arg is accidentally getting converted to bool

mlir/test/python/ir/builtin_types.py
531

nit: function name seems stale, maybe print_downcasted?

571

can all the EmptyOp/FromElementsOp/FuncOp be replaced with the same print_item approach used above?

mlir/test/python/lib/PythonTestModule.cpp
68–71

any reason to not just use mlirRankedTensorTypeGetTypeID directly? could probably use a comment here to explain what's going

77
makslevental marked 13 inline comments as done.

fix typos and such

mlir/include/mlir/Bindings/Python/PybindAdaptors.h
432–439

you can't you get error: call to constructor of 'mlir::python::adaptors::mlir_type_subclass' is ambiguous with the first constructor.

486

"here during" -> "huring" but moot now that you showed me how to remove the borrow.

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

this is basically testing that PyValue.type is traveling through the type_caster. in fact it wasn't trivial to catch all of the places that PyType was wrapping a return (and thus evading the typecaster (and it's possible I missed one or two...)

remove duplicate constructors

ftynse accepted this revision.May 26 2023, 1:53 AM

LGTM.
Beware that a huge python reformatting change has landed last night so you may need to reformat your changes with black.

mlir/include/mlir-c/Bindings/Python/Interop.h
116

Nit: can we remove CAPI from the macro name? It doesn't seem related to C API and breaks the naming scheme compared to the macros above that relate to functions prefixed with _CAPI.

125

If this function is not expected to be called directly by the user (seems to be the case), I'd mark it "private" by prefixing with "_".

mlir/lib/Bindings/Python/IRCore.cpp
3289

Nit: expand auto

mlir/lib/Bindings/Python/IRModule.cpp
138

Nit: I believe there's an unwrap overload for MlirStringRef that does this.

This revision is now accepted and ready to land.May 26 2023, 1:53 AM
makslevental marked 3 inline comments as done.

incorporate comments

mlir/include/mlir-c/Bindings/Python/Interop.h
125

it is expected that users might call this (e.g. it's exercised in python_test.py.

ftynse added inline comments.May 26 2023, 8:47 AM
mlir/include/mlir-c/Bindings/Python/Interop.h
125

I've seen that, but test is not a user. Tests may be calling some things that the users are not expected to, e.g., we are calling "private" functions to test lifetime management.

makslevental added inline comments.May 26 2023, 8:49 AM
mlir/include/mlir-c/Bindings/Python/Interop.h
125

That's true but I was just confirming that indeed that API is public and was iterated on (started life resembling AttrBuilder.register_attribute_builder) in order to be ergonomic for the public to use.

This revision was landed with ongoing or failed builds.May 26 2023, 9:02 AM
This revision was automatically updated to reflect the committed changes.
jpienaar added inline comments.May 29 2023, 10:39 AM
mlir/include/mlir/CAPI/Support.h
47

Should this be a pure C file?

makslevental added inline comments.May 29 2023, 12:20 PM
mlir/include/mlir/CAPI/Support.h
47

Ya you're right this is in the wrong place - I can't remember if I moved it here from elsewhere during a refactor or if I just spaced out. I guess it's only useful for the python bindings so I'll move it to somewhere in there.