[NFC][IndirectCallProm] Refactor function-based conditional devirtualization and indirect call value profile update into one helper function

[minglotus-6]

• The motivation is to move indirect callee profile update inside the function-based speculative indirect-call promotion, so that there are fewer diffs the vtable-based transformation and profile update is implemented in a follow-up patch.
• The Parent patch is [ThinLTO][TypeProf] Implement vtable def import #79381

[bwendling]

Please add a title and description.

[minglotus-6]

Please add a title and description.

Done. I used a (temporary) short (and undescriptive) title to work around a lengthy branch name in spr-managed branches. More details on why this happens is elaborated in this discourse thread.

I'll keep it a draft for now and send this out together with a subsequent pull request which looks cleaner with this NFC one.

[teresajohnson]

Why this change?

[teresajohnson]

Oh, is this because you no longer check the "NumPromoted == NumVals" case?

[minglotus-6]

yes, the check NumPromoted == NumVals is removed so NumVals doesn't need to be passed as an argument to tryToPromoteWithFuncCmp.

• NumVals == ICallProfDataRef.size() is always true, and NumPromoted <= NumVals is also true, so ICallProfDataRef.slice(NumPromoted) still returns a valid ArrayRef after the check is removed.

On a second thought, when if(TotalCount != 0) is true, it's also guaranteed ICallProfDataRef.slice(NumPromoted) is not empty so that ICallProfDataRef.slice(NumPromoted) returns a valid ArrayRef. Added an assert (assert(NumPromoted <= ICallProfDataRef.size()) just to make the condition that .slice relies on more explicit.

[teresajohnson]

Just return false early if NumPromoted is 0 (then Changed can be removed and just return true at the end further below).

[minglotus-6]

done.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-pgo

Author: Mingming Liu (minglotus-6)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/80762.diff

3 Files Affected:

• (modified) llvm/include/llvm/ProfileData/InstrProf.h (+2)
• (modified) llvm/lib/ProfileData/InstrProf.cpp (+2)
• (modified) llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp (+28-21)

diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h
index 248f62c7a81059..6cdceae5eeb960 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -277,6 +277,8 @@ void annotateValueSite(Module &M, Instruction &Inst,
                        uint32_t MaxMDCount = 3);
 
 /// Same as the above interface but using an ArrayRef, as well as \p Sum.
+/// This function will not annotate !prof metadata on the instruction if the
+/// referenced array is empty.
 void annotateValueSite(Module &M, Instruction &Inst,
                        ArrayRef<InstrProfValueData> VDs, uint64_t Sum,
                        InstrProfValueKind ValueKind, uint32_t MaxMDCount);
diff --git a/llvm/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp
index 1d8b0a1aca95f2..91e79e8b2e9add 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -1260,6 +1260,8 @@ void annotateValueSite(Module &M, Instruction &Inst,
                        ArrayRef<InstrProfValueData> VDs,
                        uint64_t Sum, InstrProfValueKind ValueKind,
                        uint32_t MaxMDCount) {
+  if (VDs.empty())
+    return;
   LLVMContext &Ctx = M.getContext();
   MDBuilder MDHelper(Ctx);
   SmallVector<Metadata *, 3> Vals;
diff --git a/llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp b/llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp
index 7344fea1751719..23a7c6a20aecbc 100644
--- a/llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp
+++ b/llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp
@@ -136,11 +136,13 @@ class IndirectCallPromoter {
       const CallBase &CB, const ArrayRef<InstrProfValueData> &ValueDataRef,
       uint64_t TotalCount, uint32_t NumCandidates);
 
-  // Promote a list of targets for one indirect-call callsite. Return
-  // the number of promotions.
-  uint32_t tryToPromote(CallBase &CB,
-                        const std::vector<PromotionCandidate> &Candidates,
-                        uint64_t &TotalCount);
+  // Promote a list of targets for one indirect-call callsite by comparing
+  // indirect callee with functions. Returns true if there are IR
+  // transformations and false otherwise.
+  bool tryToPromoteWithFuncCmp(
+      CallBase &CB, const std::vector<PromotionCandidate> &Candidates,
+      uint64_t TotalCount, ArrayRef<InstrProfValueData> ICallProfDataRef,
+      uint32_t NumCandidates);
 
 public:
   IndirectCallPromoter(Function &Func, InstrProfSymtab *Symtab, bool SamplePGO,
@@ -273,9 +275,10 @@ CallBase &llvm::pgo::promoteIndirectCall(CallBase &CB, Function *DirectCallee,
 }
 
 // Promote indirect-call to conditional direct-call for one callsite.
-uint32_t IndirectCallPromoter::tryToPromote(
+bool IndirectCallPromoter::tryToPromoteWithFuncCmp(
     CallBase &CB, const std::vector<PromotionCandidate> &Candidates,
-    uint64_t &TotalCount) {
+    uint64_t TotalCount, ArrayRef<InstrProfValueData> ICallProfDataRef,
+    uint32_t NumCandidates) {
   uint32_t NumPromoted = 0;
 
   for (const auto &C : Candidates) {
@@ -287,7 +290,22 @@ uint32_t IndirectCallPromoter::tryToPromote(
     NumOfPGOICallPromotion++;
     NumPromoted++;
   }
-  return NumPromoted;
+
+  if (NumPromoted == 0)
+    return false;
+
+  // Adjust the MD.prof metadata. First delete the old one.
+  CB.setMetadata(LLVMContext::MD_prof, nullptr);
+
+  assert(NumPromoted <= ICallProfDataRef.size() &&
+         "Number of promoted functions should not be greater than the number "
+         "of values in profile metadata");
+  // Annotate the remaining value profiles if counter is not zero.
+  if (TotalCount != 0)
+    annotateValueSite(*F.getParent(), CB, ICallProfDataRef.slice(NumPromoted),
+                      TotalCount, IPVK_IndirectCallTarget, NumCandidates);
+
+  return true;
 }
 
 // Traverse all the indirect-call callsite and get the value profile
@@ -305,19 +323,8 @@ bool IndirectCallPromoter::processFunction(ProfileSummaryInfo *PSI) {
       continue;
     auto PromotionCandidates = getPromotionCandidatesForCallSite(
         *CB, ICallProfDataRef, TotalCount, NumCandidates);
-    uint32_t NumPromoted = tryToPromote(*CB, PromotionCandidates, TotalCount);
-    if (NumPromoted == 0)
-      continue;
-
-    Changed = true;
-    // Adjust the MD.prof metadata. First delete the old one.
-    CB->setMetadata(LLVMContext::MD_prof, nullptr);
-    // If all promoted, we don't need the MD.prof metadata.
-    if (TotalCount == 0 || NumPromoted == NumVals)
-      continue;
-    // Otherwise we need update with the un-promoted records back.
-    annotateValueSite(*F.getParent(), *CB, ICallProfDataRef.slice(NumPromoted),
-                      TotalCount, IPVK_IndirectCallTarget, NumCandidates);
+    Changed |= tryToPromoteWithFuncCmp(*CB, PromotionCandidates, TotalCount,
+                                       ICallProfDataRef, NumCandidates);
   }
   return Changed;
 }

[minglotus-6]

Ran 'git merge main' and resolved the merge conflicts