[PATCH] D21405: [PGO] IRPGO pre-cleanup pass changes

[Xinliang David Li via llvm-commits]

On Fri, Jul 29, 2016 at 9:19 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
> chandlerc added a subscriber: chandlerc.
> chandlerc added a comment.
>
> In https://reviews.llvm.org/D21405#459364, @silvas wrote:
>
>> In https://reviews.llvm.org/D21405#459242, @mehdi_amini wrote:
>>
>> > I really don't like the fact that the PGO pipeline will be different from the non-PGO pipeline (other than what is required for instrumentations). I wonder what other people will think of that.
>>
>>
>> This was one of my initial concerns too: http://lists.llvm.org/pipermail/llvm-dev/2015-August/089058.html
>>  Jake and I (actually mostly Jake) did some basic measurements related to this and did not find any significant difference.
>>
>> Rong also did some measurements related to this in the initial RFC thread: http://lists.llvm.org/pipermail/llvm-dev/2015-August/089425.html
>>
>> I agree though: generally speaking, we should avoid running too many passes before instrumentation precisely because they cause divergence between the PGO and non-PGO pipelines. Thankfully Jake and I found that on our games only pre-inlining was needed (running other optimizations before instrumentation did not help significantly for our test cases beyond the benefit of just pre-inlining).
>
>
> I'm glad this is just pre-inlining, I strongly agree that we should not diverge here.
>
> I actually continue to disagree with pre-inlining. I have said this repeatedly in the past, and I'm not sure where we reached consensus that pre-inlining was the right design, and where the discussion of the tradeoffs took place. The closest I could find was the thread you linked to Sean, but I don't actually see a lot of discussion of alternatives there, nor do I see much real discussion of the inliner at all outside of the fact that yes, it does work.
>
> I would be much more in favor of a design which instead of changing the pipeline to optimize differently (and thus having PGO vs. normal divergence) either:
>
> - Teaches the instrumentation pass to do the necessary (possibly interprocedural) analysis to instrument only in places it will be cheap,

This is already done with IR instrumentation (using minimal spanning
tree and static prediction). The huge improvement is achieved with
this patch is on top of that.

 or
> - Teach the existing pipeline to actually optimize and transform the instrumentation code to remove the overhead as necessary.

Firstly, design and implement a special (potentially a very
complicated one) pass instead of using a generic optimization to clean
it up is a bad idea.

1) it won't be as effective -- not even close in theory. See
discussion in  "RFC: Pass to prune redundant profiling
instrumentation"
2) why bother generating redundant code and wasting compile time later
to clean it up (may or may not do a good job) instead of not
generating in the first place?
3) adding post-cleanup also potentially requires significant
change/support in profile-rt and profile host tools which adds more
cost for maintenance.
4) inliner is one of the most effective way to enable optimizations
(if used appropriately in different contexts). If it can not be
re-used, something is fundamentally wrong.

>
> This way we have a single optimization pipeline that we turn profile info on and off of ,rather than twe pipelines.

the pipeline with/without profile info is already different
(especially considering the non-existing cleanup pass which may
require lots of other analysis passes). Same is true for many other
peak optimization modes. What benefit of this goal of single pipeline?
better compile time? better runtime performance? Perhaps low
maintenance? I don't think so -- given new the pass required to do the
cleanups.

David

>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D21405
>
>
>