-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[MLIR] Split ExecutionEngine Initialization out of ctor into an explicit method call #153373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 15 commits
41bf73a
f48b3f4
ee1a6dd
5426bf4
3807a17
88b9387
7ce99af
bf7a0fd
6122c0b
ca35dde
b72476c
fb0039c
1e3920c
d90bd29
28d752c
128bf5d
49a8ba0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,8 +7,8 @@ | |
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #include "mlir-c/ExecutionEngine.h" | ||
| #include "mlir/Bindings/Python/NanobindAdaptors.h" | ||
| #include "mlir/Bindings/Python/Nanobind.h" | ||
| #include "mlir/Bindings/Python/NanobindAdaptors.h" | ||
|
|
||
| namespace nb = nanobind; | ||
| using namespace mlir; | ||
|
|
@@ -75,13 +75,13 @@ NB_MODULE(_mlirExecutionEngine, m) { | |
| "__init__", | ||
| [](PyExecutionEngine &self, MlirModule module, int optLevel, | ||
| const std::vector<std::string> &sharedLibPaths, | ||
| bool enableObjectDump) { | ||
| bool enableObjectDump, bool shouldInitialize) { | ||
| llvm::SmallVector<MlirStringRef, 4> libPaths; | ||
| for (const std::string &path : sharedLibPaths) | ||
| libPaths.push_back({path.c_str(), path.length()}); | ||
| MlirExecutionEngine executionEngine = | ||
| mlirExecutionEngineCreate(module, optLevel, libPaths.size(), | ||
| libPaths.data(), enableObjectDump); | ||
| MlirExecutionEngine executionEngine = mlirExecutionEngineCreate( | ||
| module, optLevel, libPaths.size(), libPaths.data(), | ||
| enableObjectDump, shouldInitialize); | ||
| if (mlirExecutionEngineIsNull(executionEngine)) | ||
| throw std::runtime_error( | ||
| "Failure while creating the ExecutionEngine."); | ||
|
|
@@ -90,6 +90,7 @@ NB_MODULE(_mlirExecutionEngine, m) { | |
| nb::arg("module"), nb::arg("opt_level") = 2, | ||
| nb::arg("shared_libs") = nb::list(), | ||
| nb::arg("enable_object_dump") = true, | ||
| nb::arg("should_initialize") = true, | ||
| "Create a new ExecutionEngine instance for the given Module. The " | ||
| "module must contain only dialects that can be translated to LLVM. " | ||
| "Perform transformations and code generation at the optimization " | ||
|
|
@@ -124,6 +125,12 @@ NB_MODULE(_mlirExecutionEngine, m) { | |
| }, | ||
| nb::arg("name"), nb::arg("callback"), | ||
| "Register `callback` as the runtime symbol `name`.") | ||
| .def( | ||
| "initialize", | ||
| [](PyExecutionEngine &executionEngine) { | ||
| mlirExecutionEngineInitialize(executionEngine.get()); | ||
| }, | ||
| "Initialize the ExecutionEngine.") | ||
|
||
| .def( | ||
| "dump_to_object_file", | ||
| [](PyExecutionEngine &executionEngine, const std::string &fileName) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we need a flag for this, seems to me that since we introduce a separate
initialize()method we don't need an option and users can just callinitialize()separately.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is introduced to reduce the changes downstream has to make.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not the usual criteria of development in LLVM though: we aim to keep the code here maintainable and preserve our ability to innovate. Downstream know they need to adapt when we find design issues we fix upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we lazily initialize() when needed if the user hasn't called initialize()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. The
shouldInitializeflag is removed. InsideinvokePackedweinitialize()(noop if already initialized).