[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
----------------
wenlei wrote:
> wlei wrote:
> > 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
> Would it be better to place the check for compressed form side by side with uncompressed form for readability?
Just tried but the problem here is the result is sorted by the total sample, if the order changed after compressed and merged, it couldn't properly match the line. So how about keep the current one or I can give some comments for readability?


================
Comment at: llvm/test/tools/llvm-profgen/recursion-compression.test:1
+; Firstly test uncompression(--compress-recursion=0)
+; RUN: llvm-profgen --perfscript=%S/Inputs/recursion-compression.perfscript --binary=%S/Inputs/recursion-compression.perfbin --output=%t --compress-recursion=0
----------------
wenlei wrote:
> A test case to cover line-based profile too? That goes through slightly different path for handling the inline context expansion, so e2e test would be good (doesn't have to be as through as the one for probe though).
Good catch, the line-based test added


================
Comment at: llvm/test/tools/llvm-profgen/recursion-compression.test:69
+; CHECK:  6: 1 fa:1
+ !CFGChecksum: 72617220756
+; CHECK: [main:2 @ foo:5 @ fa:8 @ fa:7 @ fb:5 @ fb:6 @ fa:8 @ fa:7 @ fb:6 @ fa]:6:2
----------------
wenlei wrote:
> nit: missed a CHECK?
Good catch!


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:96
+  template <typename T>
+  static void compressRecursionContext(SmallVectorImpl<T> &Context) {
+    compressRecursionContext<T>(Context, MaxCompressionSize == -1
----------------
wenlei wrote:
> We could remove this redirector, and just use default parameter? 
Yeah, good to know this, we can use `uint32_t CompressionSize = MaxCompressionSize` as the default parameter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93556