[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:769
+    if (Pair.first->second != NOMORE_ICP_MAGICNUM &&
+        Data.Count == NOMORE_ICP_MAGICNUM) {
+      OldSum -= Pair.first->second;
----------------
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)


================
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:
> > 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));


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97350