[PATCH] New JIT APIs (ORC)

[Pete Cooper]

> On Jan 20, 2015, at 2:33 PM, Lang Hames <lhames at gmail.com> wrote:
> 
> 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.
Yeah, agreed.  Although changing MCJIT may eventually get you back to the C interface which will make this tricky.  I guess users via the C interface might just have to live with uint64_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.
> 
> 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).
Yeah, I can see this being tricky.  Given that you std::move everything, I don’t think there’s much overhead here.  I’d like to work out the ArrayRef<std::unique_ptr> situation some time as I think it’ll be useful elsewhere in the compiler.
> 
> 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).
Ah, makes sense.  Looks like we’re actually abusing the name ‘DataLayout' by having it contain stuff like getGlobalPrefix(), but thats not something you need to solve 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}.
> 
> 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.
Sounds good.
>  
> +      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. 
Cool.  Can’t wait to see this in tree.

Thanks,
Pete
> 
> Cheers,
> Lang. 

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