This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add Shaped Type, Tensor Type and MemRef Type to python bindings.
ClosedPublic

Authored by zhanghb97 on Sep 3 2020, 6:53 AM.

Details

Summary

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.

Diff Detail

Event Timeline

zhanghb97 created this revision.Sep 3 2020, 6:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2020, 6:53 AM
zhanghb97 requested review of this revision.Sep 3 2020, 6:53 AM
stellaraccident requested changes to this revision.Sep 3 2020, 8:35 AM

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:

  • either change the getters to use getChecked under the hood (with a null location) or implement a GetChecked variant to the C API.
  • add an mlirTypeIsNull() if it doesn't already exist
  • add another todo about error messages being emitted via the diagnostic engine, watch is tbd

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.

This revision now requires changes to proceed.Sep 3 2020, 8:35 AM
zhanghb97 edited the summary of this revision. (Show Details)Sep 4 2020, 7:05 AM
zhanghb97 updated this revision to Diff 289954.Sep 4 2020, 7:56 AM
zhanghb97 edited the summary of this revision. (Show Details)
  • 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.
zhanghb97 added inline comments.Sep 4 2020, 7:59 AM
mlir/lib/Bindings/Python/IRModules.cpp
521

Now I bind the PyShapedType class as the base class of the PyVectorType, PyRankedTensorType, PyUnrankedTensorType, PyMemRefType and PyUnrankedMemRefType.

596

Done - I add ***GetChecked C API and use them in the bindings.

stellaraccident requested changes to this revision.Sep 4 2020, 9:57 AM

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

This revision now requires changes to proceed.Sep 4 2020, 9:57 AM
zhanghb97 updated this revision to Diff 290141.Sep 6 2020, 8:41 AM
zhanghb97 marked 9 inline comments as done.
  • 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.

stellaraccident accepted this revision.Sep 6 2020, 11:37 AM
This revision is now accepted and ready to land.Sep 6 2020, 11:37 AM
This revision was landed with ongoing or failed builds.Sep 6 2020, 11:46 AM
This revision was automatically updated to reflect the committed changes.
mlir/lib/Bindings/Python/IRModules.cpp
622

Fixed in D87208. I think you were close: the PyConcreteType needed to extend BaseT, not PyType, as I had suggested in my comment.