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

[Wei Mi via Phabricator via llvm-commits]

wmi marked 4 inline comments as done.
wmi added a comment.

> I would still remove PromotedInsns to keep it simple.

Yes, it is simpler. I removed them.



================
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)
----------------
wenlei wrote:
> Do we expect the same function to appear multiple times in ValueData? If not, we could exit as long as the candidate is found?
Good point. Changed.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1194
+    auto Pair = ValueCountMap.try_emplace(Data.Value, Data.Count);
+    if (Pair.second)
+      continue;
----------------
hoy wrote:
> Nit: group this if-statement and the one below?
I added some other statement after the statement below when I addressed Wenlei's comment so now it is clearer not to group them. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1198
+    // If it is 0, the call target has been promoted so keep it as 0.
+    if (Pair.first->second != 0)
+      Pair.first->second = Data.Count;
----------------
hoy wrote:
> Is a zero count automatically set when a target is promoted in a callee?
It is set by updateIDTMetaData with CallTargets containing the promoted target and count being set to 0. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1199
+    if (Pair.first->second != 0)
+      Pair.first->second = Data.Count;
+  }
----------------
wenlei wrote:
> 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?
Another good point. Changed.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96806