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

[Sean Silva 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.
>
> 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
> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160115/a8079cc7/attachment.html>