[SampleFDO] Another fix to prevent repeated indirect call promotion in

sample loader pass.

In https://reviews.llvm.org/rG5fb65c02ca5e91e7e1a00e0efdb8edc899f3e4b9,
to prevent repeated indirect call promotion for the same indirect call
and the same target, we used zero-count value profile to indicate an
indirect call has been promoted for a certain target. We removed
PromotedInsns cache in the same patch. However, there was a problem in
that patch described below, and that problem led me to add PromotedInsns
back as a mitigation in
https://reviews.llvm.org/rG4ffad1fb489f691825d6c7d78e1626de142f26cf.

When we get value profile from metadata by calling getValueProfDataFromInst,
we need to specify the maximum possible number of values we expect to read.
We uses MaxNumPromotions in the last patch so the maximum number of value
information extracted from metadata is MaxNumPromotions. If we have many
values including zero-count values when we write the metadata, some of them
will be dropped when we read them because we only read MaxNumPromotions
values. It will allow repeated indirect call promotion again. We need to
make sure if there are values indicating promoted targets, those values need
to be saved in metadata with higher priority than other values.

The patch fixed that problem. We change to use -1 to represent the count
of a promoted target instead of 0 so it is easier to sort the values.
When we prepare to update the metadata in updateIDTMetaData, we will sort
the values in the descending count order and extract only MaxNumPromotions
values to write into metadata. Since -1 is the max uint64_t number, if we
have equal to or less than MaxNumPromotions of -1 count values, they will
all be kept in metadata. If we have more than MaxNumPromotions of -1 count
values, we will only save MaxNumPromotions such values maximally. In such
case, we have logic in place in doesHistoryAllowICP to guarantee no more
promotion in sample loader pass will happen for the indirect call, because
it has been promoted enough.

With this change, now we can remove PromotedInsns without problem.

Differential Revision: https://reviews.llvm.org/D97350

Author: wmi-11

llvm/include/llvm/ProfileData/InstrProf.h

@@ -253,6 +253,10 @@ void annotateValueSite(Module &M, Instruction &Inst,
                        ArrayRef<InstrProfValueData> VDs, uint64_t Sum,
                        InstrProfValueKind ValueKind, uint32_t MaxMDCount);
 
+/// Magic number in the value profile data showing a target has been
+/// promoted for the instruction and shouldn't be promoted again.
+const uint64_t NOMORE_ICP_MAGICNUM = -1;
+
 /// Extract the value profile data from \p Inst which is annotated with
 /// value profile meta data. Return false if there is no value data annotated,
 /// otherwise  return true.
@@ -261,7 +265,7 @@ bool getValueProfDataFromInst(const Instruction &Inst,
                               uint32_t MaxNumValueData,
                               InstrProfValueData ValueData[],
                               uint32_t &ActualNumValueData, uint64_t &TotalC,
-                              bool GetZeroCntValue = false);
+                              bool GetNoICPValue = false);
 
 inline StringRef getPGOFuncNameMetadataName() { return "PGOFuncName"; }
 

llvm/lib/Analysis/IndirectCallPromotionAnalysis.cpp

@@ -45,7 +45,7 @@ static cl::opt<unsigned>
 
 // Set the maximum number of targets to promote for a single indirect-call
 // callsite.
-cl::opt<unsigned>
+static cl::opt<unsigned>
     MaxNumPromotions("icp-max-prom", cl::init(3), cl::Hidden, cl::ZeroOrMore,
                      cl::desc("Max number of promotions for a single indirect "
                               "call callsite"));

llvm/lib/ProfileData/InstrProf.cpp

