Skip to content

[TypeProf][PGO]Support skipping vtable comparisons for a class and its derived ones #110575

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.

Sign up for GitHub

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 5 commits into from
Oct 2, 2024
Merged

[TypeProf][PGO]Support skipping vtable comparisons for a class and its derived ones #110575

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bc43354
[TypeProf][PGO]Support specification of vtables for which type profil…
mingmingl-llvm Sep 30, 2024
2695641
resolve review feedback
mingmingl-llvm Oct 1, 2024
f14c6d5
1. use cl::list<std::string> for option type
mingmingl-llvm Oct 1, 2024
255edce
resolve review feedback
mingmingl-llvm Oct 1, 2024
28e2363
1. resolve review feedback 2. Update option description to mention the
mingmingl-llvm Oct 2, 2024
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
20 changes: 20 additions & 0 deletions llvm/include/llvm/Support/CommandLine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1208,6 +1208,26 @@ template <> class parser<char> : public basic_parser<char> {
void anchor() override;
};

//--------------------------------------------------

extern template class basic_parser<DenseSet<StringRef>>;

template <>
class parser<DenseSet<StringRef>> : public basic_parser<DenseSet<StringRef>> {
public:
parser(Option &O) : basic_parser(O) {}

// Return true on error.
bool parse(Option &, StringRef, StringRef Arg, DenseSet<StringRef> &Val);

StringRef getValueName() const override { return "DenseSet<StringRef>"; }

void printOptionDiff(const Option &O, const DenseSet<StringRef> &V,
OptVal Default, size_t GlobalWidth) const;

void anchor() override;
};

//--------------------------------------------------
// This collection of wrappers is the intermediary between class opt and class
// parser to handle all the template nastiness.
Expand Down
20 changes: 20 additions & 0 deletions llvm/lib/Support/CommandLine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ template class basic_parser<double>;
template class basic_parser<float>;
template class basic_parser<std::string>;
template class basic_parser<char>;
template class basic_parser<DenseSet<StringRef>>;

template class opt<unsigned>;
template class opt<int>;
Expand Down Expand Up @@ -93,6 +94,7 @@ void parser<double>::anchor() {}
void parser<float>::anchor() {}
void parser<std::string>::anchor() {}
void parser<char>::anchor() {}
void parser<DenseSet<StringRef>>::anchor() {}

//===----------------------------------------------------------------------===//

Expand Down Expand Up @@ -2060,6 +2062,24 @@ bool parser<float>::parse(Option &O, StringRef ArgName, StringRef Arg,
return false;
}

// parser<DenseSet<StringRef> implementation
//
void parser<DenseSet<StringRef>>::printOptionDiff(
const Option &O, const DenseSet<StringRef> &V,
OptionValue<DenseSet<StringRef>> D, size_t GlobalWidth) const {}

bool parser<DenseSet<StringRef>>::parse(Option &O, StringRef ArgName,
StringRef Arg,
Copy link

@snehasish snehasish Oct 1, 2024

Choose a reason for hiding this comment

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

Are you certain that the contents of StringRef Arg outlives the DenseSet returned? All the others above return the contents by value.

I would prefer a cl::optstd::string which is parsed and validated in the ICP pass. Is there an advantage to your proposed approach and are there any precedents in the codebase? IMO this option feels overly restrictive when rejecting duplicated strings from the user.

Copy link
Contributor Author

@mingmingl-llvm mingmingl-llvm Oct 1, 2024

Choose a reason for hiding this comment

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

Are you certain that the contents of StringRef Arg outlives the DenseSet returned? All the others above return the contents by value.

I took another look (at a couple of callsites of cl::ParseCommandLineOptions), and couldn't easily figure out if referenced strings remain alive for all passes. FrontendOptions.h and cc1as_main.cpp are two places which holds vector<std::string> for LLVM args. But IMO if cl::ParseCommandLineOptions (as a frequently-called function in LLVM and downstream repositories) doesn't require caller keep the argument contents alive, it's not a good idea for cl::opt to even use StringRef (users will have a hard time).

I updated to cl::list<std::string> to save a call to llvm::SplitStrings. And one interesting finding is that both cl::opt and cl::list supports internal or external storage [1], with internal storage as the default.

[1] https://llvm.org/docs/CommandLine.html#the-cl-list-class

DenseSet<StringRef> &Val) {
SmallVector<StringRef> StrRefs;
llvm::SplitString(Arg, StrRefs, ",");
for (const StringRef StrRef : StrRefs)
if (!Val.insert(StrRef).second)
return O.error(Arg + " has duplicated strings!");

return false;
}

// generic_parser_base implementation
//

