[PATCH] D32877: Restrict call metadata based hotness detection to Sample PGO mode

[Teresa Johnson via llvm-commits]

On Fri, May 5, 2017 at 4:08 PM, Dehao Chen <danielcdh at gmail.com> wrote:

> On Fri, May 5, 2017 at 4:03 PM, Teresa Johnson via Phabricator
> <reviews at reviews.llvm.org> wrote:
> > tejohnson added a comment.
> >
> > In https://reviews.llvm.org/D32877#747704, @danielcdh wrote:
> >
> >> In https://reviews.llvm.org/D32877#747667, @tejohnson wrote:
> >>
> >> > In https://reviews.llvm.org/D32877#747647, @eraman wrote:
> >> >
> >> > > I feel the underlying problem here is the use of VP metadata to get
> counts in sample PGO mode.  Dehao, even when VP metadata is available, can
> we add branch_weights to the call? Then, only sample PGO will annotate
> branch weights to calls and that won't affect instrumented PGO.
> >> >
> >> >
> >> > That sounds like potentially a good longer term change. But I'd still
> like for this to go in as an immediate fix for the issue, which is causing
> a noticeable impact on instrumentation PGO performance, and because there
> is no reason AFAIK for anything other than Sample PGO to need to look at
> call metadata for determining hotness.
> >>
> >>
> >> Just to confirm, with https://reviews.llvm.org/D32773, we should have
> recovered all the instrumented PGO performance, isn't it?
> >
> >
> > You're right, I suspect that it may (will be checking after it is fully
> integrated here), but my concern is that there may still be other
> transformations that don't update the VP metadata properly (I mentioned
> earlier this week that e.g. it appears the metadata isn't being dropped
> when calls are devirtualized after constant prop, although I don't think
> this causes the performance issue). These things should be fixed for other
> reasons including Sample PGO accuracy, but it is not needed for
> instrumentation PGO in the first place - is there any reason for anything
> other than Sample PGO to look at this instead of the branch weights?
> >
> >
> >
> > ================
> > Comment at: lib/Analysis/ProfileSummaryInfo.cpp:88
> > +  else if (auto Kind = getKind(Inst->getModule()))
> > +    IsSamplePGO = Kind.getValue() == ProfileSummary::PSK_Sample;
> > +  if (IsSamplePGO) {
> > ----------------
> > danielcdh wrote:
> >> Looks like you can always get the Kind from Inst, why would you want to
> pass in the Summary?
> > It involves doing some work that is already done when the summary is
> available, so this was added just for the case where it is invoked without
> a summary.
>
> I mean can we remove the "Summary" parameter of this function, and
> always compute this from getKind(Inst->getModule())?
>

I know that's what you were suggesting, but my point is that that requires
doing some additional work for every instruction (this routine will be
called many times), when we already have that in many cases cached on the
ProfileSummary object.


>
> Dehao
>
> >
> >
> > https://reviews.llvm.org/D32877
> >
> >
> >
>



-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170505/bff1b655/attachment.html>