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

[Teresa Johnson via llvm-commits]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160216/f8954386/attachment.html>