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

[Xinliang David Li via llvm-commits]

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.

I mean 'off' by default.

David

>
> 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.
>
> 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
>>
>>
>>