[PATCH] D93556: [CSSPGO][llvm-profgen] Compress recursive cycles in calling context

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/recursion-compression.test:1
+; RUN: llvm-profgen --perfscript=%S/Inputs/recursion-compression.perfscript --binary=%S/Inputs/recursion-compression.perfbin --output=%t --compress-recursion=-1 --show-unwinder-output | FileCheck %s --check-prefix=CHECK-UNWINDER
+; RUN: FileCheck %s --input-file %t
----------------
wmi wrote:
> Could you show the result of --compress-recursion=0 (I assume no compression will happen) so it is easy to check which part of the output is compressed for --compress-recursion=-1?
Good idea, added the uncompressed test case


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:446
+    const BranchSample &BranchCounter,
+    SmallVector<std::string, 16> &ContextStrStack, ProfiledBinary *Binary) {
   for (auto BI : BranchCounter) {
----------------
wmi wrote:
> Use SmallVectorImpl<std::string>& as a parameter type instead of SmallVector<std::string, 16>&. There are some other places with the same issue.
Yeah, thanks for the suggestions, fixed all the issues.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:497
 FunctionSamples &PseudoProbeCSProfileGenerator::getFunctionProfileForLeafProbe(
-    StringRef PrefixContextId, const PseudoProbe *LeafProbe,
+    SmallVector<std::string, 16> ContextStrStack, const PseudoProbe *LeafProbe,
     ProfiledBinary *Binary) {
----------------
wmi wrote:
> Nit: Change it to ContextStrStackCopy to make the vector copy more explicit.
Good catch, changed to use a new variable ContextStrStackCopy to make it more explicit.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:96-97
+      while (Right + I < Context.size()) {
+        // Find the first diverging point backwards. When stops, p points at
+        // the diverging point in the current sequence.
+        uint32_t Left = Right;
----------------
wmi wrote:
> Is p a variable or does it refer to some other variable? 
Sorry, I just miss to update the comments. 'p' is actually the `Left`. fixed the comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:108-110
+          // Populate the non-common-suffix part of the adjacent sequence.
+          std::copy(BeginIter + Right + 1, BeginIter + Left + I + 1,
+                    BeginIter + End);
----------------
wmi wrote:
> Could you give an example of what the sequence looks like after the population?
Sure, an example is added in the code comments


================
Comment at: llvm/unittests/tools/llvm-profgen/ContextCompressionTest.cpp:14
+
+TEST(TestCompression, TestNoSizeLimit) {
+  SmallVector<std::string, 16> Context = {"m", "a", "a", "b", "c", "a",
----------------
wmi wrote:
> How about have a similar test for pseudo probe?
Do you mean to add compression test for the element whose type is `PseudoProbe`?
The pseudo probe in the context key is represented as a `PseudoProbe` pointer, i.e an integer, so it should be same as the string. The only thing for the type T is implementing the `==` operator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93556