This is an archive of the discontinued LLVM Phabricator instance.

Add basic JIT Python Bindings
ClosedPublic

Authored by mehdi_amini on Feb 25 2021, 7:48 PM.

Details

Summary

This offers the ability to create a JIT and invoke a function by passing
ctypes pointers to the argument and the result.

Diff Detail

Event Timeline

mehdi_amini created this revision.Feb 25 2021, 7:48 PM
mehdi_amini edited the summary of this revision. (Show Details)

Switch to use ctypes for argument passing

mehdi_amini published this revision for review.Mar 2 2021, 1:58 AM
ftynse accepted this revision.Mar 2 2021, 2:28 AM
ftynse added a subscriber: ftynse.
ftynse added inline comments.
mlir/lib/Bindings/Python/Conversions/CMakeLists.txt
11

Please add the newline

mlir/lib/Bindings/Python/Jit.cpp
62

I think you can now throw py::runtime_error("message")

67

Nit: dialects

mlir/lib/Bindings/Python/Jit.h
2

Wrong header

mlir/lib/Bindings/Python/mlir/jit.py
12

Other files use 2 spaces for indentation

mlir/lib/CAPI/ExecutionEngine/ExecutionEngine.cpp
26

We need some mechanism of exposing this registration to Python so that out-of-tree dialects can also register their translations.

mlir/test/Bindings/Python/jit.py
38

Nit: add an empty line above

This revision is now accepted and ready to land.Mar 2 2021, 2:28 AM
mehdi_amini marked 7 inline comments as done.

Address comments

mehdi_amini added inline comments.Mar 2 2021, 10:54 AM
mlir/lib/CAPI/ExecutionEngine/ExecutionEngine.cpp
26

ack. Not sure what we'll do for this? I suspect we'll push this on the dialects to provide an API to register them and push on the user to register before invoking the Jit?

mehdi_amini added inline comments.Mar 2 2021, 11:13 AM
mlir/lib/Bindings/Python/Jit.cpp
62

Any link to a doc or an example, it does not work for me?

Revert back to throw SetPyError(PyExc_RuntimeError and add a test for the failure

stellaraccident accepted this revision.Mar 2 2021, 5:48 PM
stellaraccident added inline comments.
mlir/lib/Bindings/Python/Jit.cpp
22

Stale comment: ... around an MlirExecutionEngine

Maybe also a brief comment about why you switch terminology to Jit.

60

Does the input Module need to survive past this call (or is it converted and then not needed)? If the former, then we have a lifetime hazard here.

62

Just throw std::exception. See the mapping here: https://pybind11.readthedocs.io/en/stable/advanced/exceptions.html

(this SetPyError form is something I carried forward from the distant past/other projects and in newer code, we are throwing exceptions directly if possible)

66

... must contain only ...

mlir/lib/Bindings/Python/mlir/jit.py
13

Elsewhere we exclusively use triple-double quotes ("""). Our style guide is silent on it, so your call.

mlir/lib/CAPI/ExecutionEngine/ExecutionEngine.cpp
26

"Standalone" out of tree dialects have quite a few issues with them being usable in practice. Things like npcomp that bring their own C-API, can provide their own entry point for registering their local translations. I wish we had a better story here, but this is far from the only thing that will have such problems.

mehdi_amini marked 6 inline comments as done.Mar 2 2021, 9:33 PM
mehdi_amini added inline comments.
mlir/lib/Bindings/Python/Jit.cpp
22

I guess I've always used "jit" intuitively so it fit better my mental model, but I should probably stick to "ExecutionEngine" for consistency with the rest of the project.

60

It isn't needed after this call.

I checked the header and it is called out for this function: "the module ownership stays with the client and can be destroyed as soon as the call returns."

62

Couldn't throw a std::exception as there is no constructor from a string literal, but we can throw a std::runtime_error error here :)

mlir/lib/Bindings/Python/mlir/jit.py
13

Consistency is important to me: I switched to triple quotes.

mehdi_amini marked 4 inline comments as done.

Address Stella's comments

ftynse added inline comments.Mar 3 2021, 12:11 AM
mlir/lib/CAPI/ExecutionEngine/ExecutionEngine.cpp
26

I suppose we can use a mechanism similar to dialect registration: in the C API, this is pushed down to the dialect that provides a registration function taking MlirContext. In Python, I would love to see something like import mlir.dialect.llvm.translation that calls the relevant function in its __init__.py. Not necessary for this commit.

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