Expand Down
29 changes: 28 additions & 1 deletion llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,14 @@ static cl::opt<int> ICPMaxNumVTableLastCandidate(
"icp-max-num-vtable-last-candidate", cl::init(1), cl::Hidden,
cl::desc("The maximum number of vtable for the last candidate."));

static cl::opt<DenseSet<StringRef>> ICPIgnoredBaseTypes(
Copy link
Contributor

@teresajohnson teresajohnson Oct 1, 2024

Choose a reason for hiding this comment

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

cl::list is the typical way of supporting an option that takes a list. Can you just use that instead of adding another cl::opt parser?

Copy link
Contributor Author

@mingmingl-llvm mingmingl-llvm Oct 1, 2024

Choose a reason for hiding this comment

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

I wasn't aware of cl::list (at least it didn't occur to me yesterday). Now used cl::list to save llvm::SplitStrings.

"icp-ignored-base-types", cl::Hidden, cl::init(DenseSet<StringRef>()),
cl::desc("A comma-separated list of mangled vtable names. Classes "
"specified by the vtables and their derived ones will not be "
"vtable-ICP'ed. Useful when the profiled types and actual types "
"in the optimized binary could be different due to profiling "
"limitations."));

namespace {

// The key is a vtable global variable, and the value is a map.
Expand Down Expand Up @@ -851,8 +859,27 @@ bool IndirectCallPromoter::isProfitableToCompareVTables(
LLVM_DEBUG(dbgs() << "\n");

uint64_t CandidateVTableCount = 0;
for (auto &[GUID, Count] : VTableGUIDAndCounts)
SmallVector<MDNode *, 2> Types;
Copy link

@snehasish snehasish Oct 1, 2024

Choose a reason for hiding this comment

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

We can move this to line L870 and drop the clear(). Since it's a small vector with only 2 pointers (on the stack) I don't expect changing this will affect performance.

Copy link
Contributor Author

@mingmingl-llvm mingmingl-llvm Oct 1, 2024

Choose a reason for hiding this comment

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

Agree that it's better to move this vector inside the loop. Done.

for (auto &[GUID, Count] : VTableGUIDAndCounts) {
CandidateVTableCount += Count;
auto *VTableVar = Symtab->getGlobalVariable(GUID);
Copy link
Contributor

@teresajohnson teresajohnson Oct 1, 2024

Choose a reason for hiding this comment

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

can we skip all the new handling when IgnoredBaseTypes is empty?

Copy link
Contributor Author

@mingmingl-llvm mingmingl-llvm Oct 2, 2024

Choose a reason for hiding this comment

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

This makes sense.

I took the liberty to use a helper function for the new handling, to simplify the control flow (e.g., continue, inner-for-loop and return) inside the outer loop.


assert(VTableVar &&
"VTableVar must exist for GUID in VTableGUIDAndCounts");

Types.clear();
VTableVar->getMetadata(LLVMContext::MD_type, Types);

const DenseSet<StringRef> &VTableSet = ICPIgnoredBaseTypes.getValue();
for (auto *Type : Types)
if (auto *TypeId = dyn_cast<MDString>(Type->getOperand(1).get()))
if (VTableSet.contains(TypeId->getString().str())) {
LLVM_DEBUG(dbgs()
<< " vtable profiles are known to be "
Copy link

@snehasish snehasish Oct 1, 2024

Choose a reason for hiding this comment

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

Update the comment? s/known to be unrepresentative/ignored by option ..../

Copy link
Contributor Author

@mingmingl-llvm mingmingl-llvm Oct 1, 2024

Choose a reason for hiding this comment

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

done.

"unrepresentative. Bail out vtable comparison.");
return false;
}
}

if (CandidateVTableCount < Candidate.Count * ICPVTablePercentageThreshold) {
LLVM_DEBUG(
Expand Down
1 change: 1 addition & 0 deletions llvm/test/Transforms/PGOProfile/icp_vtable_cmp.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

; RUN: opt < %s -passes='pgo-icall-prom' -pass-remarks=pgo-icall-prom -enable-vtable-profile-use -icp-max-num-vtable-last-candidate=2 -S 2>&1 | FileCheck %s --check-prefixes=VTABLE-COMMON,VTABLE-CMP
; RUN: opt < %s -passes='pgo-icall-prom' -pass-remarks=pgo-icall-prom -enable-vtable-profile-use -icp-max-num-vtable-last-candidate=1 -S 2>&1 | FileCheck %s --check-prefixes=VTABLE-COMMON,FUNC-CMP
; RUN: opt < %s -passes='pgo-icall-prom' -pass-remarks=pgo-icall-prom -enable-vtable-profile-use -icp-max-num-vtable-last-candidate=1 -icp-ignored-base-types='Base1,Derived3' -S 2>&1 | FileCheck %s --check-prefixes=VTABLE-COMMON,FUNC-CMP

target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
Expand Down