[PATCH] New JIT APIs (ORC)

[Lang Hames]

Hi Guys,

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.

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.

Cheers,
Lang.


On Thu, Jan 22, 2015 at 4:44 PM, Lang Hames <lhames at gmail.com> wrote:

> Hi Justin,
>
> The constructors are templated, but the member hardcodes 4096 so the
>> only valid argument to the templates is 4096. Either template the type
>> or un-template the constructors?
>>
>
> Huh. I had missed that we couldn't even move-construct from other
> SmallVectors where the size differed.
>
> I've submitted a patch to llvm-commits to add the necessary operations to
> SmallVector so that I can just pass a SmallVectorImpl<T>&&.
>
>
>
>> > +  PersistentMangler(DataLayout DL) : DL(std::move(DL)), M(&this->DL) {}
>>
>> Shouldn't DL be an rvalue reference here?
>>
>
> 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.
>
>
>> > +++ include/llvm/ExecutionEngine/Orc/LazyEmittingLayer.h      (working
>> copy)
>>  ...
>> > +    // Build the MangledNames map. Bails out early (with MangledNames
>> set to
>> > +    // nullptr) if the given SearchName is found while building the
>> map.
>> > +    void buildMangledNames(StringRef SearchName,
>>
>> This actually leaves MangledNames untouched if it bails out early. I
>> don't know that it matters much which behaviour we choose, but please
>> make the comment and the code match.
>>
>
> Comment fixed. Thanks!
>
>
>> > +++ lib/ExecutionEngine/Orc/IndirectionUtils.cpp      (working copy)
>>  ...
>> > +std::vector<std::unique_ptr<Module>>
>> > +explode(const Module &OrigMod, const JITIndirections &Indirections) {
>> > +  std::set<std::string> ImplNames;
>> > +
>> > +  for (const auto &FuncName : Indirections.IndirectedNames)
>> > +    ImplNames.insert(Indirections.GetImplName(FuncName));
>> > +
>> > +  return explode(
>> > +      OrigMod, [&](const Function &F) { return
>> ImplNames.count(F.getName()); });
>>
>> It's better to sort a vector and binary search here, rather than use a
>> std::set.
>>
>
> Good point. I'm going to leave this as is for now though: explode is
> probably going to get completely rewritten soon.
>
> Thanks for the review Justin!
>
>
> - Lang.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150123/e1f916c7/attachment.html>