[PATCH] D100993: [CSSPGO] Fix incorrect prorating indirect call distribution factor that leads to target count loss.

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:845
+/// \param SumOrigin  Original sum of target counts for indirect call.
+/// \param Sum        Prorated sum of target counts for indirect call.
 /// \param InlinedCallSite  Output vector for new call sites exposed after
----------------
nit: 

SumOrigin -> Original sum of target counts for indirect call before promoting given candidate.
Sum -> Sum of remaining target counts for indirect call after promoting given candidate.

There's no prorated sum - the Sum is adjust using subtraction 

> Sum -= Candidate.CallsiteCount;  


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:835
+  // excluding promoted targets.
+  if (FullSum)
+    if (Optional<PseudoProbe> Probe = extractProbe(Inst))
----------------
This makes sure that the count for the indirect call instruction accurately reflects the counts for remaining indirect call. However I thought later PGO ICP pass relies on the value profiles we put onto metadata, so even if the count for that icall instruction is a bit off, that ICP should not be affected. Is that correct?

Is the problem here more about pre-link sample loader ICP screw up distribution factor so post-link sample loader ICP sees a smaller count for the icall instruction which blocks the post-link sample loader ICP? In this case, if we look at call site target counts from profile, with promoted targets excluded, we would arrive at the correct sum even without distribution factor? Relying on distribution could get us the same result, but using prorating to mimic subtraction is a bit weird.. 

I can see how accurate distribution factor helps count quality in general though - it would make counts for icall more consistent before running inference. On that, how do we make sure promoted calls get correct counts for inference input though?


================
Comment at: llvm/test/Transforms/SampleProfile/pseudo-probe-icp-factor.ll:261
+;; factors, i.e, roughly 11259 * 0.95 * 0.79 = 8444.
+; CHECK: ![[#PROF]] = !{!"VP", i32 0, i64 8444, i64 7546896869197086323, i64 -1, i64 -2012135647395072713, i64 8444}
----------------
What was the value before this fix? Is that much smaller than 0.95? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100993