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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

If the profile parser part were in a separate patch, would the size of the resulting patches be more balanced out? I'm wondering if splitting this into smaller patches would be meaningful.



================
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.
----------------
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)


================
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 "
----------------
How did we come by the 0.008 value?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp:987
   }
 }
----------------
unrelated change (the last line)


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79831