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

[Wei Mi via Phabricator via llvm-commits]

wmi 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
----------------
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?


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


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:470
 FunctionSamples &PseudoProbeCSProfileGenerator::getFunctionProfileForLeaf(
-    StringRef PrefixContextId, SmallVector<std::string, 16> &LeafInlinedContext,
+    SmallVector<std::string, 16> &ContextStrStack,
     const PseudoProbeFuncDesc *LeafFuncDesc) {
----------------
Use SmallVectorImpl<std::string>& as a parameter type instead of SmallVector<std::string, 16>&. 


================
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) {
----------------
Nit: Change it to ContextStrStackCopy to make the vector copy 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;
----------------
Is p a variable or does it refer to some other variable? 


================
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);
----------------
Could you give an example of what the sequence looks like after the population?


================
Comment at: llvm/unittests/tools/llvm-profgen/ContextCompressionTest.cpp:14
+
+TEST(TestCompression, TestNoSizeLimit) {
+  SmallVector<std::string, 16> Context = {"m", "a", "a", "b", "c", "a",
----------------
How about have a similar test for pseudo probe?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93556