[PATCH] D15390: [ThinLTO] Launch importing backends in parallel threads from gold plugin

[Teresa Johnson via llvm-commits]

Ping.

Thanks,
Teresa

On Tue, Feb 16, 2016 at 10:24 AM, Peter Collingbourne <peter at pcc.me.uk>
wrote:

> Hi Mehdi,
>
> Can we resolve this by stipulating that Teresa should implement in
> whichever
> way she finds easiest?
>
> Thanks,
> Peter
>
> On Tue, Feb 16, 2016 at 05:45:06PM +0000, Teresa Johnson wrote:
> > I can see both sides to this, and will implement either. Can we close on
> a
> > decision? I don't want to hold up the thinlto side much longer. Out of
> town
> > this week, but if we can get consensus so I can get this in when I'm
> back,
> > that would be great! Thanks, Teresa
> >
> > On Fri, Feb 12, 2016, 1:53 PM Peter Collingbourne <peter at pcc.me.uk>
> wrote:
> >
> > > On Fri, Feb 12, 2016 at 03:36:19PM -0800, Mehdi Amini wrote:
> > > >
> > > >
> > > > Sent from my iPhone
> > > >
> > > > > On Feb 12, 2016, at 3:09 PM, Peter Collingbourne <peter at pcc.me.uk>
> > > wrote:
> > > > >
> > > > >> On Fri, Feb 12, 2016 at 02:53:04PM -0800, Mehdi Amini wrote:
> > > > >>
> > > > >>
> > > > >> Sent from my iPhone
> > > > >>
> > > > >>> On Feb 12, 2016, at 2:22 PM, Peter Collingbourne <
> peter at pcc.me.uk>
> > > wrote:
> > > > >>>
> > > > >>> pcc added inline comments.
> > > > >>>
> > > > >>> ================
> > > > >>> Comment at: tools/gold/gold-plugin.cpp:1019
> > > > >>> @@ +1018,3 @@
> > > > >>> +    raw_svector_ostream BCOS(BCs.back());
> > > > >>> +    WriteBitcodeToFile(MPart.get(), BCOS);
> > > > >>> +  });
> > > > >>> ----------------
> > > > >>> joker.eph wrote:
> > > > >>>> joker.eph wrote:
> > > > >>>>> pcc wrote:
> > > > >>>>>> I don't think thread pools are necessary for split code gen,
> as
> > > we can already perfectly assign the right amount of work to individual
> > > threads. Also, this implementation loses the pipelining feature from
> the
> > > original code (i.e. worker threads can work on codegen'ing while the
> main
> > > thread is still splitting). I would prefer you to use the existing
> > > implementation in `llvm/CodeGen/ParallelCG.h`.
> > > > >>>>> The thread that is doing the splitting can issue other jobs to
> the
> > > thread pool, providing the desired pipeline.
> > > > >>>>>
> > > > >>>>> Just fuse the loop body below within the lambda...
> > > > >>>> I'll add that while the pooling is not necessary if you only
> queue
> > > as many jobs as you have threads, it is not a reason by itself not to
> use
> > > it: the paradigm is fairly clear, and it decouples the actual splitting
> > > granularity from the number of actual worker threads, allowing to
> > > experiment with different numbers for each (providing better
> pipelining for
> > > instance).
> > > > >>> Yes, but the current implementation doesn't need any of that. If
> we
> > > experimentally find that decoupling would provide some benefit, then
> by all
> > > means we can start using thread pools here.
> > > > >>>
> > > > >>> In any case, if there is a compelling reason to use thread pools,
> > > the right place to make the change is in `lib/CodeGen/ParallelCG.cpp`
> > > rather than in a duplicate implementation here. We can defer what the
> > > design for that should look like simply by not using thread pools yet.
> > > > >>>
> > > > >>
> > > > >> I don't understand why should we defer what is clearly identified
> as
> > > a decoupled implementation, for which you haven't articulate any
> drawbacks
> > > (unless I missed one point).
> > > > >
> > > > > The drawback is that it adds unnecessary complexity to the
> codebase.
> > > >
> > > > Having two places that issue thread in a different way is also
> useless
> > > complexity.
> > >
> > > I have to disagree with you there. Each place uses concurrency in a way
> > > that
> > > is appropriate to its use case. Trying to shoehorn things into
> > > inappropriate
> > > concurrency models just makes the codebase harder to understand.
> There's a
> > > reason why we use different data structures in different places, etc.
> > >
> > > (Right now I'm battling the DWARF writer implementation, which has
> built up
> > > 10 years of technical debt as a result of using a poorly designed,
> overly
> > > generic data structure.)
> > >
> > > > Rewriting the split code gen is on my list of cleanup for some time
> and
> > > if I actually had to use it on my cod path I would already have
> decoupled
> > > it and moved it to the thread pool.
> > > >
> > > > >
> > > > >> The fact that you want it implemented in a different place is not
> a
> > > reason to not implement it now.
> > > > >
> > > > > My view is that *if* we implement it, it should not be here. I am
> > > against
> > > > > adding it now, anywhere.
> > > >
> > > > Well we'll need to continue talking about it because I have a totally
> > > different opinion on this topic.
> > > >
> > > >
> > > > >
> > > > >> I'm against building any technical debt when we can easily avoid
> it.
> > > > >
> > > > > So am I. My view is that adding the thread pool implementation
> > > > The thread pool implementation is already in the code base.
> > >
> > > I meant the split code gen implementation that uses thread pools.
> > >
> > > > > adds to the
> > > > > technical debt because it would mean more code and therefore more
> > > complexity
> > > >
> > > > I don't buy that and unless we're actually looking at a diff I'm not
> > > sure we'll be able to make progress on the tradeoff at stance here.
> > >
> > > Fair enough. If you still hold that view I think we'll have to agree to
> > > disagree here, I'll drop my argument and we can get back to coding :)
> > >
> > > Thanks,
> > > --
> > > Peter
> > >
>
> --
> Peter
>



-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160222/e32f8de2/attachment-0001.html>