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

[Hongtao Yu via Phabricator via llvm-commits]

hoy 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.
----------------
Nit: overriden. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:769
+    if (Pair.first->second != NOMORE_ICP_MAGICNUM &&
+        Data.Count == NOMORE_ICP_MAGICNUM) {
+      OldSum -= Pair.first->second;
----------------
wmi wrote:
> hoy wrote:
> > IIUC, `Data` is from profile, why can it be -1?
> It is used to mark a target as being promoted (updateIDTMetaData is called in two places, one place to update the profile and another place to mark a target being promoted)
I see. 


================
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 &&
----------------
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?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97350