[PATCH] New JIT APIs (ORC)

[Lang Hames]

Thanks very much for the review Pete!

All the cleanup suggestions are worthwhile. I'll make them in the patch
before I commit it.

Addressing the non-cosmetic comments:

+    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.
>

Agreed. Arguably we should do this for all of MCJIT and RuntimeDyld too.
I'd prefer to make this change after the patch is committed though.


> 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.
>

I think I've missed a few opportunities to ArrayRef and StringRef-ize, but
in a number of places this actually comes down to memory ownership
requirements: We have to be *very* careful about keeping StringRefs to
symbol names in the JIT, since one of the intents with the new APIs is that
Modules can be free'd as soon as they've been CodeGen'd (unless the user
wants to keep them alive longer).

I'm inclined to commit things as they stand, and we can start pushing
ArrayRef and StringRef through the APIs where they make sense.


> +    const char *JITAddrSuffix = "$mcjit_addr";
> +    const char *JITImplSuffix = "$mcjit_impl”;
>
> mcjit -> orc
>

Hah. This is some of the oldest code in the patch. I forgot to fix it up
when I was renaming everything. Thanks for the catch! :)


> +
> +      // 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.
>

Agreed. I'll add one.


> +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?
>

Name mangling in Orc is non-trivial because code can outlive its Module. We
need to keep the DataLayout around for that reason. (Incidentally, I think
this is probably a problem for some advanced MCJIT use cases too, though
nobody has complained about it, probably because asm mangling is a no-op on
ELF).

+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}.
>

I expect the callback asm will always be target specific, though it's a
problem that CompileOnDemand is currently hardcoded to X86. I'll make it a
std::function argument to CompileOnDemand's constructor.


> +      if (EmitState)
> +        BaseLayer.removeModuleSet(Handle);
>
> You should explicitly do ‘EmitState != NotEmitted’ or initialize
> NotEmitted to 0 in the enum.
>
> Oh yeah - that's a bug. Thanks!

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

Great. Thanks again Pete!

I'm going to hold off committing - A few other people are still reviewing
the patch, but assuming they're generally ok with this too I'll fix all
this stuff once the patch is in tree.

Cheers,
Lang.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150120/e0811a6d/attachment.html>