[PATCH] D121643: [llvm-profgen] On-demand pseudo probe decoding

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/MC/MCPseudoProbe.cpp:363
+    MCDecodedPseudoProbeInlineTree *Cur, uint64_t &LastAddr,
+    std::unordered_set<uint64_t> &Guids) {
   // The pseudo_probe section encodes an inline forest and each tree has a
----------------
wenlei wrote:
> `Guids` -> `GuildFilter` or `ProfiledProbeGuids`
`GuildFilter` sounds better.


================
Comment at: llvm/test/tools/llvm-profgen/noinline-cs-pseudoprobe-on-demand.test:18
 ; CHECK-NEXT: !CFGChecksum: 563088904013236
-; CHECK:[main:2 @ foo:8 @ bar]:30:15
+; CHECK:[foo:8 @ bar]:30:15
 ; CHECK-NEXT: 1: 15
----------------
wenlei wrote:
> This change is because main->foo call is not profiled, right? 
Yes, main is not profiled so the calling context is truncated.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:474
+
 struct ProbeStack {
   SmallVector<const MCDecodedPseudoProbe *, 16> Stack;
----------------
wenlei wrote:
> Should this and ProbeBasedCtxKey be removed? 
Yes, this is no longer needed.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:395-398
+  for (const auto &FS : ProfileMap) {
+    if (auto *Func = Binary->getBinaryFunction(FS.first.getName()))
+      ProfiledFunctions.insert(Func);
+  }
----------------
wenlei wrote:
> Why is this needed?
Oh, this should be a change for D121655. With llvm-profgen reading in the final profile directly, profiled functions should be extracted from the input ProfileMap. Let me remove it here.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:953
+  for (auto Addr : reverse(Addresses)) {
+    const MCDecodedPseudoProbe *CallProbe = Binary->getCallProbeForAddr(Addr);
+    // We may not find a probe for a merged or external callsite.
----------------
wenlei wrote:
> CallProbe can also be null if the caller isn't profiled? Does the comment need to be updated here? 
Comment updated.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:964
+  std::reverse(Probes.begin(), Probes.end());
+  extractPrefixContextStack(ContextStack, Probes, Binary);
+}
----------------
wenlei wrote:
> Inline this call here and move into the loop? there's no other calls to this callee now.  
Inlined.  Not merging the loops since they iteration in different direction, note that the std::reverse in between.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:58
+cl::opt<bool> OnDemandPseudoProbeDecoding(
+    "on-demand-pseudo-probe-decoding", cl::init(true), cl::ZeroOrMore,
+    cl::desc("Decode pseudo probe for profiled functions only."));
----------------
wenlei wrote:
> how about `-decode-probe-for-profiled-functions-only`?
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121643