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

[Xinliang David Li via llvm-commits]

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.

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