<div dir="ltr">Thanks very much for the review Pete!<div><br></div><div>All the cleanup suggestions are worthwhile. I'll make them in the patch before I commit it.</div><div><br></div><div>Addressing the non-cosmetic comments:</div><div><br><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>
<br>
Eventually this would be great as a TargetPtrT or something like that, instead of int64_t.<br></blockquote><div><br></div><div>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><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></blockquote><div><br></div><div>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><br></div><div>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> </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>
+ const char *JITImplSuffix = "$mcjit_impl”;<br>
<br>
mcjit -> orc<br></blockquote><div><br></div><div>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> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ // Insert callback asm code into the first module.<br>
+ insertX86CallbackAsm(*ExplodedModules[0],<br>
+ *MSI.JITResolveCallbackHandlers.back());<br>
+<br>
+ for (auto &M : ExplodedModules) {<br>
+ std::vector<std::unique_ptr<Module>> MSet;<br>
+ MSet.push_back(std::move(M));<br>
+<br>
+ BaseLayerModuleSetHandleT H = B.addModuleSet(<br>
+ std::move(MSet),<br>
<br>
This loop needs a comment.<br></blockquote><div><br></div><div>Agreed. I'll add one.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+class PersistentMangler {<br>
+public:<br>
+ PersistentMangler(DataLayout DL) : DL(std::move(DL)), M(&this->DL) {}<br>
<br>
Now that the module owns DL, does the lifetime of the module mean you can avoid holding your own copy of DL here?<br></blockquote><div><br></div><div>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><br></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>
<br>
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></blockquote><div><br></div><div>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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (EmitState)<br>
+ BaseLayer.removeModuleSet(Handle);<br>
<br>
You should explicitly do ‘EmitState != NotEmitted’ or initialize NotEmitted to 0 in the enum.<br>
<br></blockquote><div>Oh yeah - that's a bug. Thanks!</div><div><br></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></blockquote><div><br></div><div>Great. Thanks again Pete!</div><div><br></div><div>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><br></div><div>Cheers,</div><div>Lang. </div></div></div></div></div>