[PATCH] New JIT APIs (ORC)

[Eric Christopher]

Hi Lang,

Some commentary on the patch. I've already looked at prior incarnations so
pretty minimal. Also, just to reiterate, this is great. Thank you for doing
this work.


> There are no tests yet. I hope to start addressing this soon, but my
> preference would be to get this in tree so everyone can muck around with it
> and contribute tests.
>
>
Any hope of the "use orc under the covers patch" so that the existing JIT
tests can be repurposed?

Few questions:
You have a directory called Orc and files/libraries called OrcJIT. Probably
move the directory to OrcJIT or change the other stuff to just be Orc?
Timing on the module splitting? Just curious what the overhead here is
looking like.
Need to have all of the headers in include?
Some function names are in StudlyCaps, others in camelCase?
C API - Did we want to define a new one here?
lookupSymbolAddressIn - some docs on why we'd need to do this in a module
set would be good. :)
explode - Since it doesn't modify the existing module we'd have duplicate
copies of IR functions here? i.e. we've indirected foo and and then
exploded foo, but bar was never indirected and we might want to JIT bar at
some point, but then we've got foo in the module as well?
Handling compilation on another thread - any ideas for making that part of
this and thread safe? :)

Otherwise I think you should feel free to commit and handle most of this
stuff after - use your discretion.

Patch commentary below:

+template <typename BaseLayerT> class CompileOnDemandLayer {

Some documentation on what BaseLayerT should be here would be nice. Means
that someone wanting to use the interface doesn't have to go hunting.

+    uint64_t lookup(LMHandle LMH, const std::string &Name) {

It's fairly obvious that you're looking for an address here, but I wonder
if a typename might make it more clear? Also, probably want to use a
StringRef in occasions like this?

+    uint64_t lookupOnlyIn(LMHandle LMH, const std::string &Name) {

Block comment for this?

+    // Create a symbol lookup context & ModuleSetInfo for this module set.

Really nitpicky, but can you spell out "and" here? Grep looking for
references is painful :)

+    auto DylibLookup = std::make_shared<CODScopedLookup>(B);
+    ModuleSetHandleT H =
+        ModuleSetInfos.insert(ModuleSetInfos.end(),
ModuleSetInfo(DylibLookup));

Should ModuleSetInfo own the lookup? I don't see a reason why it couldn't
and then everything else have a reference, but I might have missed
something.

+      // Insert callback asm code into the first module.
+      insertX86CallbackAsm(*ExplodedModules[0],
+                           *MSI.JITResolveCallbackHandlers.back());

Add a fixme please? :) You know why.

+        BaseLayerModuleSetHandleT H = B.addModuleSet(
+            std::move(MSet),
+            createLookasideRTDyldMM<SectionMemoryManager>(
+                [=](const std::string &Name) {
+                  if (uint64_t Addr = DylibLookup->lookup(LMH, Name))
+                    return Addr;
+                  return getSymbolAddress(Name, true);
+                },
+                [=](const std::string &Name) {
+                  return DylibLookup->lookup(LMH, Name);
+                }));

Comment this please?

+  ModuleSetInfoListT ModuleSetInfos;
+  BaseLayerT &B;

Document :) Probably the rest of the class variables throughout too.

+      return {std::move(*Obj), std::move(ObjBuffer)};

Apparently this doesn't work in MSVC :)

+  /// @brief Compile the each module in the given module set, then then
add the

Grammar ;)

+///   This class provides name mangling that can outlive a Module (and its
+/// DataLayout).
+class PersistentMangler {

FWIW it might be nice to have the mangler split out from DataLayout in some
way some day.

+  //   Force a look up one of the original functions that has been
indirected.

Grammar.

+    uint64_t Search(StringRef Name, bool ExportedSymbolsOnly, BaseLayerT
&B) {

Document. :)

+      if (EmitState)

Should check against an enum value yes?

+    // Build the MangledNames map. Bails out early (with MangledNames set
to
+    // nullptr) if the given SearchName is found while building the map.
+    void buildMangledNames(StringRef SearchName,

This seems odd, can you explain it for me? I'm probably missing something
simple but I'm having an issue figuring out the early exit bit and how this
works in general.

+    // If not found then search the deffered sets. The call to 'Search'
will

"deferred"?

+  template <typename... CtorArgTs>

Do we get template varargs yet?

+  uint64_t getSymbolAddressInLogicalDylib(const std::string &Name)
override {

Comment? In particular for the term "logical" :)

+                     HandleFunctionFtor HandleFunction, bool
KeepInlineAsm) {

"KeepInlineAsm" Any reason we wouldn't want to?

+      llvm::make_unique<Module>(M.getModuleIdentifier(), M.getContext());

Keeping the same identifier?

+namespace {
+
+const char *JITCallbackFuncName = "call_jit_for_lazy_compile";
+const char *JITCallbackIndexLabelPrefix = "jit_resolve_";
+
+std::array<const char *, 12> X86GPRsToSave = {{

Might want to throw the X86 bits into its own namespace?

Thanks!

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150120/57f80e09/attachment.html>