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
61 ↗(On Diff #327391)

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

66 ↗(On Diff #327391)

Nit: dialects

mlir/lib/Bindings/Python/Jit.h
1 ↗(On Diff #327391)

Wrong header

mlir/lib/Bindings/Python/mlir/jit.py
11 ↗(On Diff #327391)

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
37 ↗(On Diff #327391)

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
61 ↗(On Diff #327391)

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
61 ↗(On Diff #327391)

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)

21 ↗(On Diff #327527)

Stale comment: ... around an MlirExecutionEngine

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

59 ↗(On Diff #327527)

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.

65 ↗(On Diff #327527)

... must contain only ...

mlir/lib/Bindings/Python/mlir/jit.py
12 ↗(On Diff #327527)

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
61 ↗(On Diff #327391)

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 :)

21 ↗(On Diff #327527)

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.

59 ↗(On Diff #327527)

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."

mlir/lib/Bindings/Python/mlir/jit.py
12 ↗(On Diff #327527)

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.