[PATCH] D97350: [SampleFDO] Another fix to prevent repeated indirect call promotion in sample loader pass

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:784
+      // Sum will be used in this case. Although the existing count
+      // for the current value in value profile will be overrided,
+      // no need to update OldSum.
----------------
hoy wrote:
> Nit: overriden. 
Fixed.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:762
+               Data.Count != NOMORE_ICP_MAGICNUM) {
+      Sum -= Data.Count;
+    } else if (Pair.first->second != NOMORE_ICP_MAGICNUM &&
----------------
hoy wrote:
> wmi wrote:
> > hoy wrote:
> > > wmi wrote:
> > > > davidxl wrote:
> > > > > can it become negative?
> > > > It won't. Sum is got by adding up the counts of all the targets so it should never be less than Data.Count. I add an assertion for it. 
> > > Looks like `Sum` could be zero in one of the callsites of `updateIDTMetaData` where no value is passed and the default value 0 is used.
> > If Sum is zero, it is used to mark a specific target to be promoted already. In that case, CallTargets will only contain one element and the count of the element is NOMORE_ICP_MAGICNUM, so it won't take this branch.  
> > 
> > I will add an assertion to make the assumption explicit.
> > assert(Sum != 0 || (CallTargets.size() == 1 && CallTargets[0].Count == NOMORE_ICP_MAGICNUM));
> I see. Maybe don't give `Sum` a default value, instead, explicit pass the value 0 in one of the callsite?
> 
> 
Ok, done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97350