Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion llvm/include/llvm/ProfileData/InstrProf.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -465,7 +467,6 @@ class InstrProfSymtab {
StringSet<> VTableNames;
// A map from MD5 keys to function name strings.
std::vector<std::pair<uint64_t, StringRef>> MD5NameMap;

// A map from MD5 keys to function define. We only populate this map
// when build the Symtab from a Module.
std::vector<std::pair<uint64_t, Function *>> MD5FuncMap;
Expand Down
2 changes: 2 additions & 0 deletions llvm/lib/ProfileData/InstrProf.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1246,6 +1246,8 @@ void annotateValueSite(Module &M, Instruction &Inst,
ArrayRef<InstrProfValueData> VDs,
uint64_t Sum, InstrProfValueKind ValueKind,
uint32_t MaxMDCount) {
if (VDs.empty())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
LLVMContext &Ctx = M.getContext();
MDBuilder MDHelper(Ctx);
SmallVector<Metadata *, 3> Vals;
Expand Down
49 changes: 28 additions & 21 deletions llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand All @@ -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;
}
Expand Down