Page MenuHomePhabricator

[MLIR] Add symbol map to mlir ExecutionEngine
ClosedPublic

Authored by ezhulenev on Tue, May 12, 3:13 PM.

Details

Summary

Add additional symbol mapping to be able to provide custom symbols to jitted code at runtime.

Diff Detail

Event Timeline

ezhulenev created this revision.Tue, May 12, 3:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, May 12, 3:13 PM
mehdi_amini added inline comments.Tue, May 12, 7:33 PM
mlir/lib/ExecutionEngine/ExecutionEngine.cpp
210

Copying an entire DenseMap just to read it isn't ideal. Can we pass is as const ref?
Or better allow the client of the API to not be forced into this narrow contract by using a callback to populate the internal llvm::orc::SymbolMap or something like this?

306

Nit: no trivial brace please.

mehdi_amini added inline comments.Tue, May 12, 7:34 PM
mlir/lib/ExecutionEngine/ExecutionEngine.cpp
210

Also: isn't something can can be done after creation? With a specific API to add symbols?

ezhulenev updated this revision to Diff 263600.Tue, May 12, 9:14 PM

Pass symbolMap as a std::function callback.s

ezhulenev updated this revision to Diff 263602.Tue, May 12, 9:17 PM
ezhulenev marked 4 inline comments as done.

Fix typo.

mlir/lib/ExecutionEngine/ExecutionEngine.cpp
210

Done. I changed DenseMap to std::function callback. In theory it can be done later, after the creation of ExecutionEngine, though I think it makes sense to pass these details at the time of "create".

mehdi_amini added inline comments.Tue, May 12, 9:47 PM
mlir/lib/ExecutionEngine/ExecutionEngine.cpp
210

Nit: please use function_ref for callbacks when you don't keep a ref to it.

I think it makes sense to pass these details at the time of "create".

Why? Seems like the kind of thing that makes more sense to me to add "as needed" instead of forcing it on the creation if there is no reason for it.
It also makes the creation API simpler.

Harbormaster completed remote builds in B56536: Diff 263600.

Use llvm::function_ref instead of std::function.

ezhulenev marked 2 inline comments as done.Tue, May 12, 11:24 PM
ezhulenev added inline comments.
mlir/lib/ExecutionEngine/ExecutionEngine.cpp
210

Changed function parameters to llvm::function_ref.

It has kind of the same purpose as sharedLibPaths (register symbols), and that's why it feels right to keep them next to each other, but I agree that create signature grows too big, and something has to be done. It doesn't make sense to me to make this change just for the symbol map, it has to be a new creation API with options struct, or builder, or something else.

mehdi_amini added inline comments.Tue, May 12, 11:45 PM
mlir/lib/ExecutionEngine/ExecutionEngine.cpp
210

Right but if we agree that a refactoring of this API would be better, I rather see this *before* adding more to it (incidentally it'll reduce the churn on the users of the API)

ftynse added inline comments.Wed, May 13, 1:38 AM
mlir/lib/ExecutionEngine/ExecutionEngine.cpp
210

Alternatively, we can have an instance method ExecutionEngine::registerSymbols that can be called when necessary without touching the ExecutionEngine::create interface (it should be updated regardless, but this patch will be unblocked).

ezhulenev marked an inline comment as done.

Add registerSymbols() function to mlir ExecutionEngine.

ezhulenev marked 3 inline comments as done.Wed, May 13, 12:04 PM
ezhulenev added inline comments.
mlir/lib/ExecutionEngine/ExecutionEngine.cpp
210

I've added registerSymbols, not to do a full builder-style rewrite in this patch, because it's the one thing that does not have to be a part of create.

ftynse accepted this revision.Wed, May 13, 2:17 PM
This revision is now accepted and ready to land.Wed, May 13, 2:17 PM
ezhulenev marked an inline comment as done.Wed, May 13, 4:08 PM
mehdi_amini accepted this revision.Wed, May 13, 5:23 PM

LG, thanks!

mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
115

Nit comment should read "with this ExecutionEngine", "an" is a bit undefined / seems to refer to another object.

ezhulenev updated this revision to Diff 263898.Wed, May 13, 5:36 PM

Update comment.

ezhulenev marked an inline comment as done.Wed, May 13, 5:36 PM
This revision was automatically updated to reflect the committed changes.