[PATCH] D33540: [PM/ThinLTO] Port the ThinLTO pipeline (both components) to the new PM.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D33540#765851, @mehdi_amini wrote:

> In https://reviews.llvm.org/D33540#765031, @chandlerc wrote:
>
> > In https://reviews.llvm.org/D33540#764937, @davidxl wrote:
> >
> > > For partial inlining, leave it consistent with old PM for now.   We can introduce later an internal option to enable/disable pre-thin-link partial inlining and collect more performance number.
> >
> >
> > Is there a rationale for the position in the old PM? Was this discussed somewhere?
>
>
> Do you really think I flipped a coin? `git log`/`git blame` easily find the original review that introduced the ThinLTO pipeline: http://reviews.llvm.org/D17115


Just to fully close the loop on this: as David said this was *only* a question about the partial inliner (I'm aware of the clear discussion around the larger ThinLTO pipeline).

And even then, I absolutely don't believe it was "flipping a coin". =] I can imagine that there was some clear rationale and it maybe didn't get written down, or that there wasn't enough information at the time which is also fine but useful to understand when making subsequent decisions.

> 
> 
>> I'd almost rather adjust the position in the old PM to be consistent with here.
> 
> What data do you have? Your intuition alone is not a reason to go over the extensive tests we've been doing before setting it up the way it is. The pipeline is what it is now because I started with the same "intuition" and "rational" as you do, and tried to not perform inlining. However I did a bunch of measurement before landing it, and had to realize that (at that time at least), the bitcode was much larger, there were more functions to be imported, and it slowed down the compile time significantly (I'm not saying it is still the case, I don't know, but I don't see data here).

Yeah, sorry, this all only makes sense for *partial inlining*, not for the rest of it. Anyways... Sorry for confusion.

And even then, I want to be clear that if there have been extensive tests that show that the partial inliner needs to have a particular location, clearly that is more important. I just wasn't finding any documentation trail pointing at these results, and I think that if we actually have something better than intuition, it is important to document it where possible.



================
Comment at: include/llvm/Passes/PassBuilder.h:256
+  buildThinLTOPreLinkDefaultPipeline(OptimizationLevel Level,
+                                     bool DebugLogging = false);
+
----------------
mehdi_amini wrote:
> It is the first time I see "prelink" to describe the compile phase. I find this confusing. The "link" happens after ThinLTO, we never link the IR in ThinLTO.
We don't link the IR, but we do something called 'thin link' which this is before, and so 'pre-link' didn't seem completely crazy to me.

It also has the advantage of matching the terminology used for normal LTO...

But I'm totally open to a better set of terminology here if you have some in mind. I'm worried that 'compile' may be too generic, but maybe it isn't. Thoughts?


https://reviews.llvm.org/D33540