Skip to content

[InstrPGO][TypeProf]Annotate vtable types when they are present in the profile #99402

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 6 commits into from
Jul 22, 2024
Merged

[InstrPGO][TypeProf]Annotate vtable types when they are present in the profile #99402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
73d6985
[InstrPGO][TypeProf]Annotate vtable types if they are present in the …
mingmingl-llvm Jul 17, 2024
85dccaf
fix line number
mingmingl-llvm Jul 17, 2024
a2ec8c4
wrap RUN lines and update remark line number accordingly
mingmingl-llvm Jul 18, 2024
36b85b8
resolve review feedback
mingmingl-llvm Jul 18, 2024
523ee22
compute the instrumented vtable sites from function IR if function pr…
mingmingl-llvm Jul 18, 2024
c6fdc19
resolve review feedback
mingmingl-llvm Jul 22, 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
27 changes: 23 additions & 4 deletions compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,25 @@
// ICTEXT: {{.*}}instrprof-vtable-value-prof.cpp;_ZTVN12_GLOBAL__N_18Derived2E:750
// ICTEXT: _ZTV8Derived1:250

// When vtable value profiles exist, pgo-instr-use pass should annotate them
// even if `-enable-vtable-value-profiling` is not explicitly on.
// RUN: %clangxx -m64 -fprofile-use=test.profdata -fuse-ld=lld -O2 \
// RUN: -mllvm -print-after=pgo-instr-use -mllvm -filter-print-funcs=main \
// RUN: -mllvm -print-module-scope %s 2>&1 | FileCheck %s --check-prefix=ANNOTATE

// ANNOTATE-NOT: Inconsistent number of value sites
// ANNOTATE: !{!"VP", i32 2

// When vtable value profiles exist, pgo-instr-use pass will not annotate them
// if `-icp-max-num-vtables` is set to zero.
// RUN: %clangxx -m64 -fprofile-use=test.profdata -fuse-ld=lld -O2 \
// RUN: -mllvm -icp-max-num-vtables=0 -mllvm -print-after=pgo-instr-use \
// RUN: -mllvm -filter-print-funcs=main -mllvm -print-module-scope %s 2>&1 | \
// RUN: FileCheck %s --check-prefix=OMIT

// OMIT: Inconsistent number of value sites
// OMIT-NOT: !{!"VP", i32 2

// Test indirect call promotion transformation using vtable profiles.
// - Build with `-g` to enable debug information.
// - In real world settings, ICP pass is disabled in prelink pipeline. In
Expand All @@ -128,12 +147,12 @@
// RUN: | FileCheck %s --check-prefixes=REMARK,IR --implicit-check-not="!VP"

// For the indirect call site `ptr->func`
// REMARK: instrprof-vtable-value-prof.cpp:205:19: Promote indirect call to _ZN12_GLOBAL__N_18Derived24funcEii with count 150 out of 200, sink 1 instruction(s) and compare 1 vtable(s): {_ZTVN12_GLOBAL__N_18Derived2E}
// REMARK: instrprof-vtable-value-prof.cpp:205:19: Promote indirect call to _ZN8Derived14funcEii with count 50 out of 50, sink 1 instruction(s) and compare 1 vtable(s): {_ZTV8Derived1}
// REMARK: instrprof-vtable-value-prof.cpp:226:19: Promote indirect call to _ZN12_GLOBAL__N_18Derived24funcEii with count 150 out of 200, sink 1 instruction(s) and compare 1 vtable(s): {_ZTVN12_GLOBAL__N_18Derived2E}
// REMARK: instrprof-vtable-value-prof.cpp:226:19: Promote indirect call to _ZN8Derived14funcEii with count 50 out of 50, sink 1 instruction(s) and compare 1 vtable(s): {_ZTV8Derived1}
//
// For the indirect call site `delete ptr`
// REMARK: instrprof-vtable-value-prof.cpp:207:5: Promote indirect call to _ZN12_GLOBAL__N_18Derived2D0Ev with count 750 out of 1000, sink 2 instruction(s) and compare 1 vtable(s): {_ZTVN12_GLOBAL__N_18Derived2E}
// REMARK: instrprof-vtable-value-prof.cpp:207:5: Promote indirect call to _ZN8Derived1D0Ev with count 250 out of 250, sink 2 instruction(s) and compare 1 vtable(s): {_ZTV8Derived1}
// REMARK: instrprof-vtable-value-prof.cpp:228:5: Promote indirect call to _ZN12_GLOBAL__N_18Derived2D0Ev with count 750 out of 1000, sink 2 instruction(s) and compare 1 vtable(s): {_ZTVN12_GLOBAL__N_18Derived2E}
// REMARK: instrprof-vtable-value-prof.cpp:228:5: Promote indirect call to _ZN8Derived1D0Ev with count 250 out of 250, sink 2 instruction(s) and compare 1 vtable(s): {_ZTV8Derived1}

// The IR matchers for indirect callsite `ptr->func`.
// IR-LABEL: @main
Expand Down
21 changes: 19 additions & 2 deletions llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1096,7 +1096,7 @@ class PGOUseFunc {
: F(Func), M(Modu), BFI(BFIin), PSI(PSI),
FuncInfo(Func, TLI, ComdatMembers, false, BPI, BFIin, IsCS,
InstrumentFuncEntry, HasSingleByteCoverage),
FreqAttr(FFA_Normal), IsCS(IsCS) {}
FreqAttr(FFA_Normal), IsCS(IsCS), VPC(Func, TLI) {}

void handleInstrProfError(Error Err, uint64_t MismatchedFuncSum);

Expand Down Expand Up @@ -1178,6 +1178,8 @@ class PGOUseFunc {
// Is to use the context sensitive profile.
bool IsCS;

ValueProfileCollector VPC;

// Find the Instrumented BB and set the value. Return false on error.
bool setInstrumentedCounts(const std::vector<uint64_t> &CountFromProfile);

Expand Down Expand Up @@ -1755,8 +1757,23 @@ void PGOUseFunc::annotateValueSites() {
void PGOUseFunc::annotateValueSites(uint32_t Kind) {
assert(Kind <= IPVK_Last);
unsigned ValueSiteIndex = 0;
auto &ValueSites = FuncInfo.ValueSites[Kind];

unsigned NumValueSites = ProfileRecord.getNumValueSites(Kind);

// Since there isn't a reliable or fast way for profile reader to tell if a
// profile is generated with `-enable-vtable-value-profiling` on, we run the
// value profile collector over the function IR to find the instrumented sites
// iff function profile records shows the number of instrumented vtable sites
// is not zero. Function cfg already takes the number of instrumented
// indirect call sites into account so it doesn't hash the number of
// instrumented vtables; as a side effect it makes it easier to enable
// profiling and profile use in two steps if needed.
// TODO: Remove this if/when -enable-vtable-value-profiling is on by default.
if (NumValueSites > 0 && Kind == IPVK_VTableTarget &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a TODO to remove this if/when -enable-vtable-value-profiling is on by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

NumValueSites != FuncInfo.ValueSites[IPVK_VTableTarget].size() &&
MaxNumVTableAnnotations != 0)
FuncInfo.ValueSites[IPVK_VTableTarget] = VPC.get(IPVK_VTableTarget);
auto &ValueSites = FuncInfo.ValueSites[Kind];
if (NumValueSites != ValueSites.size()) {
auto &Ctx = M->getContext();
Ctx.diagnose(DiagnosticInfoPGOProfile(
Expand Down
Loading