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

[Peter Collingbourne via llvm-commits]

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