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

[Easwaran Raman via Phabricator via llvm-commits]

eraman added inline comments.


================
Comment at: include/llvm/IR/Function.h:237
 
+  enum ProfileCountType { PCT_Invalid, PCT_Real, PCT_Synthetic };
+
----------------
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?


================
Comment at: include/llvm/IR/Function.h:246
+    ProfileCountType PCT;
+    static ProfileCount Invalid;
+
----------------
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. 


================
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:
> 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.


https://reviews.llvm.org/D41883