[PATCH] D99787: [CSSPGO] Fix incorrect probe distribution factor computation in top-down inliner

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1327
       auto CalleeSamples = findIndirectCallFunctionSamples(*I, Sum);
       uint64_t SumOrigin = Sum;
       Sum *= Candidate.CallsiteDistribution;
----------------
wenlei wrote:
> hoy wrote:
> > Thanks for the fix! I'm wondering if the fix should be made here by scaling down `SumOrigin` as well. `SumOrigin` is used to guide ICP and we are checking the scaled count against the original count below (line 1342). This could cause a promotable indirect callsite not promoted after the callsite is duplicated. E.g, an indirect callsite with only one target is duplicated in prelink with even distribution. And in postlink, each of the callsite has a target that counts 50% of the original counts. If the original callsite is supposed to be promoted, once duplicated, it may not get promoted in postlink because of the lower weight (50% vs original 100%) in either place.
> It's arguable as to what is more desirable, if a call site is duplicated. If we scale down SumOrigin, the ICP logic would be more consistent between when there's duplication vs not. But on the other hand, we might indeed want to penalize duplicated call sites because ICP (and inline) for all of them has larger cost that is not taken into account. I think that's more of matter of tuning, but the current fix in the patch is addressing something clearly wrong. We would need some perf testing if we're making the change of scaling down SumOrigin. 
Agreed. Numbers are needed to validate a change scaling down SumOrigin. Current change looks good to me.

@wlei Can you please add a test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99787