[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