<div dir="ltr">Hi Guys,<div><br></div><div>I've committed this patch with a number of the suggested alterations in r226940. I'm anticipating fallout when the builders get to this - I'll try to be quick with reverts/fixes as they're needed.</div><div><br></div><div>Thanks again for all the feedback. Where I've overlooked anything please feel free to suggest/make alterations. The obvious missing piece right now is testing: I'm currently cleaning up the execution engine regression tests and plan to commit some orc tests shortly. Orc's modularity should also make it a good candidate for unit-testing. If anyone wants to get the ball rolling on that please feel free, otherwise I'll get some tests written as soon as my schedule permits. I'll also try to knock up a TODO list - I've got a long mental list of ways we can improve and build on this infrastructure.</div><div><br></div><div>Cheers,</div><div>Lang.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 22, 2015 at 4:44 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Justin,<div><br><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The constructors are templated, but the member hardcodes 4096 so the<br>
only valid argument to the templates is 4096. Either template the type<br>
or un-template the constructors?<br></blockquote><div><br></div></span><div>Huh. I had missed that we couldn't even move-construct from other SmallVectors where the size differed.</div><div><br></div><div>I've submitted a patch to llvm-commits to add the necessary operations to SmallVector so that I can just pass a SmallVectorImpl<T>&&.</div><span class=""><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
> + PersistentMangler(DataLayout DL) : DL(std::move(DL)), M(&this->DL) {}<br>
<br>
</span>Shouldn't DL be an rvalue reference here?<br></blockquote><div><br></div></span><div>I usually just use pass-by-value where I can. You do end up doubling the number of move operations for clients who want to move, but that should still be cheap.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> +++ include/llvm/ExecutionEngine/Orc/LazyEmittingLayer.h (working copy)<br>
...<br>
<span>> + // Build the MangledNames map. Bails out early (with MangledNames set to<br>
> + // nullptr) if the given SearchName is found while building the map.<br>
> + void buildMangledNames(StringRef SearchName,<br></span><br></span><span class="">
This actually leaves MangledNames untouched if it bails out early. I<br>
don't know that it matters much which behaviour we choose, but please<br>
make the comment and the code match.<br></span></blockquote><div><br></div><div>Comment fixed. Thanks!</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +++ lib/ExecutionEngine/Orc/IndirectionUtils.cpp (working copy)<br>
...<br>
> +std::vector<std::unique_ptr<Module>><br>
> +explode(const Module &OrigMod, const JITIndirections &Indirections) {<br>
> + std::set<std::string> ImplNames;<br>
> +<br>
> + for (const auto &FuncName : Indirections.IndirectedNames)<br>
> + ImplNames.insert(Indirections.GetImplName(FuncName));<br>
> +<br>
> + return explode(<br>
> + OrigMod, [&](const Function &F) { return ImplNames.count(F.getName()); });<br>
<br>
It's better to sort a vector and binary search here, rather than use a<br>
std::set.<br>
</blockquote></span></div><br></div></div><div class="gmail_extra">Good point. I'm going to leave this as is for now though: explode is probably going to get completely rewritten soon.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Thanks for the review Justin!</div><span class="HOEnZb"><font color="#888888"><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">- Lang.</div></font></span></div>
</blockquote></div><br></div>