Based on the PyType and PyConcreteType classes, this patch implements the bindings of Shaped Type, Tensor Type and MemRef Type subclasses.
The Tensor Type and MemRef Type are bound as ranked and unranked separately.
This patch adds the ***GetChecked C API to make sure the python side can get a valid type or a nullptr.
Shaped type is not a kind of standard types, it is the base class for vectors, memrefs and tensors, this patch binds the PyShapedType class as the base class of Vector Type, Tensor Type and MemRef Type subclasses.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Still just on mobile so not a comprehensive review. Thanks for working on this.
mlir/lib/Bindings/Python/IRModules.cpp | ||
---|---|---|
173 | This is not a complete definition (see TensorType::isValidElementType) But I think this is not required: see comment below | |
521 | There is a design point here: should we present a python ShapedType intermediate class? (This is how it is done in c++ but it's also under discussion to change to a type interface). Offhand: we probably at least want to present this kind of API, so this may be the simplest/reasonable starting point. | |
596 | This is the wrong layer to be enforcing this invariant. I believe we really want to be calling the getChecked() factories (instead of get()). You will need to do some work on the C API to access it:
Imo, the python API should only use the getChecked variant, regardless of the choice for the C API: having high level functions that native assert/abort on invalid inputs is very unpythonic. |
- Add ***GetChecked C API.
- Bind the "get_***" python methods with the corresponding ***GetChecked C API.
- Add TODO to rework to error reporting.
- Add PyShapedType class as the base class of the PyVectorType, PyRankedTensorType, PyUnrankedTensorType, PyMemRefType and PyUnrankedMemRefType.
Nice job on adding the PyShapedType intermediate base class -- there is some trickiness there. There is a cleaner way to get to the same end that I've left a comment on: feel free to leave a TODO or try to take it on in this patch.
Other comments are minor.
mlir/include/mlir-c/StandardTypes.h | ||
---|---|---|
169 | Maybe change these comments throughout to something like: Same as "mlirVectorTypeGet" but returns a nullptr wrapping MlirType on illegal arguments, emitting appropriate diagnostics. | |
mlir/lib/Bindings/Python/IRModules.cpp | ||
528 | You are going to want the self argument to be a PyShapedType in order to call helpers on it. | |
538 | I think this will abort if called on an UnrankedTensorType? (note "Otherwise, abort." notes in the C++ StandardTypes.h) I suggest adding a helper method to PyShapedType requireHasRank() which will raise a ValueError with verbiage like "Calling this method requires that the type has a rank." if it does not have a rank. | |
549 | Needs a call to requireHasRank() as above. | |
556 | Needs a call to requireHasRank() as above. | |
575 | Needs a call to requireHasRank() as above. | |
596 | For these APIs, I am moderately biased towards making the location optional and internally creating an UnknownLocation if needed. This deviates from the C-API, but I think it makes sense for Python where everything is checked and we do not have the option of bypassing the checking (but may wish to not strew locations everywhere in global contexts). Just a TODO is fine. If you were to go ahead and make it optional, you would find that llvm::Optional<> is not known by pybind. In order to teach it, you need something like is done here for absl::optional: https://github.com/google/iree/blob/a0c1b93da8c62681dad570f1198d63bb63172d5a/bindings/python/pyiree/common/binding.h#L27 (note that you would also want an additional declaration for llvm::None in order to be able to specify it as a default, but I would need to fiddle with that to get it to work right and don't have a recipe on hand) | |
622 | Can you add a TODO here for me: Switch back to bindDerived by making the ClassTy modifiable by subclasses, exposing the ShapedType hierarchy. I'd need to fiddle with it, but I think the right way involves changing PyConcreateType: template <typename T, typename BaseT = PyType> class PyConcreteType : public PyType { using ClassTy = py::class_<T, BaseT>; ... Then in a second-level derived class like this: class PyRankedTensorType : public PyConcreteType<PyRankedTensorType, PyShapedType> { static void bindDerived(ClassTy &c) { // Additional attributes here will extend those defined in PyShapedType based on python inheritance. } You are welcome to try it yourself, but if there is more to it than that, there is a subtle template thing going on that will require some debugging. | |
762 | Typo: s/spcae/space/ | |
mlir/test/Bindings/Python/ir_types.py | ||
250 | Can you add a test that rank, get_dim_size, and is_dynamic_size raise an appropriate exception (and do not crash). | |
298 | Ditto on exception tests as for UnrankedTensorType |
- Add a helper method requireHasRank().
- Change comments and self arguments.
- Add TODO for location and swithing back to bindDerived.
mlir/include/mlir-c/StandardTypes.h | ||
---|---|---|
169 | Done - Change the comment of ***GetChecked C API. | |
mlir/lib/Bindings/Python/IRModules.cpp | ||
528 | Done - Change self argument to PyShapedType. | |
538 | Done - Add a helper method requireHasRank(). | |
596 | Add a TODO: Make the location optional and create a default location. | |
622 | Add a TODO. I tried it myself, and failed to deal with the assertion: static assertion failed: Unknown/invalid class_ template parameters provided When it comes to class_<PyRankedTensorType, PyShapedType>, it seems that the PyShapedType is unknown as the template parameter, the PyShaped. |
Maybe change these comments throughout to something like: