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


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:396
+  }
+  Index.addBlockCount(NumBlocks);
 
----------------
Can we use Index.addBlockCount(F.size())?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79831