Page MenuHomePhabricator

[mlir][Python] Upstream the PybindAdaptors.h helpers and use it to implement sparse_tensor.encoding.
ClosedPublic

Authored by stellaraccident on May 9 2021, 6:15 PM.

Details

Summary
  • The PybindAdaptors.h file has been evolving across different sub-projects (npcomp, circt) and has been successfully used for out of tree python API interop/extensions and defining custom types.
  • Since sparse_tensor.encoding is the first in-tree custom attribute we are supporting, it seemed like the right time to upstream this header and use it to define the attribute in a way that we can support for both in-tree and out-of-tree use (prior, I had not wanted to upstream dead code which was not used in-tree).
  • Adapted the circt version of mlir_type_subclass, also providing an mlir_attribute_subclass. As we get a bit of mileage on this, I would like to transition the builtin types/attributes to this mechanism and delete the old in-tree only PyConcreteType and PyConcreteAttribute template helpers (which cannot work reliably out of tree as they depend on internals).
  • Added support for defaulting the MlirContext if none is passed so that we can support the same idioms as in-tree versions.

There is quite a bit going on here and I can split it up if needed, but would prefer to keep the first use and the header together so sending out in one patch.

Diff Detail

Event Timeline

stellaraccident created this revision.May 9 2021, 6:15 PM
stellaraccident requested review of this revision.May 9 2021, 6:15 PM
jdd accepted this revision.May 9 2021, 7:47 PM

Psyched to see this upstream! We've been using a subset of this in CIRCT with only the issue Mike encountered, though that may have been us.

Not being a pybind11 expert, this looks reasonable to me.

mlir/include/mlir/Bindings/Python/PybindAdaptors.h
113

Nice! My reading of this is that API users can just supply "None" and this'll get the default so the C++ side doesn't have to reason about null contexts, yes?

214

Pretty sure this should be "<->".

278

@mikeurbach was having some difficulty with readonly properties in CIRCT, though I don't know that I can articulate it. (https://github.com/llvm/circt/pull/1002). Would you like to comment, Mike?

mlir/lib/Bindings/Python/Dialects.h
1

This header needs updating.

This revision is now accepted and ready to land.May 9 2021, 7:47 PM

Fix issue where 'get' factory method was returning the base class.

stellaraccident marked 2 inline comments as done.

Comments.

mlir/include/mlir/Bindings/Python/PybindAdaptors.h
113

Yes!

278

I just pushed an update here which fixes this. I was shooting for a magical thing that didn't require going through the classmethod plumbing, but alas, my C++ template-foo is not up to par. It's basically the same at runtime so just opted for the thing that is at least non magical from a Python perspective (i.e. you would write this in Python roughly if doing it with ctypes).

ftynse added inline comments.May 10 2021, 7:05 AM
mlir/include/mlir/Bindings/Python/PybindAdaptors.h
34

Yes please!

70

Nit: expand auto

72–75

Nit: return !mlirAffineMapIsNull(value)

mikeurbach accepted this revision.May 10 2021, 8:26 AM

Thanks for pulling this upstream. It looks good to me, but I'm just a user, not a pybind expert.

mlir/include/mlir/Bindings/Python/PybindAdaptors.h
278

Thanks! I'm taking a look but I think it makes sense.

stellaraccident marked 2 inline comments as done.

Rebase and comments.

Addressed comments. Thanks.

This revision was landed with ongoing or failed builds.May 10 2021, 10:16 AM
This revision was automatically updated to reflect the committed changes.