[PATCH] D79831: [PGO] Improve the working set size heuristics under the partial sample PGO.

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/IR/ProfileSummary.h:62
+  /// partial sample profile. When 'Partial' is false, it is undefined.
+  double PartialProfileRatio;
   /// Return detailed summary as metadata.
----------------
yamauchi wrote:
> wmi wrote:
> > yamauchi wrote:
> > > mtrofin wrote:
> > > > Can this be initialized, for maintainability (even if it's currently initialized in the ctor - code evolution may omit that later. Initializing at declaration reduces surprise bugs later)
> > > Done in D79951.
> > PartialProfileRatio is only available in ThinLTO mode. Better make it explicit in the comment.
> > 
> > PartialProfileRatio is set to "BlockCount / NumCounts". It doesn't match the ratio of "profile counters of the program being built" / "the number of profile counters in the partial sample profile", because for AFDO every block may have multiple counters inside of it. I think that is why you put "approximiately" in front of the comment. 
> > 
> > Can we redefine PartialProfileRatio to "BlockCount / NumCounts * PartialSampleProfileWorkingSetSizeScaleFactor", so PartialProfileRatio can use the definition in the comment exactly instead of approximately? We may rename PartialSampleProfileWorkingSetSizeScaleFactor using something like NumofCounterInBlockScaleFactor. That will also give the flag PartialSampleProfileWorkingSetSizeScaleFactor more concrete meaning. 
> Will add a comment about "only available in thin LTO mode."
> 
> It'd be nice to give precise meaning, but I wonder if those counts are //that// precise.
> 
> If NumofCounterInBlockScaleFactor == the number of counters per block, it'd have the value of 0.008, which is set by tuning, would the value/name make sense?
> 
> 
Ok, the scale factor is not just the "factor of counters per block", but "factors of counters per block * (number of block s in working set / number of all the blocks of current program)". How about keep using PartialSampleProfileWorkingSetSizeScaleFactor but explain its meaning more clearly?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79831