[PATCH] D61540: [PGO] Use sum of count values to fix func entry count and add a check to verify BFI counts

[Rong Xu via Phabricator via llvm-commits]

xur marked 5 inline comments as done.
xur added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:257
+
+static cl::opt<unsigned> PGOVerifyBFICutoff(
+    "pgo-verify-bfi-cutoff", cl::init(1), cl::Hidden,
----------------
davidxl wrote:
> It is probably more useful to have an option controlling checking hot BB's only -- check with either RawCount or BFI count is hot (can catch the case when a cold block becomes 'hot' with BFI info).
Yes. Checking hot BFI counts will catch some abnormal cases.
The reason I used a cutoff instead of default hot threshold is that there could be too many hot BB to check for some programs.
Using a high cutoff could make the output much cleaner. And the user can specify the hot threshold from detailed summary.

I will probably keep this option and add another option that checking hot BB as you suggested.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1559
+// adjust the func entry count.
+static void fixFuncEntryCount(PGOUseFunc &Func, LoopInfo &LI,
+                              BranchProbabilityInfo &NBPI) {
----------------
davidxl wrote:
> what is the rationale and use of this fixup? 
There is no perfect way to fix this: when the information is lost, we could not recover. This is just one way to fix it so that the total count close.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1612
+  bool PrintFunc = false;
+  unsigned BBNum = 0, BBNoMatchNum = 0, NonC0BBNum = 0;
+  for (auto &BBI : F) {
----------------
davidxl wrote:
> Add comment of make variable name NonC0BBNum more obvious for its meaning.
will do.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1636
+        dbgs() << "VerifyFuncBFI: in " << F.getName() << "\n";
+        PrintFunc = true;
+      }
----------------
davidxl wrote:
> Variable not used.
will put this variable under Debug macro.


================
Comment at: llvm/test/Transforms/PGOProfile/fix_bfi.ll:1
+; RUN: llvm-profdata merge %S/Inputs/fix_bfi.proftext -o %t.profdata
+; RUN: opt < %s -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-fix-entry-count=true -pgo-verify-bfi-ratio=2 -pgo-verify-bfi=true 2>&1 | FileCheck %s --check-prefix=FIX-CHECK
----------------
davidxl wrote:
> Add an comment on the what the test case is testing (especially the fix check part).
will do.


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

https://reviews.llvm.org/D61540