[PATCH] D79812: [MLIR] Add symbol map to mlir ExecutionEngine

[Eugene Zhulenev via Phabricator via llvm-commits]

ezhulenev marked 3 inline comments as done.
ezhulenev added inline comments.


================
Comment at: mlir/lib/ExecutionEngine/ExecutionEngine.cpp:199
     Optional<llvm::CodeGenOpt::Level> jitCodeGenOptLevel,
-    ArrayRef<StringRef> sharedLibPaths, bool enableObjectCache,
-    bool enableGDBNotificationListener, bool enablePerfNotificationListener) {
+    ArrayRef<StringRef> sharedLibPaths, SymbolMap symbolMap,
+    bool enableObjectCache, bool enableGDBNotificationListener,
----------------
ftynse wrote:
> mehdi_amini wrote:
> > ezhulenev wrote:
> > > mehdi_amini wrote:
> > > > ezhulenev wrote:
> > > > > mehdi_amini wrote:
> > > > > > mehdi_amini wrote:
> > > > > > > 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?
> > > > > > Also: isn't something can can be done after creation? With a specific API to add symbols?
> > > > > 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".
> > > > 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.
> > > 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.
> > 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)
> 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).
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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79812/new/

https://reviews.llvm.org/D79812