This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Python] Add python binding to create DenseElementsAttribute.
ClosedPublic

Authored by stellaraccident on Oct 13 2020, 8:33 PM.

Details

Summary
  • Interops with Python buffers/numpy arrays to create.
  • Also cleans up 'get' factory methods on some types to be consistent.
  • Adds mlirAttributeGetType() to C-API to facilitate error handling and other uses.
  • Punts on a lot of features of the ElementsAttribute hierarchy for now.
  • Does not yet support bool or string attributes.

Diff Detail

Event Timeline

stellaraccident requested review of this revision.Oct 13 2020, 8:33 PM
zhanghb97 added inline comments.Oct 14 2020, 4:05 AM
mlir/lib/Bindings/Python/IRModules.cpp
1052

Why only consider tensor here? Should we also consider the vector case?

mlir/test/Bindings/Python/ir_array_attributes.py
8

I have numpy in my environment, so there is no problem. I'm wondering if the numpy is not installed, will the test fail?

mlir/lib/Bindings/Python/IRModules.cpp
1052

It's a fairly complicated API to use with all options exposed, and I wanted the default "import from array" case to just work without a lot of fiddling for a dominant case. I'll add a TODO to add optional args to control which shaped type gets used. I'll also extend the docs.

mlir/test/Bindings/Python/ir_array_attributes.py
8

Currently, yes. I'll address any issues here even we enable this by default in the build.

zhanghb97 added inline comments.Oct 17 2020, 6:40 PM
mlir/lib/Bindings/Python/IRModules.cpp
930

Now it seems that the view is freed correctly only when the calling PyObject_GetBuffer fails. But successful calls to PyObject_GetBuffer have no paired with calls to PyBuffer_Release when the attribute object ends its lifetime, I think it may cause resource leaks.

ftynse accepted this revision.Oct 19 2020, 9:11 AM
This revision is now accepted and ready to land.Oct 19 2020, 9:11 AM
mlir/lib/Bindings/Python/IRModules.cpp
930

Nice to be looking for it, but in this case it is safe because the py::buffer_info constructor defaults to stealing the Py_buffer* and will free it upon going out of scope.

Fwiw, I'm using a relatively unconventional way of getting the buffer_info here, resorting to the underlying C functions because I want to cast it to specific constraints so that I know it will line up with the memory format for constructing dense elements attributes. A lot of code that accesses a buffer doesn't do this and you won't see these extra coercions and error checking.

This revision was landed with ongoing or failed builds.Oct 19 2020, 10:37 PM
This revision was automatically updated to reflect the committed changes.