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

Tue May 26 13:05:51 PDT 2020

yamauchi marked 2 inline comments as done.
yamauchi 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.
----------------
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79831