This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Add InferShapedTypeOpInterface bindings
ClosedPublic

Authored by ataheridezfouli-groq on Apr 28 2023, 4:04 PM.

Details

Summary

Add C and python bindings for InferShapedTypeOpInterface
and ShapedTypeComponents. This allows users to invoke
InferShapedTypeOpInterface for ops that implement it.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
ataheridezfouli-groq requested review of this revision.Apr 28 2023, 4:04 PM
ftynse requested changes to this revision.Apr 30 2023, 4:19 PM

I have a design suggestion, please see inline comments.

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

This type isn't used in C++ in the same way as the other C_API_STRUCT classes this approach was intended for, that is, it is not a wrapper around a pointer (unlike Attribute/Type/*Op). Using a pointer for it would require C++ allocation and deallocation just for API purposes.

I'd consider a different approach here: "unpack" the structure into components (dimensions given as rank + pointer to the contiguous list of the first, MlirType elementType, MlirAttr attribute, int ranked), and pass them as separate attributes to the callback below. The user will then copy them over to any storage they have without having to deal with C++ memory.

96

Nit.

112

See above, this callback can take more than one argument.

115

Nit: canonical what?

mlir/lib/Bindings/Python/IRInterfaces.cpp
304–470

This class can still exist, but contain fields for scalars (type, attribute, boolean) and store contents of the shape array in a py::list so it is managed by Python.

340

Nit: please be systematic about ending the documentation with a period.

426–478

Please refactor the existing code to reuse it instead of copying.

mlir/lib/CAPI/Interfaces/Interfaces.cpp
149

Nit: unsigned->size_t.

171–198

Please refactor existing code into a function and call it instead of copying.

mlir/test/python/python_test_ops.td
112

Ultra-nit. (That's why it is better to have code in .cpp fliles).

This revision now requires changes to proceed.Apr 30 2023, 4:19 PM
mlir/include/mlir-c/Interfaces.h
44

Hey Alex!
Thanks for taking the time to review this code! I just wanted to clarify something before I go ahead and make the changes. Are you saying that we should still have the MlirShapedTypeComponents structure, but just define it as something like this:

struct MlirShapedTypeComponents {
    intptr_t rank;
    int64_t *shape;
    MlirType elementType;
    MlirAttr attribute;
    int ranked;
};

Or are you saying to get rid of MlirShapedTypeComponents altogether, as well as any of the C-APIs, as just pass around the components?

Thanks again for all your comments!

Addressed review comments

ataheridezfouli-groq marked 7 inline comments as done.May 1 2023, 4:38 PM

@ftynse

Thanks again for the review. I addressed your comments. My concern now is that now with the changes, PyShapedTypeComponents is completely separated from mlir::ShapedTypeComponents. Let me know what you think! Thanks!

ataheridezfouli-groq marked 2 inline comments as done.May 1 2023, 4:39 PM
ftynse accepted this revision.May 5 2023, 6:03 AM

LGTM with a couple of nits.

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

Either way is fine with me as long as there is no allocation for individual structs (it's okay to allocate an array thereof).

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

Nit: please document top-level entities.

47

Nit: now that this is a function, we can invert the condition here and do exit early (https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code).

50

Nit: use auto && with enumerate.

304–470

Nit: ShapedTypeComponents reads like a class, but we don't actually have one, spell it as shaped type components. Also, the ownership part of the comment doesn't seem relevant anymore.

327–329

I think the casters defined in PybindAdaptors.h should allow us to just return self.elementType here.

340–353

Optional: we may consider passing the shape and attribute as kwargs.

354–361

Nit: a more Pythonic way could be to have rank property return None when there is no rank instead of having a separate has_rank.

This revision is now accepted and ready to land.May 5 2023, 6:03 AM
ataheridezfouli-groq marked 8 inline comments as done.May 8 2023, 8:50 AM
ataheridezfouli-groq added inline comments.
mlir/lib/Bindings/Python/IRInterfaces.cpp
327–329

Seems to compile but during runtime throws this type error:

TypeError: Unable to convert function return value to a Python type!
354–361

I added the changes for rank and shape properties to return None when there is no rank. However I will still keep has_rank to be consistent with the mlir::ShapedTypeComponents interface as well as other python classes such as ShapedType. Let me know if you really want me to remove it.

ataheridezfouli-groq marked 2 inline comments as done.

Address various minor comments

Add hasRank to CAPI callback

makslevental added inline comments.
mlir/lib/Bindings/Python/IRInterfaces.cpp
330–340

nit: I believe all of these def_statics can be def_classmethods like this

cpp
      .def_classmethod(
          "get",
          [](const py::object &cls, MlirType elementType) {
            return cls(type);
          },
ataheridezfouli-groq marked an inline comment as done.May 10 2023, 8:51 AM
ataheridezfouli-groq added inline comments.
mlir/lib/Bindings/Python/IRInterfaces.cpp
330–340

py::class_ does not have a def_classmethod as seen here: https://github.com/pybind/pybind11/blob/cca4c51ca463ea02fa504331ff21bc313c80c7f3/include/pybind11/pybind11.h#L1496

In the MLIR python bindings, there is a hacky way to define classmethod in lib/Bindings/Python/IRCore.cpp::classmethod. But I believe this method just sets the attribute of an object, which in my case I can't have 3 attributes with the same name.

In general I agree with you that factory functions in python should use classmethod but this seems to be a limitation of pybind11. And I believe that's the reason why all the other py::class_ objects in MLIR use a static get method.

makslevental added inline comments.May 10 2023, 9:09 AM
mlir/lib/Bindings/Python/IRInterfaces.cpp
330–340

El Oh El I thought those uses of def_classmethod (in mlir_*_subclass) were via a pybind feature - I didn't realize they were actually members of pure_subclass. Alright carry on!

ataheridezfouli-groq marked 2 inline comments as done.May 11 2023, 7:54 AM
ataheridezfouli-groq added inline comments.
mlir/lib/Bindings/Python/IRInterfaces.cpp
330–340

Haha no worries! I learned something new as well!

ataheridezfouli-groq marked an inline comment as done.

Rebase

This revision was automatically updated to reflect the committed changes.