[PATCH] D13145: Change sample profile format to hierarchical using inline stack.

[Diego Novillo via llvm-commits]

dnovillo added inline comments.

================
Comment at: include/llvm/ProfileData/SampleProf.h:90
@@ +89,3 @@
+  StringRef CalleeName;
+};
+
----------------
Perhaps make CallsiteLocation inherit from LineLocation?  Or embed it.  I think inheriting from it may make better sense.

================
Comment at: include/llvm/ProfileData/SampleProf.h:202
@@ -162,1 +201,3 @@
+class FunctionSamples;
+typedef DenseMap<CallsiteLocation, FunctionSamples> CallsiteSampleMap;
 
----------------
Move this typedef below. No need to forward declare FunctionSamples.

================
Comment at: include/llvm/ProfileData/SampleProf.h:247
@@ +246,3 @@
+  /// If the location is not found in profile, return 0.
+  unsigned findSamplesAt(int LineOffset, unsigned Discriminator) const {
+    const auto &ret = BodySamples.find(LineLocation(LineOffset, Discriminator));
----------------
Too similar to samplesAt().  Merge them?

================
Comment at: include/llvm/ProfileData/SampleProf.h:262
@@ +261,3 @@
+  const FunctionSamples *
+  findFunctionSamplesAt(const CallsiteLocation &Loc) const {
+    auto iter = CallsiteSamples.find(Loc);
----------------
Likewise with functionSamplesAt.  Why not allow functionSamplesAt to return an empty FunctionSamples instance?  Seems better than dealing with potential segfaults because of the nullptrs.
 
To avoid unnecessary insertions, we can use ::find() in functionSamplesAt().  Similarly with samplesAt().

Having two functions with almost the same semantics is confusing.

================
Comment at: lib/ProfileData/SampleProfReader.cpp:37
@@ +36,3 @@
+// This is a nested tree in which the identation represent the nest level
+// of the inline stack. There is no blank line in the file. And the spacing
+// within a single line is fixed. Additional spaces will result in an error
----------------
Add a short explanation of what the 'inline stack' is.

Also, the user documentation in tools/clang/docs/UsersManual.rst will need to be updated.

================
Comment at: lib/ProfileData/SampleProfReader.cpp:208
@@ +207,3 @@
+  size_t n1 = Input.find(':');
+  StringRef Loc = Input.substr(Depth, n1 - Depth);
+  size_t n2 = Loc.find('.');
----------------
Can we split this in two changes?  The new parsing code is not completely related to the inline stack functionality.  While I agree that not using REs may be faster, it's not really something we care about.

Could you modify it so the switch out of RE-based parsing is done in a later patch?

================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:301
@@ +300,3 @@
+  if (!DIL) {
+    return NULL;
+  }
----------------
s/NULL/nullptr/

This happens in several places.

================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:368
@@ +367,3 @@
+/// \returns True if there is any inline happened.
+bool SampleProfileLoader::inlineHotFunctions(Function &F) {
+  bool Changed = false;
----------------
Isn't it sufficient to mark these functions as hot or always_inline?  


http://reviews.llvm.org/D13145