[PATCH] D102961: [CSSPGO][llvm-profgen] Report samples for untrackable frames.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/Inputs/truncated-pseudoprobe.ll:88
+  call void @llvm.pseudoprobe(i64 -2624081020897602054, i64 1, i32 0, i64 -1), !dbg !64
+  call void @foo(), !dbg !65
+  ret i32 0, !dbg !67
----------------
wenlei wrote:
> Is this the one missing probe id in discriminator? 
Yes. The discriminator field is intentionally set empty to mimid the real case.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:147
+      // Start a new traversal ignoring its bottom context
+      if (!Cur->isLeafFrame())
+        WithColor::warning() << "Untracked frame at "
----------------
wenlei wrote:
> Why do we need this check? Is it ok for leaf to not find an associated call probe?
> 
> I'm thinking that the warning can be moved into `ProbeStack::pushFrame`, so it can be more specific, and the message can be "Truncated context due to missing call probe." 
`isLeafFrame` here means the address is from LBR not the stack samples. So it doesn't refer to a function frame and it'll mostly have no call probe associated. 

Yes, moving the assert into `ProbeStack::pushFrame` sounds good.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:611
 
-private:
   BinarySampleCounterMap BinarySampleCounters;
----------------
wenlei wrote:
> is this removal intentional?
Yes, there's already a private preceding this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102961