This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][python binding] Add OpaqueAttribute to python binding.
ClosedPublic

Authored by longy on Mar 2 2022, 2:03 PM.

Diff Detail

Event Timeline

longy created this revision.Mar 2 2022, 2:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 2:03 PM
longy requested review of this revision.Mar 2 2022, 2:03 PM
ftynse requested changes to this revision.Mar 3 2022, 2:10 AM
ftynse added a subscriber: ftynse.
ftynse added inline comments.
mlir/lib/Bindings/Python/IRAttributes.cpp
332

Please put DefaultingPyMlirContext as the last argument.

333

We shouldn't expose this C-level API to Python. Consider Python buffer protocol, https://pybind11.readthedocs.io/en/stable/advanced/pycpp/numpy.html#buffer-protocol, which is already used in DenseElementsAttrs here, for example, https://github.com/llvm/llvm-project/blob/main/mlir/lib/Bindings/Python/IRAttributes.cpp#L367. Accepting a buffer of bytes and failing on the rest is likely enough.

This revision now requires changes to proceed.Mar 3 2022, 2:10 AM
longy added a comment.Mar 3 2022, 12:50 PM

PTAL

mlir/lib/Bindings/Python/IRAttributes.cpp
333

So your idea is to remove this get method?

ftynse added inline comments.Mar 4 2022, 2:59 AM
mlir/lib/Bindings/Python/IRAttributes.cpp
333

No, what I am suggesting is to use proper APIs and types. using an integer and a pointer is not how one is supposed to pass around a string in Python. Take a string with data here or, better, take something like bytes or bytearray as it better reflects the purpose of opaque attributes. For the latter, you may need to use the buffer protocol.

longy updated this revision to Diff 413645.Mar 7 2022, 3:27 PM

Addressing the comments. PTAL

longy added inline comments.Mar 7 2022, 3:37 PM
mlir/lib/Bindings/Python/IRAttributes.cpp
333

Hi, I made changes accordingly, using py::buffer to pass string from python to C++ and also moving the PyMlirContext to the end.
Please take a look. Thank you.

longy added a comment.Mar 8 2022, 12:22 PM

A gentle ping to the patch.

ftynse accepted this revision.Mar 11 2022, 1:42 AM
This revision is now accepted and ready to land.Mar 11 2022, 1:42 AM
This revision was automatically updated to reflect the committed changes.