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 5 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
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 `-max-num-vtable-annotaitons` is set to zero.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in. "annotaitons". However, this isn't the option used in the test below? It does seem to be what is used in the source code, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The option name is wrong. I corrected it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused though because the code in the PR checks MaxNumVTableAnnotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah this is because the option variable name is MaxNumVTableAnnotations but its registered string is icp-max-num-vtables.

llvm-project/llvm/lib/Analysis/IndirectCallPromotionAnalysis.cpp

Lines 48 to 50 in bee2654

cl::opt<unsigned> MaxNumVTableAnnotations(
"icp-max-num-vtables", cl::init(6), cl::Hidden,
cl::desc("Max number of vtables annotated for a vtable load instruction."));

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. I was thrown off by the naming difference.

// 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
20 changes: 18 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,22 @@ 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.
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