[PATCH] D28965: [PGO] Value profile for size of memory intrinsic calls

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

How to make the llvm-profdata 'show' command dump the range information as well?



================
Comment at: include/llvm/Transforms/InstrProfiling.h:63
+  // The start value of precise value profile range for memory intrinsic sizes.
+  const int64_t DefaultMemOPSizeRangeStart = 0;
+  int64_t MemOPSizeRangeStart;
----------------
should it be 1?


================
Comment at: include/llvm/Transforms/InstrProfiling.h:66
+  // The end value of precise value profile range for memory intrinsic sizes.
+  const int64_t DefaultMemOPSizeRangeLast = 8;
+  int64_t MemOPSizeRangeLast;
----------------
Why 8?  why not 64?


================
Comment at: lib/ProfileData/InstrProfReader.cpp:185
         uint64_t TakenCount, Value;
-        if (VK == IPVK_IndirectCallTarget) {
+        if (ValueKind == IPVK_IndirectCallTarget) {
           Symtab->addFuncName(VD.first);
----------------
Can you submit this separately as a bug fix?


================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:181
+  // Parse the value profile options.
+  MemOPSizeRangeStart = DefaultMemOPSizeRangeStart;
+  MemOPSizeRangeLast = DefaultMemOPSizeRangeLast;
----------------
extract this into an option parsing helper.


================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:546
+      if (Kind == IPVK_MemOPSize)
+        TotalNS += I * (MemOPSizeRangeLast + 1);
+    }
----------------
Larger range does not mean on average the number of values tracked will be proportionally larger. Perhaps using a different heuristic for allocation size? Besides, TotalNS tracks the number of value sites, overloading it for number of values does not make sense


================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:553
 
   uint64_t NumCounters = TotalNS * NumCountersPerValueSite;
 // Heuristic for small programs with very few total value sites.
----------------
Here is the counter # estimation.


================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1301
     Func.annotateIndirectCallSites();
+
     PGOUseFunc::FuncFreqAttr FreqAttr = Func.getFuncFreqAttr();
----------------
unrelated change.


================
Comment at: test/tools/llvm-profdata/memop-size-prof.proftext:3
+# RUN: llvm-profdata show -memop-sizes -counts -text -all-functions %s | FileCheck %s --check-prefix=MEMOP_TEXT
+# RUN: llvm-profdata merge -o %t.profdata  %s
+# RUN: llvm-profdata show -memop-sizes -all-functions %t.profdata | FileCheck %s --check-prefix=MEMOP --check-prefix=MEMOP_SUM
----------------
also  test 'merge' with -text option.


================
Comment at: test/tools/llvm-profdata/memop-size-prof.proftext:18
+# Num Value Kinds:
+1
+# ValueKind = IPVK_MemOPSize:
----------------
perhaps add a test with 2 value kinds


================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:468
   size_t ShownFunctions = 0;
   uint64_t TotalNumValueSites = 0;
   uint64_t TotalNumValueSitesWithValueProfile = 0;
----------------
TotalNumValueSitesICall

Or perhaps defined an array 

TotalNumValueSites[..] and use value kind index to access.


================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:469
   uint64_t TotalNumValueSites = 0;
   uint64_t TotalNumValueSitesWithValueProfile = 0;
   uint64_t TotalNumValues = 0;
----------------
TotalNumValuesSitesWithValueProfileICall


================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:472
   std::vector<unsigned> ICHistogram;
+  uint64_t MemOPSizeTotalNumValueSites = 0;
+  uint64_t MemOPSizeTotalNumValueSitesWithValueProfile = 0;
----------------
Make the var name consistent -- TotalNumValueSitesMemOpSize


================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:509
 
+      uint32_t MemOPSize = Func.getNumValueSites(IPVK_MemOPSize);
+      if (ShowMemOPSizes && MemOPSize > 0)
----------------
NumMemOpCalls


================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:511
+      if (ShowMemOPSizes && MemOPSize > 0)
+        OS << "    Memory Intrinsics Size: " << MemOPSize << "\n";
+
----------------
Number of memory intrinsic calls : 


================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:546
+      if (ShowMemOPSizes && MemOPSize > 0) {
+        uint32_t NS = Func.getNumValueSites(IPVK_MemOPSize);
+        OS << "    Memory Instrinsic Size Results:\n";
----------------
extract this into to a common helper function? There are many duplications. 


https://reviews.llvm.org/D28965