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

minglotus-6 · 2024-07-17T23:13:18Z

Before this change, when file.profdata have vtable profiles but --enable-vtable-value-profiling is not on for optimized build, warnings from this line will show up. They are benign for performance but confusing.

It's better to automatically annotate vtable profiles if file.profdata has them. This change implements it in profile use pass.

If -icp-max-num-vtables is zero (default value is 6), vtable profiles won't be annotated.

…profiles and 'enable-vtable-value-profiling' is not explicitly off

llvmbot · 2024-07-18T04:44:31Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-pgo

Author: Mingming Liu (minglotus-6)

Changes

Before this change, when file.profdata have vtable profiles but --enable-vtable-value-profiling is not on for optimized build, warnings from this line will show up. They are benign for performance but confusing.

It's better to automatically annotate vtable profiles if file.profdata has them. This change implements it in pgo-instr-use pass.

If --enable-vtable-profile-use is explicitly set to false in the command line, vtable profiles won't be annotated.

Full diff: https://github.com/llvm/llvm-project/pull/99402.diff

2 Files Affected:

(modified) compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp (+24-4)
(modified) llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp (+14-2)

diff --git a/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp b/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
index 24a96a227393a..83ceb9b0cf556 100644
--- a/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
+++ b/compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp
@@ -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
@@ -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
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index 35b1bbf21be97..2baca05988808 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -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);
 
@@ -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);
 
