Hi Lang,<div><br></div><div>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.</div><div><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>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.</div><div><br></div></div></blockquote><div><br></div><div>Any hope of the "use orc under the covers patch" so that the existing JIT tests can be repurposed?</div><div><br></div><div>Few questions:</div><div>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?</div><div>Timing on the module splitting? Just curious what the overhead here is looking like.</div><div>Need to have all of the headers in include?</div><div>Some function names are in StudlyCaps, others in camelCase?</div><div>C API - Did we want to define a new one here?</div><div>lookupSymbolAddressIn - some docs on why we'd need to do this in a module set would be good. :)</div><div>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?</div><div>Handling compilation on another thread - any ideas for making that part of this and thread safe? :)</div><div><br></div><div>Otherwise I think you should feel free to commit and handle most of this stuff after - use your discretion.</div><div><br></div><div>Patch commentary below:</div><div><br></div><div>+template <typename BaseLayerT> class CompileOnDemandLayer {<br></div><div><br></div><div>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.</div><div><br></div><div>+    uint64_t lookup(LMHandle LMH, const std::string &Name) {<br></div><div><br></div><div>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?</div><div><br></div><div>+    uint64_t lookupOnlyIn(LMHandle LMH, const std::string &Name) {<br></div><div><br></div><div>Block comment for this?</div><div><br></div><div>+    // Create a symbol lookup context & ModuleSetInfo for this module set.<br></div><div><br></div><div>Really nitpicky, but can you spell out "and" here? Grep looking for references is painful :)</div><div><br></div><div><div>+    auto DylibLookup = std::make_shared<CODScopedLookup>(B);</div><div>+    ModuleSetHandleT H =</div><div>+        ModuleSetInfos.insert(ModuleSetInfos.end(), ModuleSetInfo(DylibLookup));</div></div><div><br></div><div>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.</div><div><br></div><div><div>+      // Insert callback asm code into the first module.</div><div>+      insertX86CallbackAsm(*ExplodedModules[0],</div><div>+                           *MSI.JITResolveCallbackHandlers.back());</div></div><div><br></div><div>Add a fixme please? :) You know why.</div><div><br></div><div><div>+        BaseLayerModuleSetHandleT H = B.addModuleSet(</div><div>+            std::move(MSet),</div><div>+            createLookasideRTDyldMM<SectionMemoryManager>(</div><div>+                [=](const std::string &Name) {</div><div>+                  if (uint64_t Addr = DylibLookup->lookup(LMH, Name))</div><div>+                    return Addr;</div><div>+                  return getSymbolAddress(Name, true);</div><div>+                },</div><div>+                [=](const std::string &Name) {</div><div>+                  return DylibLookup->lookup(LMH, Name);</div><div>+                }));</div></div><div><br></div><div>Comment this please?</div><div><br></div><div><div>+  ModuleSetInfoListT ModuleSetInfos;</div><div>+  BaseLayerT &B;</div></div><div><br></div><div>Document :) Probably the rest of the class variables throughout too.</div><div><br></div><div>+      return {std::move(*Obj), std::move(ObjBuffer)};<br></div><div><br></div><div>Apparently this doesn't work in MSVC :)</div><div><br></div><div>+  /// @brief Compile the each module in the given module set, then then add the<br></div><div><br></div><div>Grammar ;)</div><div><br></div><div><div>+///   This class provides name mangling that can outlive a Module (and its</div><div>+/// DataLayout).</div><div>+class PersistentMangler {</div></div><div><br></div><div>FWIW it might be nice to have the mangler split out from DataLayout in some way some day.</div><div><br></div><div>+  //   Force a look up one of the original functions that has been indirected.<br></div><div><br></div><div>Grammar.</div><div><br></div><div>+    uint64_t Search(StringRef Name, bool ExportedSymbolsOnly, BaseLayerT &B) {<br></div><div><br></div><div>Document. :)</div><div><br></div><div>+      if (EmitState)<br></div><div><br></div><div>Should check against an enum value yes?</div><div><br></div><div><div>+    // Build the MangledNames map. Bails out early (with MangledNames set to</div><div>+    // nullptr) if the given SearchName is found while building the map.</div><div>+    void buildMangledNames(StringRef SearchName,</div></div><div><br></div><div>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.</div><div><br></div><div>+    // If not found then search the deffered sets. The call to 'Search' will<br></div><div><br></div><div>"deferred"?</div><div><br></div><div>+  template <typename... CtorArgTs><br></div><div><br></div><div>Do we get template varargs yet?</div><div><br></div><div>+  uint64_t getSymbolAddressInLogicalDylib(const std::string &Name) override {<br></div><div><br></div><div>Comment? In particular for the term "logical" :)</div><div><br></div><div>+                     HandleFunctionFtor HandleFunction, bool KeepInlineAsm) {<br></div><div><br></div><div>"KeepInlineAsm" Any reason we wouldn't want to?</div><div><br></div><div>+      llvm::make_unique<Module>(M.getModuleIdentifier(), M.getContext());<br></div><div><br></div><div>Keeping the same identifier?</div><div><br></div><div><div>+namespace {</div><div>+</div><div>+const char *JITCallbackFuncName = "call_jit_for_lazy_compile";</div><div>+const char *JITCallbackIndexLabelPrefix = "jit_resolve_";</div><div>+</div><div>+std::array<const char *, 12> X86GPRsToSave = {{</div></div><div><br></div><div>Might want to throw the X86 bits into its own namespace?</div><div><br></div><div>Thanks!</div><div><br></div><div>-eric</div></div></div>