[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:200
@@ -162,1 +199,3 @@
+class FunctionSamples;
+typedef DenseMap<CallsiteLocation, FunctionSamples> CallsiteSampleMap;
 
----------------
I meant moving it after FunctionSamples's definition, but I now see that it makes no sense.  This map is used inside FunctionSamples.  Ignore me :)

================
Comment at: include/llvm/ProfileData/SampleProf.h:260
@@ +259,3 @@
+  const FunctionSamples *
+  findFunctionSamplesAt(const CallsiteLocation &Loc) const {
+    auto iter = CallsiteSamples.find(Loc);
----------------
Ah, good.  Yeah, your plan makes perfect sense.

================
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
----------------
SGTM.

================
Comment at: lib/ProfileData/SampleProfReader.cpp:41
@@ +40,3 @@
+//
+// Inline stack is a stack of source locations in which the tip of the stack
+// represents the leaf function, and the bottom of the stack represents the
----------------
s/tip/top/

================
Comment at: lib/ProfileData/SampleProfReader.cpp:43
@@ -36,1 +42,3 @@
+// represents the leaf function, and the bottom of the stack represents the
+// actual symol in which the instruction belongs.
 //
----------------
r/symol in/symbol to/

================
Comment at: lib/ProfileData/SampleProfReader.cpp:212
@@ +211,3 @@
+  size_t n1 = Input.find(':');
+  StringRef Loc = Input.substr(Depth, n1 - Depth);
+  size_t n2 = Loc.find('.');
----------------
Ah, OK.  Yeah, REs are sometimes annoying to define.  No worries then.  We can refine this later.  I've no problems with the code as you wrote it.

================
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;
----------------
Oh, yes, we discussed this before.  The way propagation works now, we need to have the interesting inline decisions applied to align the block weights properly.

I think we will want to revisit this at some point.  It would be nice if we can avoid doing the inliner's job here and still deal with the weights properly in the propagator.

Could you add a TODO here?  Thanks.

================
Comment at: test/Transforms/SampleProfile/Inputs/calls.prof:8
@@ +7,2 @@
+ 3: 5391
+ 3.1: 5752 _Z3sumii:5860
----------------
Could you leave the original comment in?  Useful for documentation.

================
Comment at: test/Transforms/SampleProfile/syntax.ll:8
@@ -7,3 +7,3 @@
 ; RUN: not opt < %s -sample-profile -sample-profile-file=%S/Inputs/bad_samples.prof 2>&1 | FileCheck -check-prefix=BAD-SAMPLES %s
-; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/bad_mangle.prof 2>&1 >/dev/null
+; RUN: not opt < %s -sample-profile -sample-profile-file=%S/Inputs/bad_mangle.prof 2>&1 | FileCheck -check-prefix=BAD-MANGLE %s
 
----------------
Hm, not sure about losing this capability of tolerating badly mangled functions. I specifically added this because the optimized binaries sometimes don't have properly mangled symbols. The thinking was to be more tolerant of this.

See the comment in SampleProfileReaderText::read().  If we are really going to not tolerate this anymore, could you please make it part of a different patch?  This will break internal tests.


http://reviews.llvm.org/D13145