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

[Wei Mi via Phabricator via llvm-commits]

wmi marked 2 inline comments as done.
wmi added inline comments.


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:1012
   // Get total count
   ConstantInt *TotalCInt = mdconst::dyn_extract<ConstantInt>(MD->getOperand(2));
+  if (!TotalCInt)
----------------
davidxl wrote:
> Does total count include the magic num? It should not.
total count won't include the magic number. It is not got by adding all the value counts in the value profile, but precomputed before the value profile is written.  


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:713
   if (Valid) {
     for (uint32_t I = 0; I < NumVals; I++) {
+      if (ValueData[I].Count == NOMORE_ICP_MAGICNUM) {
----------------
wenlei wrote:
> bail out the loop if NumPromoted > MaxNumPromotions?
Good idea. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:732
                   const SmallVectorImpl<InstrProfValueData> &CallTargets,
-                  uint64_t Total = 0) {
+                  uint64_t Sum = 0) {
   DenseMap<uint64_t, uint64_t> ValueCountMap;
----------------
davidxl wrote:
> document 'Sum'  What is the difference to TotalCount?
Done. Also rename TotalCount to OldSum so it is clearer that is the old total count in value profile and will be replaced by Sum if Sum is not 0.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97350