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

[Dehao Chen via llvm-commits]

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())?

Dehao

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