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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini 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,
----------------
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?


================
Comment at: mlir/lib/ExecutionEngine/ExecutionEngine.cpp:297
+    mangledSymbolMap[Mangle(kv.first)] = std::move(kv.second);
+  }
+  cantFail(mainJD.define(absoluteSymbols(mangledSymbolMap)));
----------------
Nit: no trivial brace please.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79812