@@ -988,7 +988,7 @@ bool getValueProfDataFromInst(const Instruction &Inst,
                               uint32_t MaxNumValueData,
                               InstrProfValueData ValueData[],
                               uint32_t &ActualNumValueData, uint64_t &TotalC,
-                              bool GetZeroCntValue) {
+                              bool GetNoICPValue) {
   MDNode *MD = Inst.getMetadata(LLVMContext::MD_prof);
   if (!MD)
     return false;
@@ -1015,7 +1015,7 @@ bool getValueProfDataFromInst(const Instruction &Inst,
 
   // Get total count
   ConstantInt *TotalCInt = mdconst::dyn_extract<ConstantInt>(MD->getOperand(2));
-  if (!TotalCInt && !GetZeroCntValue)
+  if (!TotalCInt)
     return false;
   TotalC = TotalCInt->getZExtValue();
 
@@ -1027,10 +1027,13 @@ bool getValueProfDataFromInst(const Instruction &Inst,
     ConstantInt *Value = mdconst::dyn_extract<ConstantInt>(MD->getOperand(I));
     ConstantInt *Count =
         mdconst::dyn_extract<ConstantInt>(MD->getOperand(I + 1));
-    if (!Value || (!Count && !GetZeroCntValue))
+    if (!Value || !Count)
       return false;
+    uint64_t CntValue = Count->getZExtValue();
+    if (!GetNoICPValue && (CntValue == NOMORE_ICP_MAGICNUM))
+      continue;
     ValueData[ActualNumValueData].Value = Value->getZExtValue();
-    ValueData[ActualNumValueData].Count = Count->getZExtValue();
+    ValueData[ActualNumValueData].Count = CntValue;
     ActualNumValueData++;
   }
   return true;

llvm/lib/Transforms/IPO/SampleProfile.cpp

@@ -218,7 +218,11 @@ static cl::opt<std::string> ProfileInlineReplayFile(
         "by inlining from sample profile loader."),
     cl::Hidden);
 
-extern cl::opt<unsigned> MaxNumPromotions;
+static cl::opt<unsigned>
+    MaxNumPromotions("sample-profile-icp-max-prom", cl::init(3), cl::Hidden,
+                     cl::ZeroOrMore,
+                     cl::desc("Max number of promotions for a single indirect "
+                              "call callsite in sample profile loader"));
 
 namespace {
 
@@ -364,8 +368,7 @@ class SampleProfileLoader final
   // Attempt to promote indirect call and also inline the promoted call
   bool tryPromoteAndInlineCandidate(
       Function &F, InlineCandidate &Candidate, uint64_t SumOrigin,
-      uint64_t &Sum, DenseSet<Instruction *> &PromotedInsns,
-      SmallVector<CallBase *, 8> *InlinedCallSites = nullptr);
+      uint64_t &Sum, SmallVector<CallBase *, 8> *InlinedCallSites = nullptr);
   bool inlineHotFunctions(Function &F,
                           DenseSet<GlobalValue::GUID> &InlinedGUIDs);
   InlineCost shouldInlineCandidate(InlineCandidate &Candidate);
@@ -696,43 +699,70 @@ SampleProfileLoader::findFunctionSamples(const Instruction &Inst) const {
   return it.first->second;
 }
 
-/// If the profile count for the promotion candidate \p Candidate is 0,
-/// it means \p Candidate has already been promoted for \p Inst.
-static bool isPromotedBefore(const Instruction &Inst, StringRef Candidate) {
+/// Check whether the indirect call promotion history of \p Inst allows
+/// the promotion for \p Candidate.
+/// If the profile count for the promotion candidate \p Candidate is
+/// NOMORE_ICP_MAGICNUM, it means \p Candidate has already been promoted
+/// for \p Inst. If we already have at least MaxNumPromotions
+/// NOMORE_ICP_MAGICNUM count values in the value profile of \p Inst, we
+/// cannot promote for \p Inst anymore.
+static bool doesHistoryAllowICP(const Instruction &Inst, StringRef Candidate) {
   uint32_t NumVals = 0;
   uint64_t TotalCount = 0;
   std::unique_ptr<InstrProfValueData[]> ValueData =
       std::make_unique<InstrProfValueData[]>(MaxNumPromotions);
   bool Valid =
       getValueProfDataFromInst(Inst, IPVK_IndirectCallTarget, MaxNumPromotions,
                                ValueData.get(), NumVals, TotalCount, true);
-  if (Valid) {
-    for (uint32_t I = 0; I < NumVals; I++) {
-      // If the promotion candidate has 0 count in the metadata, it
-      // means the candidate has been promoted for this indirect call.
-      if (ValueData[I].Value == Function::getGUID(Candidate))
-        return ValueData[I].Count == 0;
-    }
+  // No valid value profile so no promoted targets have been recorded
+  // before. Ok to do ICP.
+  if (!Valid)
+    return true;
+
+  unsigned NumPromoted = 0;
+  for (uint32_t I = 0; I < NumVals; I++) {
+    if (ValueData[I].Count != NOMORE_ICP_MAGICNUM)
+      continue;
+
+    // If the promotion candidate has NOMORE_ICP_MAGICNUM count in the
+    // metadata, it means the candidate has been promoted for this
+    // indirect call.
+    if (ValueData[I].Value == Function::getGUID(Candidate))
+      return false;
+    NumPromoted++;
+    // If already have MaxNumPromotions promotion, don't do it anymore.
+    if (NumPromoted == MaxNumPromotions)
+      return false;
   }
-  return false;
+  return true;
 }
 
-/// Update indirect call target profile metadata for \p Inst. If \p Total
-/// is given, set TotalCount of call targets counts to \p Total, otherwise
-/// keep the original value in metadata.
+/// Update indirect call target profile metadata for \p Inst.
+/// Usually \p Sum is the sum of counts of all the targets for \p Inst.
+/// If it is 0, it means updateIDTMetaData is used to mark a
+/// certain target to be promoted already. If it is not zero,
+/// we expect to use it to update the total count in the value profile.
 static void
 updateIDTMetaData(Instruction &Inst,
                   const SmallVectorImpl<InstrProfValueData> &CallTargets,
-                  uint64_t Total = 0) {
-  DenseMap<uint64_t, uint64_t> ValueCountMap;
+                  uint64_t Sum) {
+  assert((Sum != 0 || (CallTargets.size() == 1 &&
+                       CallTargets[0].Count == NOMORE_ICP_MAGICNUM)) &&
+         "If sum is 0, assume only one element in CallTargets with count "
+         "being NOMORE_ICP_MAGICNUM");
 
   uint32_t NumVals = 0;
-  uint64_t TotalCount = 0;
+  // OldSum is the existing total count in the value profile data.
+  // It will be replaced by Sum if Sum is not 0.
+  uint64_t OldSum = 0;
   std::unique_ptr<InstrProfValueData[]> ValueData =
       std::make_unique<InstrProfValueData[]>(MaxNumPromotions);
   bool Valid =
       getValueProfDataFromInst(Inst, IPVK_IndirectCallTarget, MaxNumPromotions,
-                               ValueData.get(), NumVals, TotalCount, true);
+                               ValueData.get(), NumVals, OldSum, true);
+
+  DenseMap<uint64_t, uint64_t> ValueCountMap;
+  // Initialize ValueCountMap with existing value profile data.
   if (Valid) {
     for (uint32_t I = 0; I < NumVals; I++)
       ValueCountMap[ValueData[I].Value] = ValueData[I].Count;
@@ -742,13 +772,24 @@ updateIDTMetaData(Instruction &Inst,
     auto Pair = ValueCountMap.try_emplace(Data.Value, Data.Count);
     if (Pair.second)
       continue;
-    // Update existing profile count of the call target if it is not 0.
-    // If it is 0, the call target has been promoted so keep it as 0.
-    if (Pair.first->second != 0)
+    // Whenever the count is NOMORE_ICP_MAGICNUM for a value, keep it
+    // in the ValueCountMap. If both the count in CallTargets and the
+    // count in ValueCountMap is not NOMORE_ICP_MAGICNUM, keep the
+    // count in CallTargets.
+    if (Pair.first->second != NOMORE_ICP_MAGICNUM &&
+        Data.Count == NOMORE_ICP_MAGICNUM) {
+      OldSum -= Pair.first->second;
+      Pair.first->second = NOMORE_ICP_MAGICNUM;
+    } else if (Pair.first->second == NOMORE_ICP_MAGICNUM &&
+               Data.Count != NOMORE_ICP_MAGICNUM) {
+      assert(Sum >= Data.Count && "Sum should never be less than Data.Count");
+      Sum -= Data.Count;
+    } else if (Pair.first->second != NOMORE_ICP_MAGICNUM &&
+               Data.Count != NOMORE_ICP_MAGICNUM) {
+      // Sum will be used in this case. Although the existing count
+      // for the current value in value profile will be overriden,
+      // no need to update OldSum.
       Pair.first->second = Data.Count;
-    else {
-      assert(Total >= Data.Count && "Total should be >= Data.Count");
-      Total -= Data.Count;
     }
   }
 
@@ -757,36 +798,38 @@ updateIDTMetaData(Instruction &Inst,
     NewCallTargets.emplace_back(
         InstrProfValueData{ValueCount.first, ValueCount.second});
   }
+
   llvm::sort(NewCallTargets,
              [](const InstrProfValueData &L, const InstrProfValueData &R) {
                if (L.Count != R.Count)
                  return L.Count > R.Count;
                return L.Value > R.Value;
              });
+
+  uint32_t MaxMDCount =
+      std::min(NewCallTargets.size(), static_cast<size_t>(MaxNumPromotions));
   annotateValueSite(*Inst.getParent()->getParent()->getParent(), Inst,
-                    NewCallTargets, Total ? Total : TotalCount,
-                    IPVK_IndirectCallTarget, NewCallTargets.size());
+                    NewCallTargets, Sum ? Sum : OldSum, IPVK_IndirectCallTarget,
+                    MaxMDCount);
 }
 
 /// Attempt to promote indirect call and also inline the promoted call.
 ///
 /// \param F  Caller function.
 /// \param Candidate  ICP and inline candidate.
 /// \param Sum  Sum of target counts for indirect call.
-/// \param PromotedInsns  Map to keep track of indirect call already processed.
 /// \param InlinedCallSite  Output vector for new call sites exposed after
 /// inlining.
 bool SampleProfileLoader::tryPromoteAndInlineCandidate(
     Function &F, InlineCandidate &Candidate, uint64_t SumOrigin, uint64_t &Sum,
-    DenseSet<Instruction *> &PromotedInsns,
     SmallVector<CallBase *, 8> *InlinedCallSite) {
   auto CalleeFunctionName = Candidate.CalleeSamples->getFuncName();
   auto R = SymbolMap.find(CalleeFunctionName);
   if (R == SymbolMap.end() || !R->getValue())
     return false;
 
   auto &CI = *Candidate.CallInstr;
-  if (isPromotedBefore(CI, R->getValue()->getName()))
+  if (!doesHistoryAllowICP(CI, R->getValue()->getName()))
     return false;
 
   const char *Reason = "Callee function not available";
@@ -799,11 +842,11 @@ bool SampleProfileLoader::tryPromoteAndInlineCandidate(
   if (!R->getValue()->isDeclaration() && R->getValue()->getSubprogram() &&
       R->getValue()->hasFnAttribute("use-sample-profile") &&
       R->getValue() != &F && isLegalToPromote(CI, R->getValue(), &Reason)) {
-    // For promoted target, save 0 count in the value profile metadata so
-    // the target won't be promoted again.
-    SmallVector<InstrProfValueData, 1> SortedCallTargets = {
-        InstrProfValueData{Function::getGUID(R->getValue()->getName()), 0}};
-    updateIDTMetaData(CI, SortedCallTargets);
+    // For promoted target, set its value with NOMORE_ICP_MAGICNUM count
+    // in the value profile metadata so the target won't be promoted again.
+    SmallVector<InstrProfValueData, 1> SortedCallTargets = {InstrProfValueData{
+        Function::getGUID(R->getValue()->getName()), NOMORE_ICP_MAGICNUM}};
+    updateIDTMetaData(CI, SortedCallTargets, 0);
 
     auto *DI = &pgo::promoteIndirectCall(
         CI, R->getValue(), Candidate.CallsiteCount, Sum, false, ORE);
@@ -817,7 +860,6 @@ bool SampleProfileLoader::tryPromoteAndInlineCandidate(
       // be prorated so that the it will reflect the real callsite counts.
       setProbeDistributionFactor(CI, Candidate.CallsiteDistribution * Sum /
                                          SumOrigin);
-      PromotedInsns.insert(Candidate.CallInstr);
       Candidate.CallInstr = DI;
       if (isa<CallInst>(DI) || isa<InvokeInst>(DI)) {
         bool Inlined = tryInlineCandidate(Candidate, InlinedCallSite);
@@ -890,8 +932,6 @@ void SampleProfileLoader::emitOptimizationRemarksForInlineCandidates(
 /// \returns True if there is any inline happened.
 bool SampleProfileLoader::inlineHotFunctions(
     Function &F, DenseSet<GlobalValue::GUID> &InlinedGUIDs) {
-  DenseSet<Instruction *> PromotedInsns;
-
   // ProfAccForSymsInList is used in callsiteIsHot. The assertion makes sure
   // Profile symbol list is ignored when profile-sample-accurate is on.
   assert((!ProfAccForSymsInList ||
@@ -945,8 +985,6 @@ bool SampleProfileLoader::inlineHotFunctions(
       if (CalledFunction == &F)
         continue;
       if (I->isIndirectCall()) {
-        if (PromotedInsns.count(I))
-          continue;
         uint64_t Sum;
         for (const auto *FS : findIndirectCallFunctionSamples(*I, Sum)) {
           uint64_t SumOrigin = Sum;
@@ -959,8 +997,7 @@ bool SampleProfileLoader::inlineHotFunctions(
             continue;
 
           Candidate = {I, FS, FS->getEntrySamples(), 1.0};
-          if (tryPromoteAndInlineCandidate(F, Candidate, SumOrigin, Sum,
-                                           PromotedInsns)) {
+          if (tryPromoteAndInlineCandidate(F, Candidate, SumOrigin, Sum)) {
             LocalNotInlinedCallSites.erase(I);
             LocalChanged = true;
           }
@@ -1169,7 +1206,6 @@ SampleProfileLoader::shouldInlineCandidate(InlineCandidate &Candidate) {
 
 bool SampleProfileLoader::inlineHotFunctionsWithPriority(
     Function &F, DenseSet<GlobalValue::GUID> &InlinedGUIDs) {
-  DenseSet<Instruction *> PromotedInsns;
   assert(ProfileIsCS && "Prioritiy based inliner only works with CSSPGO now");
 
   // ProfAccForSymsInList is used in callsiteIsHot. The assertion makes sure
@@ -1218,8 +1254,6 @@ bool SampleProfileLoader::inlineHotFunctionsWithPriority(
     if (CalledFunction == &F)
       continue;
     if (I->isIndirectCall()) {
-      if (PromotedInsns.count(I))
-        continue;
       uint64_t Sum;
       auto CalleeSamples = findIndirectCallFunctionSamples(*I, Sum);
       uint64_t SumOrigin = Sum;
@@ -1254,7 +1288,7 @@ bool SampleProfileLoader::inlineHotFunctionsWithPriority(
         Candidate = {I, FS, EntryCountDistributed,
                      Candidate.CallsiteDistribution};
         if (tryPromoteAndInlineCandidate(F, Candidate, SumOrigin, Sum,
-                                         PromotedInsns, &InlinedCallSites)) {
+                                         &InlinedCallSites)) {
           for (auto *CB : InlinedCallSites) {
             if (getInlineCandidate(&NewCandidate, CB))
               CQueue.emplace(NewCandidate);
@@ -1351,6 +1385,8 @@ void SampleProfileLoader::generateMDProfMetadata(Function &F) {
                 Sum += NameFS.second.getEntrySamples();
             }
           }
+          if (!Sum)
+            continue;
           updateIDTMetaData(I, SortedCallTargets, Sum);
         } else if (!isa<IntrinsicInst>(&I)) {
           I.setMetadata(LLVMContext::MD_prof,

llvm/test/Transforms/SampleProfile/Inputs/norepeated-icp-2.prof

@@ -0,0 +1,16 @@
+_Z3goov:5860:1
+ 1: 5279 _Z3foov:2000 _Z3barv:1000
+ 2: 5279 _Z3foov:2000 _Z3barv:1000
+ 3: 5279 _Z3foov:2000 _Z3barv:1000
+ 1: _Z3hoov:5860
+   1: 5000
+ 1: _Z3moov:5860
+   1: 5000
+ 2: _Z3hoov:5860
+   1: 5000
+ 2: _Z3moov:5860
+   1: 5000
+ 3: _Z3hoov:5860
+   1: 5000
+ 3: _Z3moov:5860
+   1: 5000

llvm/test/Transforms/SampleProfile/indirect-call.ll

@@ -202,7 +202,7 @@ attributes #0 = {"use-sample-profile"}
 ; CHECK: ![[PROF]] = !{!"VP", i32 0, i64 3457, i64 9191153033785521275, i64 2059, i64 -1069303473483922844, i64 1398}
 ; CHECK: ![[BR1]] = !{!"branch_weights", i32 4000, i32 4000}
 ; CHECK: ![[BR2]] = !{!"branch_weights", i32 3000, i32 1000}
-; CHECK: ![[VP]] = !{!"VP", i32 0, i64 8000, i64 -6391416044382067764, i64 1000, i64 7476224446746900038, i64 0, i64 925324185419832389, i64 0}
+; CHECK: ![[VP]] = !{!"VP", i32 0, i64 8000, i64 7476224446746900038, i64 -1, i64 925324185419832389, i64 -1, i64 -6391416044382067764, i64 1000}
 ; CHECK: ![[BR3]] = !{!"branch_weights", i32 1, i32 0}
 !6 = distinct !DISubprogram(name: "test_inline", scope: !1, file: !1, line: 6, unit: !0)
 !7 = !DILocation(line: 7, scope: !6)