[PATCH] D113781: [llvm-profgen] Compute and show profile density

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/inline-cs-pseudoprobe.test:23-24
 
+; CHECK-DENSITY: AutoFDO is estimated to optimize better with 1675.7x more samples. Please consider increasing sampling rate or profiling for longer duration to get more samples.
+; CHECK-DENSITY: Minimum profile density for hot functions with top 99.00% total samples: 0.6
+
----------------
wenlei wrote:
> I suggest we use a dedicated test case instead of adding feature test into various existing test cases. That way it helps keep testing isolated separated.
Sounds good!


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:143-149
+    OS << "The --profile-summary-cutoff-hot option may be set too low. Please "
+          "check your command.\n";
+  else if (Density < HotFunctionDensityThreshold)
+    OS << "AutoFDO is estimated to optimize better with "
+       << format("%.1f", HotFunctionDensityThreshold / Density)
+       << "x more samples. Please consider increasing sampling rate or "
+          "profiling for longer duration to get more samples.\n";
----------------
wenlei wrote:
> Maybe these two messages should be `warning`?
Good point!


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:735
+
+  calculateAndShowDensity(ProfileMap);
 }
----------------
wenlei wrote:
> Perhaps we can let calculateAndShowDensity take a merged profile map, then `calculateAndShowDensity` doesn't have to be a virtual function and can have full implementation in `ProfileGeneratorBase`. We can prepare merged profiles here and pass into `calculateAndShowDensity`.
Changed.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:89
+
+  void showDensitySuggestion(double Density, raw_fd_ostream &OS);
+
----------------
wenlei wrote:
> Can we omit `raw_fd_ostream &OS` as a parameter and use `outs()` directly inside? 
> 
> With that, can we also remove the include for `raw_os_ostream.h`? 
Sounds good!


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:80
+
+  size_t getFuncSize() {
+    size_t Sum = 0;
----------------
wenlei wrote:
> nit: we've been using `uint64_t` for size. would be good to be consistent, even though they're the same thing here. 
Good to know this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113781