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

[Rong Xu via Phabricator via llvm-commits]

xur marked 10 inline comments as done.
xur added inline comments.


================
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;
----------------
davidxl wrote:
> should it be 1?
there are calls to memory intrinsics with size 0.  There are quite many actually.


================
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;
----------------
davidxl wrote:
> Why 8?  why not 64?
I think 8 is enough. In the optimization, I multi-version up to value of 8.
This is a tunable.  64 might be too big.


================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:546
+      if (Kind == IPVK_MemOPSize)
+        TotalNS += I * (MemOPSizeRangeLast + 1);
+    }
----------------
davidxl wrote:
> 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
I removed this change. Now MemOPSize profile uses the same heuristic as indirect-call profiling.


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


================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:468
   size_t ShownFunctions = 0;
   uint64_t TotalNumValueSites = 0;
   uint64_t TotalNumValueSitesWithValueProfile = 0;
----------------
davidxl wrote:
> TotalNumValueSitesICall
> 
> Or perhaps defined an array 
> 
> TotalNumValueSites[..] and use value kind index to access.
Used a vector here.


https://reviews.llvm.org/D28965