[PATCH] D113781: [llvm-profgen] Compute and show profile density
Lei Wang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Nov 28 23:36:31 PST 2021
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
More information about the llvm-commits
mailing list