[PATCH] D67561: [SampleFDO] minimize performance impact when profile-sample-accurate is enabled

[Wei Mi via Phabricator via llvm-commits]

wmi marked 4 inline comments as done.
wmi added inline comments.


================
Comment at: include/llvm/ProfileData/SampleProfReader.h:334
+  /// Get the pointer of name table which includes all the names used in
+  /// FunctionSamples.
+  virtual std::vector<StringRef> *getNameTable() { return nullptr; }
----------------
davidxl wrote:
> Make it clearer that it includes all the names that have samples either in outline instance or an inline instance. It is a subset of the symbol list of the binary.
Will fix. 


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:481
+/// inlining to happen for warm callsites and it is helpful for performance.
 static bool callsiteIsHot(const FunctionSamples *CallsiteFS,
+                          ProfileSummaryInfo *PSI, bool ProfSampleAccEnabled) {
----------------
davidxl wrote:
> Make this a member function to avoid passing the boolean parameter around.
Will fix.


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:1616
 
+  if (SampleProfileRecordCoverage || SampleProfileSampleCoverage)
+    CoverageTracker.setProfSampleAccurateEnabled();
----------------
davidxl wrote:
> what are these two flags?
They are used to emit a warning when certain ratio of profile records or samples are not used during annotation. callsiteIsHot is used when computing the total samples (Only samples in hot callsite will be considered in total). 


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:1792
+    // actually not be cold after current build.
+    StringRef CanonName = FunctionSamples::getCanonicalFnName(F);
+    if (NamesInProfile.count(CanonName))
----------------
davidxl wrote:
> If this is no symbol list, then it should not use this heuristic to be consistent with old behavior? 
I think it is beneficial to have the change when profile-sample-accurate is true no matter symbol list is there or not. symbol list is used to prevent new hot functions from being treated as cold.  NamesInProfile is used to prevent functions are mistakenly treated as cold because inlining difference between sampled binary and the IR after early inlining in current build. These two changes are orthogonal to each other and are both beneficial individually. 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67561/new/

https://reviews.llvm.org/D67561