Skip to content
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

Jump to bottom

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

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 3 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 …
minglotus-6 Jul 17, 2024
85dccaf
fix line number
minglotus-6 Jul 17, 2024
a2ec8c4
wrap RUN lines and update remark line number accordingly
minglotus-6 Jul 18, 2024
36b85b8
resolve review feedback
minglotus-6 Jul 18, 2024
523ee22
compute the instrumented vtable sites from function IR if function pr…
minglotus-6 Jul 18, 2024
c6fdc19
resolve review feedback
minglotus-6 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
28 changes: 24 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,26 @@
// 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 `-enable-vtable-profile-use` is explicitly set to false to discard vtable
// value profiles.
// RUN: %clangxx -m64 -fprofile-use=test.profdata -fuse-ld=lld -O2 \
// RUN: -mllvm -enable-vtable-profile-use=false -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 +148,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:227: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:227: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:229: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:229: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
16 changes: 14 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,18 @@ 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);
// FuncPGOInstrumentation ctor finds value sites for each kind. It runs on the
// common path of pgo-instr-gen and pgo-instr-use, and vtable kind path
// is gated by `-enable-vtable-value-profiling`. If vtable profiles are
// present, not explicitly discarded and vtable sites remain empty, try to
// find the sites again.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't this just always be done here:

llvm-project/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

Lines 606 to 607 in d06b55e

if (EnableVTableValueProfiling)
ValueSites[IPVK_VTableTarget] = VPC.get(IPVK_VTableTarget);
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This runs on the common path of pgo-instr-gen and pgo-instr-run. There isn't a reliably way to tell if iFDO profile are generated with --enable-vtable-value-profiling or not [1], and it's preferred to have an option to disable VPC.get(IPVK_VTableTarget) (which iterates instructions and collect instrumentation sites via InstVisitor)

As discussed, I simplified this code by

  • Enable value-profile-collector (VPC) instruction iteration for optimized build.
  • Allow skipping VPC.get(IPVK_VTableTarget)

[1] if a program doesn't use vtable but its instrumented profiles are generated with --enable-vtable-value-profiling=true, iterating function profile record won't tell this with the current profile format.

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.

FuncInfo.ValueSites[Kind].empty() &&
!(EnableVTableProfileUse.getNumOccurrences() &&
EnableVTableProfileUse == false))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to depend on whether we are using it in the optimization passes? Won't we then get matching warnings if someone disables that?

Copy link
Contributor Author

@minglotus-6 minglotus-6 Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

EnableVTableProfileUse.getNumOccurrences() && EnableVTableProfileUse == false meant to allow skipping VPC.get(IPVK_VTableTarget). Now this PR uses -icp-max-num-vtables to do that.

While adding TODOs to speed up optimized build by skipping VPC.get(<value-profile-kind>) if there are no value profiles of this kind in function profile record, I realized VPC.get(<value-profile-kind>) for optimized build is necessary to compute function cfg hash with IR and detect profile mismatch.

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