<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jan 20, 2015, at 2:33 PM, Lang Hames <<a href="mailto:lhames@gmail.com" class="">lhames@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Thanks very much for the review Pete!<div class=""><br class=""></div><div class="">All the cleanup suggestions are worthwhile. I'll make them in the patch before I commit it.</div><div class=""><br class=""></div><div class="">Addressing the non-cosmetic comments:</div><div class=""><br class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ uint64_t lookup(LMHandle LMH, const std::string &Name) {<br class="">
<br class="">
Eventually this would be great as a TargetPtrT or something like that, instead of int64_t.<br class=""></blockquote><div class=""><br class=""></div><div class="">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.</div></div></div></div></div></div></blockquote>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.<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br class=""></blockquote><div class=""><br class=""></div><div class="">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).</div></div></div></div></div></div></blockquote>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.<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><div class="">I'm inclined to commit things as they stand, and we can start pushing ArrayRef and StringRef through the APIs where they make sense. </div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ const char *JITAddrSuffix = "$mcjit_addr";<br class="">
+ const char *JITImplSuffix = "$mcjit_impl”;<br class="">
<br class="">
mcjit -> orc<br class=""></blockquote><div class=""><br class=""></div><div class="">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! :)</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br class="">
+ // Insert callback asm code into the first module.<br class="">
+ insertX86CallbackAsm(*ExplodedModules[0],<br class="">
+ *MSI.JITResolveCallbackHandlers.back());<br class="">
+<br class="">
+ for (auto &M : ExplodedModules) {<br class="">
+ std::vector<std::unique_ptr<Module>> MSet;<br class="">
+ MSet.push_back(std::move(M));<br class="">
+<br class="">
+ BaseLayerModuleSetHandleT H = B.addModuleSet(<br class="">
+ std::move(MSet),<br class="">
<br class="">
This loop needs a comment.<br class=""></blockquote><div class=""><br class=""></div><div class="">Agreed. I'll add one.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+class PersistentMangler {<br class="">
+public:<br class="">
+ PersistentMangler(DataLayout DL) : DL(std::move(DL)), M(&this->DL) {}<br class="">
<br class="">
Now that the module owns DL, does the lifetime of the module mean you can avoid holding your own copy of DL here?<br class=""></blockquote><div class=""><br class=""></div><div class="">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).</div></div></div></div></div></div></blockquote>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.<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+void insertX86CallbackAsm(Module &M, JITResolveCallbackHandler &J);<br class="">
<br class="">
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}.<br class=""></blockquote><div class=""><br class=""></div><div class="">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.</div></div></div></div></div></div></blockquote>Sounds good.<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (EmitState)<br class="">
+ BaseLayer.removeModuleSet(Handle);<br class="">
<br class="">
You should explicitly do ‘EmitState != NotEmitted’ or initialize NotEmitted to 0 in the enum.<br class="">
<br class=""></blockquote><div class="">Oh yeah - that's a bug. Thanks!</div><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Overall I think these are minor changes that can be done post-commit. LGTM.<br class=""></blockquote><div class=""><br class=""></div><div class="">Great. Thanks again Pete!</div><div class=""><br class=""></div><div class="">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. </div></div></div></div></div></div></blockquote>Cool. Can’t wait to see this in tree.</div><div><br class=""></div><div>Thanks,</div><div>Pete<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><div class="">Cheers,</div><div class="">Lang. </div></div></div></div></div>
</div></blockquote></div><br class=""></body></html>