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

[Peter Collingbourne via llvm-commits]

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