This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Fix and test python bindings for dump_to_object_file
ClosedPublic

Authored by shabalin on Oct 20 2022, 3:59 AM.

Diff Detail

Event Timeline

shabalin created this revision.Oct 20 2022, 3:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 3:59 AM
shabalin requested review of this revision.Oct 20 2022, 3:59 AM
ftynse added inline comments.Oct 20 2022, 4:02 AM
mlir/lib/CAPI/ExecutionEngine/ExecutionEngine.cpp
58

Are you sure there's no unintended effects due to this being always-on? I'd rather take the flag as argument and thread that to Python.

shabalin updated this revision to Diff 469183.Oct 20 2022, 5:25 AM

Make dump_to_object_file work even if no functions were invoked previously.

shabalin updated this revision to Diff 469185.Oct 20 2022, 5:32 AM

Make sure temporary file is always deleted, even if test fails.

ftynse added inline comments.Oct 20 2022, 5:34 AM
mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
49

Please add documentation.

mlir/lib/ExecutionEngine/ExecutionEngine.cpp
111

Nit: please expand auto. MLIR only uses auto if the type is clear from immediate context.

113–116

So this just suppresses all errors? This must be at least documented. Even better if we propagate failure to avoid the surprise for the caller when the dumped object is silently missing some functions that couldn't be compiled.

258

I'd rather do m->getOps<LLVM::FuncOp> or at least indicate that traversal _inside_ functions isn't necessary.

shabalin updated this revision to Diff 469191.Oct 20 2022, 5:47 AM

Address another round of comments.

shabalin updated this revision to Diff 469199.Oct 20 2022, 6:03 AM

Expose enableObjectDump toggle to python & c bindings.

shabalin updated this revision to Diff 469201.Oct 20 2022, 6:06 AM

Report failures on dump-triggered lazy compilation failures.

shabalin marked 5 inline comments as done.Oct 20 2022, 6:08 AM
ftynse accepted this revision.Oct 20 2022, 6:12 AM

LGTM with the last batch of comments addressed.

mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
206

I think we need to use owning strings here. There doesn't seem to be a guarantee that the owner of the function name (attribute of the MLIR operation, so the content is presumably owned by the MLIRContext) outlives the execution engine. It looks unlikely, but someone may decide to free up the memory after compilation and before execution.

mlir/lib/ExecutionEngine/ExecutionEngine.cpp
111

There's no need to have a reference to StringRef which itself a reference to a string.

This revision is now accepted and ready to land.Oct 20 2022, 6:12 AM
shabalin updated this revision to Diff 469205.Oct 20 2022, 6:23 AM

Address another round of review comments.

shabalin marked 2 inline comments as done.Oct 20 2022, 6:24 AM
This revision was landed with ongoing or failed builds.Oct 20 2022, 6:53 AM
This revision was automatically updated to reflect the committed changes.

This change has broken the Windows buildbot, in an unfortunately opaque way. https://lab.llvm.org/buildbot/#/builders/13/builds/27291

******************** TEST 'MLIR :: python/execution_engine.py' FAILED ********************
Script:
--
: 'RUN: at line 1';   "C:/Program Files/Python39/python.exe" C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\test\python\execution_engine.py 2>&1 | c:\buildbot\mlir-x64-windows-ninja\build\bin\filecheck.exe C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\test\python\execution_engine.py
--
Exit Code: 1
Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "C:/Program Files/Python39/python.exe" "C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\test\python\execution_engine.py"
note: command had no output on stdout or stderr
error: command failed with exit status: 1
$ "c:\buildbot\mlir-x64-windows-ninja\build\bin\filecheck.exe" "C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\test\python\execution_engine.py"
--
********************