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

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi marked 8 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.
----------------
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.


================
Comment at: llvm/lib/Analysis/ProfileSummaryInfo.cpp:82
+    "partial-sample-profile-working-set-size-scale-factor", cl::Hidden,
+    cl::init(0.008),
+    cl::desc("The scale factor used to scale the working set size of the "
----------------
mtrofin wrote:
> How did we come by the 0.008 value?
It came from tuning of large app / benchmarks so large working set size won't trigger for benchmarks.


================
Comment at: llvm/lib/IR/ProfileSummary.cpp:118
+static bool getVal(MDTuple *MD, const char *Key, double &Val) {
+  if (!MD)
+    return false;
----------------
mtrofin wrote:
> Most of the code looks similar to the getVal above. Could some factoring reduce that duplication?
Done in D79951.


================
Comment at: llvm/lib/IR/ProfileSummary.cpp:194
 
-  int i = 0;
+  unsigned i = 0;
   auto &FormatMD = Tuple->getOperand(i++);
----------------
mtrofin wrote:
> While at it - could we rename lower-case i to upper case something?
Done in D79951.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79831