Page MenuHomePhabricator

[mlir] support interfaces in Python bindings
ClosedPublic

Authored by ftynse on Oct 12 2021, 9:16 AM.

Details

Summary

Introduce the initial support for operation interfaces in C API and Python
bindings. Interfaces are a key component of MLIR's extensibility and should be
available in bindings to make use of full potential of MLIR.

This initial implementation exposes InferTypeOpInterface all the way to the
Python bindings since it can be later used to simplify the operation
construction methods by inferring their return types instead of requiring the
user to do so. The general infrastructure for binding interfaces is defined and
InferTypeOpInterface can be used as an example for binding other interfaces.

Diff Detail

Event Timeline

ftynse created this revision.Oct 12 2021, 9:16 AM
ftynse requested review of this revision.Oct 12 2021, 9:16 AM

Nice

mlir/include/mlir-c/Interfaces.h
54

And so these are the ones we want to autogenerate later?

57

Why would the callback be invoked multiple times? Don't we just set it once? E.g., this is a way to return a std::vector of Type, so I'd expect 1 return. Also what happens if it is called with 5 types, then 2 types, then 4 types ? We just have to discard all except the last?

ftynse updated this revision to Diff 379146.Oct 12 2021, 12:32 PM

Reorganize cmake.

Generally LGTM modulo my comments about collapsing the test dialect into the main module set.

mlir/include/mlir-c/Interfaces.h
32

Obvious to those of us who know, but can you document?

57

Honestly, for this kind of case I think it is defensible to have:

intptr_t *nReturn, MlirType *returnTypes

Then have it so that on a failure due to too many returns, it sets *nReturns (and otherwise sets it to 0).

In general, I dislike these kind of multi-dip APIs, but I expect this to be used a lot from contexts where the return arity is known.

I don't feel super strongly about this. It is kind of gross either way.

mlir/lib/Bindings/Python/IRInterfaces.cpp
52

For compatibility with out of tree, we may want to do this in the way that is done for attributes/types (see PybindAdaptors.h). However, there was deep hacking to get that to work, and I am fine check-pointing with this and then following up to generalize.

mlir/python/CMakeLists.txt
315

Do you need to gate this and the inclusion of it below based on whether tests are being built? (we disable tests in some downstreams)

368

I think we should just remove this and conditionally add MLIRPythonTestSources to the above if testing is enabled. I believe it is more important to be testing a real artifact, as assembled, vs having this kind of arbitrary division. Downstreams shouldn't need to deal with it (they can just not include MLIRPythonTestSources).

ftynse updated this revision to Diff 379378.Oct 13 2021, 7:42 AM
ftynse marked 2 inline comments as done.

Working cmake, documentation and more tests.

ftynse added inline comments.Oct 13 2021, 7:44 AM
mlir/include/mlir-c/Interfaces.h
54

Yes. This one is quite annoying given the number of operands to translate.

57

I took the same approach as we did for owning strings returned from functions. IIRC, we preferred callbacks to avoid double allocations. Otherwise, the code in bindings would malloc the array for individual types and the caller would have to free it and, potentially, copy the data to some other memory it manages differently. With callbacks, the caller still has to allocate and copy, but at least no handle the malloced array.

mlir/lib/Bindings/Python/IRInterfaces.cpp
52

This follows what we do for (builtin) attributes and types - https://github.com/llvm/llvm-project/blob/main/mlir/lib/Bindings/Python/IRModule.h#L656. Let's transition everything at once maybe.

ftynse retitled this revision from WIP: [mlir] support interfaces in Python bindings to [mlir] support interfaces in Python bindings.Oct 13 2021, 7:47 AM
ftynse edited the summary of this revision. (Show Details)
gysit accepted this revision.Oct 21 2021, 1:04 AM

LGTM! Only found some minor things and assuming the big picture comments are already addressed.

mlir/docs/CAPI.md
201

typo: appropriate

mlir/lib/Bindings/Python/IRInterfaces.cpp
33

nit: typo

mlir/python/CMakeLists.txt
316

nit: pyTypo :)

This revision is now accepted and ready to land.Oct 21 2021, 1:04 AM
ftynse updated this revision to Diff 381906.Oct 25 2021, 3:18 AM
ftynse marked 3 inline comments as done.

Address review.

This revision was automatically updated to reflect the committed changes.