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

Wei Mi via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 18 10:04:22 PST 2021


wmi 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;
----------------
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


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1201
+      Pair.first->second = Data.Count;
+    else if (Total > Data.Count)
+      Total -= Data.Count;
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96806



More information about the llvm-commits mailing list