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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> PromotedInsns can still be useful as a cache so it doesn't need to access metadata every loop iteration in inlineHotFunctions, so we may want to keep it in the end. Sorry about it.

I would still remove PromotedInsns to keep it simple. If we worry about build time in inlineHotFunctions loop, we can change it to a work list to avoid revisiting all call sites on each iteration, similar to InlineHotFunctionWithPriority does this. That will save more repeated work.

> The fix brings 0.1~0.2% performance on our search benchmark.

This is great - thanks for the fix. Without top-down order, the performance improvement would probably be larger.



================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1163
+      // means the candidate has been promoted for this indirect call.
+      if (ValueData[I].Value == Function::getGUID(Candidate) &&
+          ValueData[I].Count == 0)
----------------
Do we expect the same function to appear multiple times in ValueData? If not, we could exit as long as the candidate is found?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1199
+    if (Pair.first->second != 0)
+      Pair.first->second = Data.Count;
+  }
----------------
When we update a non-zero count to zero to represent promoted call, do we need to reduce the total as well to reflect the remaining count for the actual indirect call?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1250
+    SmallVector<InstrProfValueData, 1> SortedCallTargets = {
+        InstrProfValueData{Function::getGUID(R->getValue()->getName()), 0}};
+    updateIDTMetaData(CI, SortedCallTargets);
----------------
A comment here would be helpful too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96806