[PATCH] New JIT APIs (ORC)

[Pete Cooper]

Hey Lang

This is looking great.  The structure and comments are extremely good.

Mostly some nitpicks.  Feel free to do most of this post-commit, or even land the patch and i’ll help with some of it.

+  /// setUseOrcJIT - Use the OrcJIT instead of MCJIT. Off by default.

We use \brief instead of *name* - 

+//===--- ObjectBuffer.h - Utility class to wrap object memory ---*- C++ -*-===//

Wrong filename

+
+/// @brief Compile-on-demand layer.

I think its \brief.  Hopefully this doesn’t kick off a discussion over which one is better.

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

Eventually this would be great as a TargetPtrT or something like that, instead of int64_t.

Also, you’ve used std::string and std::vector in many places such as the one above.  I’m not sure where, but we should use ArrayReg and StringRef in as many as possible.  

Saying that, i’m not sure you could std::move from string -> StringRef -> string, whereas you could std::move from string -> string.  So perhaps improvements need to be made to StringRef for this to work as you need.

+    const char *JITAddrSuffix = "$mcjit_addr";
+    const char *JITImplSuffix = "$mcjit_impl”;

mcjit -> orc

+
+      // Insert callback asm code into the first module.
+      insertX86CallbackAsm(*ExplodedModules[0],
+                           *MSI.JITResolveCallbackHandlers.back());
+
+      for (auto &M : ExplodedModules) {
+        std::vector<std::unique_ptr<Module>> MSet;
+        MSet.push_back(std::move(M));
+
+        BaseLayerModuleSetHandleT H = B.addModuleSet(
+            std::move(MSet),

This loop needs a comment.

Also, perhaps a TODO to say that we should try std::move(M) on the last line instead of going through a vector.  We may need to improve ArrayRef to handle this case, but that can be done post-commit.

+  BaseLayerT &B;

Later you name it BaseLayer instead of B.  I think BaseLayer is better.

+class PersistentMangler {
+public:
+  PersistentMangler(DataLayout DL) : DL(std::move(DL)), M(&this->DL) {}

Now that the module owns DL, does the lifetime of the module mean you can avoid holding your own copy of DL here?

+void insertX86CallbackAsm(Module &M, JITResolveCallbackHandler &J);

We should make this method be target agnostic.  Also the implementation later is in a file which has X86 code but not X86 in its name - OrcTargetSupport.{h,cpp}.

+    uint64_t Search(StringRef Name, bool ExportedSymbolsOnly, BaseLayerT &B) {
+      if (EmitState == NotEmitted) {
+        if (Provides(Name, ExportedSymbolsOnly)) {
+          EmitState = Emitting;
+          Handle = Emit(B);
+          EmitState = Emitted;
+        } else
+          return 0;
+      } else if (EmitState == Emitting) {
+        // The module has been added to the base layer but we haven't gotten a
+        // handle back yet so we can't use lookupSymbolAddressIn. Just return
+        // '0' here - LazyEmittingLayer::getSymbolAddress will do a global
+        // search in the base layer when it doesn't find the symbol here, so
+        // we'll find it in the end.
+        return 0;
+      }
+      return B.lookupSymbolAddressIn(Handle, Name, ExportedSymbolsOnly);
+    }

You could probably use a switch here, or reorder it a little to remove some indentation.  Getting the ‘return 0’ to remove indentation basically, or a fall through from NotEmitted to Emitted.

+      if (EmitState)
+        BaseLayer.removeModuleSet(Handle);

You should explicitly do ‘EmitState != NotEmitted’ or initialize NotEmitted to 0 in the enum.

+  class EmissionDeferredSetImpl : public EmissionDeferredSet {
...
+    virtual BaseLayerHandleT Emit(BaseLayerT &BaseLayer) {

You might be missing some override’s on the methods here.

+JITIndirections makeCallsDoubleIndirect(

Technically this makes all uses double indirect, not just calls.  But perhaps as calls are typical the name is fine.

+#ifndef LLVM_LIB_EXECUTIONENGINE_ORCJIT_MCJITREPLACEMENT_H

ORCJIT -> ORC

+  // Compute index, load object address, and call JIT.
+  JITCallbackAsm << "  movq    " << ReturnAddrOffset << "(%rsp), %rax\n”

You might need more of a comment here so that people can port it to other targets.

Overall I think these are minor changes that can be done post-commit. LGTM.

Thanks,
Pete
> On Jan 19, 2015, at 3:22 PM, Lang Hames <lhames at gmail.com> wrote:
> 
> Hi All,
> 
> Attached is an updated patch (against r226514) for the Orc JIT APIs that were discussed on the llvm-dev list the other day.
> 
> The impact on existing code is minimal. A new flag (-use-orc) is added to lli to allow the Orc-based MCJITReplacement class to be substituted as the execution engine. I expect this will be useful for testing. A new method, getSymbolAddressInLogicalDylib, is added to RTDyldMemoryManager, however this can be safely ignored by MCJIT users - it's only there to support Orc.
> 
> 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.
> 
> Questions and comments welcome.
> 
> Cheers,
> Lang.
> 
> <Orc-2015-01-19.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits