[PATCH] D41883: Add a ProfileCount class to represent entry counts.

[Easwaran Raman via Phabricator via llvm-commits]

eraman marked 2 inline comments as done.
eraman added a comment.

Also expanded getEntryCount to read synthetic_function_entry_count.



================
Comment at: include/llvm/IR/Function.h:237
 
+  enum ProfileCountType { PCT_Invalid, PCT_Real, PCT_Synthetic };
+
----------------
davidxl wrote:
> eraman wrote:
> > davidxl wrote:
> > > Perhaps also split sample count and instrumentation count?
> > 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?
> We can split the enum, but keep the interfaces as they are.  We may find the need to differentiate later.
As discussed offline, there is not enough  information in the metdata to differentiate the two. We can go down that route later if needed.


================
Comment at: include/llvm/IR/Function.h:246
+    ProfileCountType PCT;
+    static ProfileCount Invalid;
+
----------------
davidxl wrote:
> eraman wrote:
> > davidxl wrote:
> > > Why is there a need for the static member?
> > 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. 
> Why is using static member more preferrable? Or make it a const object?
Removed the static


================
Comment at: include/llvm/IR/Function.h:255
+    uint64_t getValue() const { return Count; }
+    ProfileCountType getType() const { return PCT; }
+    bool isSynthetic() const { return PCT == PCT_Synthetic; }
----------------
davidxl wrote:
> Is this interface useful?
Useful in unit tests and I have also added a assert in setEntryCount where this is used.


================
Comment at: include/llvm/IR/Function.h:260
+    // Update the count retaining the same profile count type.
+    ProfileCount &setCount(uint64_t C) {
+      Count = C;
----------------
davidxl wrote:
> eraman wrote:
> > davidxl wrote:
> > > Perhaps also add a parameter for the type?
> > > 
> > > This can avoid dropping count silently when the type mismatches.
> > 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.
> 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 ?
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. 


https://reviews.llvm.org/D41883