[PATCH] D132601: [llvm-profdata] Improve profile supplementation

[Rong Xu via Phabricator via llvm-commits]

xur added inline comments.


================
Comment at: llvm/lib/ProfileData/ProfileSummaryBuilder.cpp:226
   // Skip invalid count.
-  if (Count == (uint64_t)-1)
+  if (Count == (uint64_t)-1 || Count == (uint64_t)-2)
     return;
----------------
davidxl wrote:
> xur wrote:
> > davidxl wrote:
> > > probably worth defining symbolic constants for these values.
> > > 
> > > Also how can we prevent these values are accidentally used somewhere else?
> > Symbolic constants seem reasonable.
> > 
> > These values should not be encountered in real profiles. 2^64 is too large for any counter values to come close.
> > But we do need to special handling of these values. 
> > It's possible to misuse -- I think overlap function does not handle this well.
> > There are not too many places to use the counters directly. We just need to check all of them. 
> > 
> > There might be other ways to implement -- like extending the profile format. But this is the simplest way.
> profile counter reader should do something when reading those values into instrProfRecord so that the special values are not leaked.
Agreed. We can have some APIs to InstrProfRecord to encapsulate the values.

The other alternative is to extend the profile format to have a information section for each function. Right now we are overload some information in the function hash function. I think have an optional information section for each function would be useful. 




================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:507
 
   // Scale up the MaxCount to be multiple times above hot threshold.
   const unsigned MultiplyFactor = 3;
----------------
davidxl wrote:
> comment mismatches.
> 
will fix


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:514
+    return;
   ProfRecord->scale(Numerator, Denominator, [&](instrprof_error E) {
     warn(toString(make_error<InstrProfError>(E)));
----------------
davidxl wrote:
> are the counts all -1 or -2 in this case? what is the point of scaling?
If we assign -1 or -2, we will not do scaling, it will early return on line 504.
(that happens all zero counters or zero counters percentage exceed the threshold.)

This part is doing the scaling on the original counters.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132601/new/

https://reviews.llvm.org/D132601