@@ -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.
+  if (NumValueSites > 0 && Kind == IPVK_VTableTarget &&
+      FuncInfo.ValueSites[Kind].empty() &&
+      !(EnableVTableProfileUse.getNumOccurrences() &&
+        EnableVTableProfileUse == false))
+    FuncInfo.ValueSites[IPVK_VTableTarget] = VPC.get(IPVK_VTableTarget);
+  auto &ValueSites = FuncInfo.ValueSites[Kind];
   if (NumValueSites != ValueSites.size()) {
     auto &Ctx = M->getContext();
     Ctx.diagnose(DiagnosticInfoPGOProfile(

teresajohnson · 2024-07-18T17:03:07Z

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

+  // 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.


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);

?

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.

teresajohnson · 2024-07-18T17:03:55Z

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

+  if (NumValueSites > 0 && Kind == IPVK_VTableTarget &&
+      FuncInfo.ValueSites[Kind].empty() &&
+      !(EnableVTableProfileUse.getNumOccurrences() &&
+        EnableVTableProfileUse == false))


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?

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.

teresajohnson

lgtm with a suggestion below

teresajohnson · 2024-07-18T20:17:14Z

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

+      // Collect value sites for 'pgo-instr-use' pass if `icp-max-num-vtables`
+      // is not zero.
+      if (EnableVTableValueProfiling ||
+          (!CreateGlobalVar && MaxNumVTableAnnotations != 0))


Can you add a comment about why the !CreateGlobalVar check.

Sorry I didn't realize 36b85b8 would cause benign warnings when profile is generated in enable-vtable-value-profiling=false when we discussed earlier.

I updated the PR to conditionally compute the vtable value sites and annotate profiles iff function profile record has vtable value profiles.

Regarding rollout, it'd make sense to keep enable-vtable-value-profiling consistent in instrumentation and optimized build.

…ofile record has vtable profiles

teresajohnson · 2024-07-22T16:12:33Z

compiler-rt/test/profile/Linux/instrprof-vtable-value-prof.cpp

+// 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.


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.

The option name is wrong. I corrected it.

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

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."));

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

teresajohnson · 2024-07-22T16:13:13Z

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

+  // 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 &&


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

…e profile (llvm#99402) Before this change, when `file.profdata` have vtable profiles but `--enable-vtable-value-profiling` is not on for optimized build, warnings from this line [1] will show up. They are benign for performance but confusing. It's better to automatically annotate vtable profiles if `file.profdata` has them. This PR implements it in profile use pass. * If `-icp-max-num-vtables` is zero (default value is 6), vtable profiles won't be annotated. [1] https://github.com/llvm/llvm-project/blob/464d321ee8dde1eaf14b5537eaf030e6df513849/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp#L1762-L1768

…e profile (#99402) Summary: Before this change, when `file.profdata` have vtable profiles but `--enable-vtable-value-profiling` is not on for optimized build, warnings from this line [1] will show up. They are benign for performance but confusing. It's better to automatically annotate vtable profiles if `file.profdata` has them. This PR implements it in profile use pass. * If `-icp-max-num-vtables` is zero (default value is 6), vtable profiles won't be annotated. [1] https://github.com/llvm/llvm-project/blob/464d321ee8dde1eaf14b5537eaf030e6df513849/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp#L1762-L1768 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251294

[InstrPGO][TypeProf]Annotate vtable types if they are present in the …

73d6985

…profiles and 'enable-vtable-value-profiling' is not explicitly off

minglotus-6 force-pushed the pgoannotate branch from 6eaa917 to 92ca07d Compare July 18, 2024 04:20

fix line number

85dccaf

minglotus-6 force-pushed the pgoannotate branch from 92ca07d to 85dccaf Compare July 18, 2024 04:25

minglotus-6 changed the title ~~[InstrPGO][TypeProf]Annotate vtable types when they are present in iFDO profiles and vtable profile-use option is not explicitly off~~ [InstrPGO][TypeProf]Annotate vtable types when they are present in iFDO profiles Jul 18, 2024

wrap RUN lines and update remark line number accordingly

a2ec8c4

minglotus-6 changed the title ~~[InstrPGO][TypeProf]Annotate vtable types when they are present in iFDO profiles~~ [InstrPGO][TypeProf]Annotate vtable types when they are present Jul 18, 2024

minglotus-6 changed the title ~~[InstrPGO][TypeProf]Annotate vtable types when they are present~~ [InstrPGO][TypeProf]Annotate vtable types when they are present in the profile Jul 18, 2024

minglotus-6 requested review from teresajohnson, david-xl, snehasish and modiking July 18, 2024 04:43

minglotus-6 marked this pull request as ready for review July 18, 2024 04:44

llvmbot added compiler-rt PGO Profile Guided Optimizations llvm:transforms labels Jul 18, 2024

teresajohnson reviewed Jul 18, 2024

View reviewed changes

resolve review feedback

36b85b8

minglotus-6 requested a review from teresajohnson July 18, 2024 18:58

teresajohnson approved these changes Jul 18, 2024

View reviewed changes

compute the instrumented vtable sites from function IR if function pr…

523ee22

…ofile record has vtable profiles

minglotus-6 requested a review from teresajohnson July 18, 2024 22:07

teresajohnson reviewed Jul 22, 2024

View reviewed changes

resolve review feedback

c6fdc19

minglotus-6 requested a review from teresajohnson July 22, 2024 17:06

teresajohnson approved these changes Jul 22, 2024

View reviewed changes

minglotus-6 merged commit a634171 into llvm:main Jul 22, 2024
5 of 7 checks passed

minglotus-6 deleted the pgoannotate branch July 22, 2024 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

minglotus-6 commented Jul 17, 2024 •

edited

Loading

llvmbot commented Jul 18, 2024 •

edited

Loading

teresajohnson Jul 18, 2024

minglotus-6 Jul 18, 2024

teresajohnson Jul 18, 2024

minglotus-6 Jul 18, 2024 •

edited

Loading

teresajohnson left a comment

teresajohnson Jul 18, 2024

minglotus-6 Jul 18, 2024 •

edited

Loading

teresajohnson Jul 22, 2024

minglotus-6 Jul 22, 2024

teresajohnson Jul 22, 2024

minglotus-6 Jul 22, 2024

teresajohnson Jul 22, 2024

teresajohnson Jul 22, 2024

minglotus-6 Jul 22, 2024

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

	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."));

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

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

Conversation

minglotus-6 commented Jul 17, 2024 • edited Loading

llvmbot commented Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minglotus-6 Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

teresajohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minglotus-6 Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minglotus-6 commented Jul 17, 2024 •

edited

Loading

llvmbot commented Jul 18, 2024 •

edited

Loading

minglotus-6 Jul 18, 2024 •

edited

Loading

minglotus-6 Jul 18, 2024 •

edited

Loading