[PATCH] D15828: [PGO] Passmanagerbuilder change that enable IR level PGO instrumentation

[Xinliang David Li via llvm-commits]

sgtm.

On Fri, Jan 15, 2016 at 6:45 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
> On Fri, Jan 15, 2016 at 6:21 PM, Xinliang David Li <davidxl at google.com>
> wrote:
>>
>> On Fri, Jan 15, 2016 at 6:09 PM, Sean Silva <chisophugis at gmail.com> wrote:
>> > silvas added inline comments.
>> >
>> > ================
>> > Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:216
>> > @@ +215,3 @@
>> > +    MPM.add(createGVNPass(DisableGVNLoadPRE));  // Remove redundancies
>> > +    addExtensionsToPM(EP_Peephole, MPM);
>> > +  }
>> > ----------------
>> > Let's drop the pre-instrumentation passes for now and leave a TODO. We
>> > will want to have a focused discussion on what to put here.
>> >
>> > ================
>> > Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:281
>> > @@ -239,1 +280,3 @@
>> >
>> > +  addPGOInstrPasses(MPM);
>> > +
>> > ----------------
>> >> Yes, and there was not consensus in the RFC for the pre-inlining.
>> >> Instead, the approach suggested was to work on independent components which
>> >> seem non-controversial and have a separate focused discussion about
>> >> pre-inlining and other interactions.
>> >
>> > Agreed. We will definitely want to have a focused discussion with
>> > real-world performance measurements when doing pass pipeline tuning (I have
>> > a big chunk of time allocated for characterizing and tuning the new IR-level
>> > instrumentation stuff and am looking forward to participating!). For the
>> > moment, just wiring it up and leaving a TODO for the pre-instrumentation
>> > passes is probably the right incremental step.
>> >
>>
>> My suggestion is to leave the pipeline changes in but not off by
>> default in some way. This allows much easier experiments with options
>> flipped on and off.
>>
>> For serious performance tuning, I strongly recommend doing so
>> (extensively) only after PGO data is hooked up with inliner. Without
>> that in place, the results will be misleading.
>
>
> Not for analyzing the performance benefits for the instrumented build.
> Leaving in the pipeline changes as an on/off option is misleading, because
> there are many more possibilities than just what you have there now. Testing
> on/off only what you have there now (and not other variations) is not
> particularly interesting (and may be actively detrimental if it suggests
> that there are only two configurations to test).
>
> As part of a more focused discussion we can think about what is interesting
> to test and add flags to support that. But adding on/off support for a
> single arbitrary configuration is not particularly interesting.
>
> -- Sean Silva
>
>
>>
>>
>> David
>>
>>
>> > (I just got back from vacation and am still working on my email backlog,
>> > but I should be back on my feet next week and the new IR-level
>> > instrumentation stuff is my top priority at the moment)
>> >
>> >
>> > http://reviews.llvm.org/D15828
>> >
>> >
>> >
>
>