[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 10 inline comments as done.
xur added a comment.
Herald added a subscriber: wenlei.

I'm reviving this patch because this fixes one of our internal bugs.

Before the fix, The BFI counters are way off the raw profile counter:

` BB : Count=96615607  BFI Count=16

  BB : Count=96615607  BFI Count=16
  BB : Count=9874441  BFI Count=2
  BB : Count=9849389  BFI Count=2
  BB : Count=9928400  BFI Count=2
  BB : Count=96694618  BFI Count=16
  BB : Count=30  BFI Count=0
  BB : Count=96694588  BFI Count=16
  BB : Count=93506121  BFI Count=16
  BB : Count=99833531  BFI Count=16
  BB : Count=99833531  BFI Count=16
  BB : Count=6933665  BFI Count=1
  BB : Count=6933665  BFI Count=1
  BB : Count=4641130  BFI Count=1
  BB : Count=4634507  BFI Count=1
  BB : Count=4634507  BFI Count=1
  BB : Count=3439260  BFI Count=1
  BB : Count=4634090  BFI Count=1
  BB : Count=4624009  BFI Count=1
  BB : Count=4634090  BFI Count=1
  BB : Count=2299158  BFI Count=0
  BB : Count=2230547  BFI Count=0
  BB : Count=2234047  BFI Count=0
  BB : Count=3500  BFI Count=0
  BB : Count=2230547  BFI Count=0
  BB : Count=2225955  BFI Count=0
  BB : Count=18824  BFI Count=0
  BB : Count=2230547  BFI Count=0
  BB : Count=2230547  BFI Count=0
  BB : Count=2230585  BFI Count=0
  BB : Count=2229242  BFI Count=0
  BB : Count=38  BFI Count=0
  BB : Count=2230547  BFI Count=0
  BB : Count=68611  BFI Count=0
  BB : Count=103021581  BFI Count=16
  BB : Count=103021611  BFI Count=16
  BB : Count=6406005  BFI Count=1

`

With this patch, we can get:

  `BB : Count=1  BFI Count=6145756
  BB : Count=96615607  BFI Count=98836277
  BB : Count=96615607  BFI Count=98836277
  BB : Count=9874441  BFI Count=10101401
  BB : Count=9849389  BFI Count=10075773
  BB : Count=9928400  BFI Count=10075773
  BB : Count=96694618  BFI Count=98836277
  BB : Count=30  BFI Count=31
  BB : Count=96694588  BFI Count=98836246
  BB : Count=93506121  BFI Count=95577159
  BB : Count=99833531  BFI Count=95577159
  BB : Count=99833531  BFI Count=95577159
  BB : Count=6933665  BFI Count=6844610
  BB : Count=6933665  BFI Count=6844610
  BB : Count=4641130  BFI Count=4581520
  BB : Count=4634507  BFI Count=4574982
  BB : Count=4634507  BFI Count=4574982
  BB : Count=3439260  BFI Count=3395087
  BB : Count=4634090  BFI Count=4574982
  BB : Count=4624009  BFI Count=4565030
  BB : Count=4634090  BFI Count=4574982
  BB : Count=2299158  BFI Count=2269628
  BB : Count=2230547  BFI Count=2201898
  BB : Count=2234047  BFI Count=2205353
  BB : Count=3500  BFI Count=3455
  BB : Count=2230547  BFI Count=2201898
  BB : Count=2225955  BFI Count=2197365
  BB : Count=18824  BFI Count=18582
  BB : Count=2230547  BFI Count=2201898
  BB : Count=2230547  BFI Count=2201898
  BB : Count=2230585  BFI Count=2201936
  BB : Count=2229242  BFI Count=2200610
  BB : Count=2230547  BFI Count=2201898
  BB : Count=68611  BFI Count=67730
  BB : Count=103021581  BFI Count=98836246
  BB : Count=103021611  BFI Count=98836277
  BB : Count=6406005  BFI Count=6145756

`

I will post the updated patch shortly.



================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1568
+    uint64_t BFICountValue = 0;
+    if (!Func.getBBInfo(&BBI).CountValid)
+      continue;
----------------
davidxl wrote:
> In what situation can this happen?
I changed to findBBInfo.
Usually this won't happen. But this can happen if there is unreachable BB (like when build at -O0).


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1571
+    auto BFICount = NBFI.getBlockProfileCount(&BBI);
+    if (!BFICount)
+      continue;
----------------
davidxl wrote:
> how can this be possible?
If the entryCount is 0, we don't have Profile Count for BB.  
This could happen if we did not fix the entryCount.

But now, 0 entryCount is not possible after we populatedCounters (if the maxcount is nonzero). This is not needed.  I will remove.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1585
+
+    if (Scale >= 1.01 || Scale <= 0.99) {
+      uint64_t FuncEntryCount = Func.getBBInfo(&*F.begin()).CountValue;
----------------
davidxl wrote:
> early return?
Done


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1588
+      uint64_t NewEntryCount = FuncEntryCount;
+      if (NewEntryCount == 0)
+        NewEntryCount = 1;
----------------
davidxl wrote:
> Is this handled by the next statements?
Changed the code.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1602
+  }
+  F.setEntryCount(ProfileCount(0, Function::PCT_Real));
+}
----------------
davidxl wrote:
> Is this needed?
Probably not. I removed it.


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

https://reviews.llvm.org/D61540