This is an archive of the discontinued LLVM Phabricator instance.

Add a ProfileCount class to represent entry counts.
ClosedPublic

Authored by eraman on Jan 9 2018, 2:45 PM.

Details

Summary

The class wraps a uint64_t and an enum to represent the type of profile
count (real and synthetic) with some helper methods.

Diff Detail

Repository
rL LLVM

Event Timeline

eraman created this revision.Jan 9 2018, 2:45 PM
davidxl added inline comments.Jan 10 2018, 1:55 PM
include/llvm/IR/Function.h
237 ↗(On Diff #129165)

Perhaps also split sample count and instrumentation count?

246 ↗(On Diff #129165)

Why is there a need for the static member?

254 ↗(On Diff #129165)

Do we need to interfaces for the same thing? I suggest removing one.

255 ↗(On Diff #129165)

Is this interface useful?

260 ↗(On Diff #129165)

Perhaps also add a parameter for the type?

This can avoid dropping count silently when the type mismatches.

eraman added inline comments.Jan 10 2018, 3:32 PM
include/llvm/IR/Function.h
237 ↗(On Diff #129165)

Right now the name used in metadata is used to differentiate real and synthetic counts. We currently use function_entry_count which I treat as real and then add a new synthetic_entry_count which is treated as of type synthetic. We use function_entry_count for both sample and instrumented PGO. Do you want to use separate names there?

246 ↗(On Diff #129165)

I create an unqiue ProfileCount with type set to Invalid and use that when getInvalid() is called. I could construct an invalid object everytime getInvalid is called but I thought this is preferable.

260 ↗(On Diff #129165)

Not sure what you mean here. We have several places where we get the count and clone it to a different function or scale the value. All these places, we don't need to know the type of the count is, but want to preserve the same type after manipulating the value. This can be done by doing something like

Count = F.getEntryCount();
NewCount = foo(Count.getValue())
Count.setCount(NewCount)
F.setEntryCount(Count)

So there is no issue of type mismatch here.

davidxl added inline comments.Jan 10 2018, 3:39 PM
include/llvm/IR/Function.h
237 ↗(On Diff #129165)

We can split the enum, but keep the interfaces as they are. We may find the need to differentiate later.

246 ↗(On Diff #129165)

Why is using static member more preferrable? Or make it a const object?

260 ↗(On Diff #129165)

The case I am thinking is the mixed profile case:

e.g, the function has real profile count but overriden by this method with synthetic count.

Maybe change the name to 'updateCount' or resetCount ?

eraman marked 2 inline comments as done.Jan 10 2018, 6:11 PM

Also expanded getEntryCount to read synthetic_function_entry_count.

include/llvm/IR/Function.h
237 ↗(On Diff #129165)

As discussed offline, there is not enough information in the metdata to differentiate the two. We can go down that route later if needed.

246 ↗(On Diff #129165)

Removed the static

255 ↗(On Diff #129165)

Useful in unit tests and I have also added a assert in setEntryCount where this is used.

260 ↗(On Diff #129165)

The right place for that is setEntryCount. This one just changes the value of a ProfileCount object. Added an assert in setEntryCount to check either existing type is invalid or the same as what you're setting.

eraman updated this revision to Diff 129384.Jan 10 2018, 6:12 PM

Address David's comments and support synthetic entry counts in getEntryCount

davidxl accepted this revision.Jan 11 2018, 8:52 AM

lgtm

This revision is now accepted and ready to land.Jan 11 2018, 8:52 AM
This revision was automatically updated to reflect the committed changes.