Add C and python bindings for InferShapedTypeOpInterface
and ShapedTypeComponents. This allows users to invoke
InferShapedTypeOpInterface for ops that implement it.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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). |
mlir/include/mlir-c/Interfaces.h | ||
---|---|---|
44 | Hey Alex! 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! |
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!
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. |
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. |
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); }, |
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. |
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! |
mlir/lib/Bindings/Python/IRInterfaces.cpp | ||
---|---|---|
330–340 | Haha no worries! I learned something new as well! |
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.