[PATCH] D84378: [PGO] Fix incorrect function entry count

[Rong Xu via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1144
+    // Fix it if necessary.
+    if (InstrBB == FuncEntry && CountValue == 0)
+      CountValue = 1;
----------------
davidxl wrote:
> davidxl wrote:
> > xur wrote:
> > > davidxl wrote:
> > > > add comment here about the root cause: counter cleared before dump. 
> > > > 
> > > > If counter promotion is disabled, in what situation entry count can not be recovered with propagation?
> > > > 
> > > This is independent of count promotion. Usually the entry count is not promoted (unless it's inlined to a callsite inside a loop).
> > > 
> > > This code here is for the case when we instrument the entry BB. If the entry BB is instrumented, it cannot be computed from propagation.  (MST does not have the redundancy to recover)
> > > 
> > > Typically, it happens this way: for a func with a long running loop, after we reset the counter, the entry counters of this func are also zeroed out. If we dump the counters in the middle of the loop, the entry count will still be 0.
> > > 
> > > If we promote the counters inside the loop, the counter value inside the loop will be 0 (because the updates are in the exit block -- never exercised). If we disable the promote, the counter value insides the loop are OK.
> > ok. what if entry is not instrumented, but two post-dom successors? If they are zeroed out, is there a way to recover?
> Also look at the caller of this function -- even when AllZeros is true, this function is still called, so the entry count may be wrongly fixed up.
It will not fix here, but later. When we set the entry count.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1144
+    // Fix it if necessary.
+    if (InstrBB == FuncEntry && CountValue == 0)
+      CountValue = 1;
----------------
xur wrote:
> davidxl wrote:
> > davidxl wrote:
> > > xur wrote:
> > > > davidxl wrote:
> > > > > add comment here about the root cause: counter cleared before dump. 
> > > > > 
> > > > > If counter promotion is disabled, in what situation entry count can not be recovered with propagation?
> > > > > 
> > > > This is independent of count promotion. Usually the entry count is not promoted (unless it's inlined to a callsite inside a loop).
> > > > 
> > > > This code here is for the case when we instrument the entry BB. If the entry BB is instrumented, it cannot be computed from propagation.  (MST does not have the redundancy to recover)
> > > > 
> > > > Typically, it happens this way: for a func with a long running loop, after we reset the counter, the entry counters of this func are also zeroed out. If we dump the counters in the middle of the loop, the entry count will still be 0.
> > > > 
> > > > If we promote the counters inside the loop, the counter value inside the loop will be 0 (because the updates are in the exit block -- never exercised). If we disable the promote, the counter value insides the loop are OK.
> > > ok. what if entry is not instrumented, but two post-dom successors? If they are zeroed out, is there a way to recover?
> > Also look at the caller of this function -- even when AllZeros is true, this function is still called, so the entry count may be wrongly fixed up.
> It will not fix here, but later. When we set the entry count.
When AllZeros is set, the entry count we set here is NOOP.

The caller (annotateAllFunctions) has a shortcut when AllZero is 0.  It sets call setEntryCount directly (to 0) and skip counter population.

Maybe I should return early here if AllZero is 0.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1345
+  // Fix the obviously inconsistent entry count.
+  if (FuncMaxCount > 0 && FuncEntryCount == 0)
+    FuncEntryCount = 1;
----------------
davidxl wrote:
> Just this fix should be enough to fix the insanity, right?
This only fixes the entry for the func. It will not contribute to the count value population. 



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

https://reviews.llvm.org/D84378