Page MenuHomePhabricator

Profile summary cleanup.
ClosedPublic

Authored by eraman on Mar 24 2016, 5:19 PM.

Details

Summary

Move MaxFunctionCount and NumFunctions to the base class. This allows to get max function count from profile summary (which is needed to phase out MaxFunctionCount module flag) without knowing what format of profile is used. I have retained all the existing interfaces in SampleProfSummary (so, for instance, getMaxHeadSamples and getMaxFunctionCount would return the same thing).

Diff Detail

Repository
rL LLVM

Event Timeline

eraman updated this revision to Diff 51620.Mar 24 2016, 5:19 PM
eraman retitled this revision from to Profile summary cleanup..
eraman updated this object.
eraman added a reviewer: davidxl.
eraman added subscribers: llvm-commits, vsk.
davidxl accepted this revision.Mar 25 2016, 11:23 AM
davidxl edited edge metadata.

lgtm

This revision is now accepted and ready to land.Mar 25 2016, 11:23 AM
eraman updated this revision to Diff 51686.Mar 25 2016, 2:36 PM
eraman edited edge metadata.

My original change had a bug due to the fact that sample profile head samples are also counted in body samples (I couldn't run the tests yesterday since the head was broken). I had to undo some of the refactoring from my original patch, but the main stuff - moving max function count and num functions to the base class - still remains the same. I have tested this.

Is there a way to exclude header sample from the body samples?

Is there a way to exclude header sample from the body samples?

That'll definitely make it consistent with instrumented profile and I'll let Diego chime in. However, that should be a separate patch.

dnovillo edited edge metadata.Mar 28 2016, 9:16 AM

Is there a way to exclude header sample from the body samples?

I'm surprised that body samples count header samples. That seems like a bug to me. Easwaran, making both instrumented and sample consistent is a good idea. Thanks.

dnovillo accepted this revision.Mar 28 2016, 9:19 AM
dnovillo added a reviewer: danielcdh.
dnovillo edited edge metadata.

LGTM

include/llvm/ProfileData/ProfileCommon.h
153 ↗(On Diff #51686)

Perhaps we should deprecate getMaxHeadSamples? IIUC, MaxFunctionCount is the number of times a function was called, correct? That's essentially the same information provided by counting the number of samples in the function header.

There is a difference between the two, but I don't think the distinction is worth keeping. Dehao, what do you think?

danielcdh added inline comments.Mar 28 2016, 9:38 AM
include/llvm/ProfileData/ProfileCommon.h
153 ↗(On Diff #51686)

Agree, MaxHeadSamples should be the same as MaxFunctionCount.

eraman added inline comments.Mar 28 2016, 10:50 AM
include/llvm/ProfileData/ProfileCommon.h
153 ↗(On Diff #51686)

I thought it is useful in terms of readability to have interfaces named in SampleProfile specific terms. Both MaxHeadSamples and MaxFunctionCount are indeed same and hence getMaxHeadSamples returns the MaxFunctionCount. If this is causing confusion, I'll deprecate this in a separate change.

This revision was automatically updated to reflect the committed changes.