Page MenuHomePhabricator

[MLIR] Execution engine python binding support for shared libraries
ClosedPublic

Authored by bondhugula on Jun 9 2021, 10:44 PM.

Details

Summary

Add support to Python bindings for the MLIR execution engine to load a
specified list of shared libraries - for eg. to use MLIR runtime
utility libraries.

Diff Detail

Event Timeline

bondhugula created this revision.Jun 9 2021, 10:44 PM
bondhugula requested review of this revision.Jun 9 2021, 10:44 PM

Drop unnecessary import.

All else being equal, it is better to not be in the business of textually manipulating file paths with ad hoc separators. While a bit more typing, would you be open to changing the c API to take a size and pointer to an array of string views? Then on the python side, accept a list of strings.

I know it's annoying but I've been bitten by this kind of path munging on multiple occasions.

All else being equal, it is better to not be in the business of textually manipulating file paths with ad hoc separators. While a bit more typing, would you be open to changing the c API to take a size and pointer to an array of string views? Then on the python side, accept a list of strings.

I know it's annoying but I've been bitten by this kind of path munging on multiple occasions.

I was thinking about it but the <number, pointer to an array of strings> looked a bit clumsy to me. For eg., one could have an inconsistency b/w the two? (Btw, what did you mean by "string views" for a C API?) For the scope of this functionality (it's really Python bindings as opposed to the main thing which has to be far more robust), I felt rarely would the issue of commas in the names of runtime libraries would come up. So, this itself I felt would be sufficient?

I'm looking for a better way though to specify the path in the test case - comment below.

mlir/test/python/execution_engine.py
335

Comments welcome here on specifying this in a better way - for eg. using LIT's %mlir_runner_utils_dir in the run line and somehow passing that along here.

+1 to Stella's comment :)

(Btw, what did you mean by "string views" for a C API?)

https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir-c/Support.h#L52-L67

+1 to Stella's comment :)

(Btw, what did you mean by "string views" for a C API?)

https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir-c/Support.h#L52-L67

Oh, this is then different than what I originally understood from Stella's comment. Can you clarify what signature you are recommending? Is it this:

..., int numPaths, const MlirStringRef *libPaths)

or

..., MlirStringRef libPaths)

If it's the former, what happens if the user accidentally provides fewer paths in libPaths than numPaths? It's going to crash without an assertion and that would be a pretty bad dev experience. That was my question and I felt a single comma separated string (either as StringRef or const char *) is better.

I would have wrote: ..., int numPaths, const MlirStringRef *libPaths).

This is the kind of API that I expect the user to map to an ArrayRef on the other side: the C API is unsafe (by nature somehow...) but the chance for error should be almost none because this is only a "binding" API and safe constructs can be used to wrap around this.

I would have wrote: ..., int numPaths, const MlirStringRef *libPaths).

This is the kind of API that I expect the user to map to an ArrayRef on the other side: the C API is unsafe (by nature somehow...) but the chance for error should be almost none because this is only a "binding" API and safe constructs can be used to wrap around this.

Makes sense to me - this makes it cleaner at the call site both for C and Python. It's just a downside that you'd have a bad crash if there is a consistency b/w the count and the number of elements in the ref. The comma separated string avoids that by design and you never get an opaque crash in the C API. Note that the C API user won't typically have an ArrayRef or a higher level structure to map that to the C API. They are using C right? :-)

I would have wrote: ..., int numPaths, const MlirStringRef *libPaths).

This is the kind of API that I expect the user to map to an ArrayRef on the other side: the C API is unsafe (by nature somehow...) but the chance for error should be almost none because this is only a "binding" API and safe constructs can be used to wrap around this.

Makes sense to me - this makes it cleaner at the call site both for C and Python. It's just a downside that you'd have a bad crash if there is a consistency b/w the count and the number of elements in the ref. The comma separated string avoids that by design and you never get an opaque crash in the C API. Note that the C API user won't typically have an ArrayRef or a higher level structure to map that to the C API. They are using C right? :-)

It's kind of hard to armor a C API more than this (and stay portable, etc). This is far from the first place we have this counted array idiom.

mlir/test/python/execution_engine.py
335

I don't know that particular bit will enough to have a better approach off have. It seems like we should be anchoring it to some kind of absolute path that cmake provides. If it works as is in ok committing like this if you can't find a better way, and I'll clean it up if I see something better.

Switch to list of strings on Python API.

bondhugula marked an inline comment as done.Jun 11 2021, 1:30 AM

Switched to list of strings on Python API and <count, MlirStringRef array> on the CAPI.

mehdi_amini accepted this revision.Jun 11 2021, 9:00 AM

LGTM

Note that the C API user won't typically have an ArrayRef or a higher level structure to map that to the C API. They are using C right? :-)

Do we expect people to actually use C? For example you're using a c++ vector to call this API here :)

This revision is now accepted and ready to land.Jun 11 2021, 9:00 AM

Drop unnecessary include STLExtras.h