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.

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

Perhaps also split sample count and instrumentation count?

246

Why is there a need for the static member?

254

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

255

Is this interface useful?

260

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

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

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

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

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

246

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

260

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

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

Removed the static

255

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

260

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.