[PATCH] D96806: [SampleFDO] Stop repeated indirect call promotion for the same target

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1201
+      Pair.first->second = Data.Count;
+    else if (Total > Data.Count)
+      Total -= Data.Count;
----------------
wmi wrote:
> wmi wrote:
> > hoy wrote:
> > > wmi wrote:
> > > > wmi wrote:
> > > > > wenlei wrote:
> > > > > > In what case do we have Total < Data.Count? And Total == Data.Count should be fine for subtraction too?
> > > > > Currently we won't have such case. It is just some extra protect in case of future change. I think assertion will be better. Will change it.
> > > > > 
> > > > > Right, Total == Data.Count should be added. will change it.
> > > > The assertion catches a case where we have Total < Data.Count when running the test test/Transforms/SampleProfile/csspgo-inline-icall.ll.
> > > > 
> > > > For CS profile, the sum computed in findIndirectCallFunctionSamples can be smaller than the sum of the target counts returned by findCallTargetMapAt. Is it expected?
> > > For CS profile, we use callsite count (including both inlined and non-inlined targets) instead of using entry counts, since there's yet a way to tell if an indirect call target is inlined or not from a CS profile. 
> > > 
> > > We have a fix for this to review. Haven't sent it out yet. It basically replaces this code
> > > 
> > > ```
> > >          uint64_t Sum = 0;
> > >           findIndirectCallFunctionSamples(I, Sum);
> > > ```
> > > 
> > > with
> > >           
> > > ```
> > > if (FunctionSamples::ProfileIsCS) {
> > >             // With CSSPGO all indirect call targets are counted towards the
> > >             // original indirect call site in the profile, including both
> > >             // inlined and non-inlined targets.
> > >             Sum = 0;
> > >             for (const auto &T_C : T.get())
> > >               Sum += T_C.second;
> > >           } else {
> > >             findIndirectCallFunctionSamples(I, Sum);
> > >           }
> > > ```
> > > 
> > > 
> > >  
> > findIndirectCallFunctionSamples
> Thanks. That fix makes sense to me. This change will depend on your fix if I keep the assumption Total >= Data.Count and the assertion. 
Sure, let me send out the fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96806