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

[Dehao Chen via llvm-commits]

danielcdh added inline comments.

================
Comment at: include/llvm/ProfileData/SampleProf.h:202
@@ -162,1 +201,3 @@
+class FunctionSamples;
+typedef DenseMap<CallsiteLocation, FunctionSamples> CallsiteSampleMap;
 
----------------
dnovillo wrote:
> Move this typedef below. No need to forward declare FunctionSamples.
You mean move typedef inside the class? But the type is global and other modules actually uses this type.

================
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));
----------------
dnovillo wrote:
> Too similar to samplesAt().  Merge them?
This is a const member function, while samplesAt can modify member variables. Not sure how to merge them?

================
Comment at: include/llvm/ProfileData/SampleProf.h:262
@@ +261,3 @@
+  const FunctionSamples *
+  findFunctionSamplesAt(const CallsiteLocation &Loc) const {
+    auto iter = CallsiteSamples.find(Loc);
----------------
dnovillo wrote:
> 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.
Save as above. Want to make the function as const member function. I will have a separate patch to remove samplesAt sampleRecordAt and functionSamplesAt entirely, leaving only const member funcntions.

================
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
----------------
dnovillo wrote:
> 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.
Updated the document in this file. Will have a separate patch to update the clang doc bits.

================
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('.');
----------------
dnovillo wrote:
> 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?
That is what I initially tried to do. But I need to write new REs for the new profile format (with hierarchical profile format), spent 1+ hour designing REs, and finally gave up. So if we are gonna abandon the RE, then it does not make sense to spend effort to invent new RE for the new format.

================
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;
----------------
dnovillo wrote:
> Isn't it sufficient to mark these functions as hot or always_inline?  
No, we actually need to have inlining done before annotation.


http://reviews.llvm.org/D13145