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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Some linter warning seems legit, please format/fix.



================
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
----------------
`Guids` -> `GuildFilter` or `ProfiledProbeGuids`


================
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
----------------
This change is because main->foo call is not profiled, right? 


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


================
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);
+  }
----------------
Why is this needed?


================
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.
----------------
CallProbe can also be null if the caller isn't profiled? Does the comment need to be updated here? 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:964
+  std::reverse(Probes.begin(), Probes.end());
+  extractPrefixContextStack(ContextStack, Probes, Binary);
+}
----------------
Inline this call here and move into the loop? there's no other calls to this callee now.  


================
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."));
----------------
how about `-decode-probe-for-profiled-functions-only`?


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:362
+  std::unordered_set<uint64_t> ProfiledGuids;
+  for (auto *F : ProfiledFunctions) {
+    ProfiledGuids.insert(Function::getGUID(F->FuncName));
----------------
can we assert `!ProfiledFunctions.empty()` to make sure profile functions are collected already? 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:382
               reinterpret_cast<const uint8_t *>(Contents.data()),
-              Contents.size()))
+              Contents.size(), ProfiledGuids))
         exitWithError("Pseudo Probe decoder fail in .pseudo_probe section");
----------------
How about completely unify the workflow of on-demand vs full decoding? 

The only difference would be - for full decoding, we just need to provide a full Guid set; and for on-demand, we provide ProfiledGuids. 

Correct me if I'm wrong, but I think this was we keep the same behavior for full decoding as of today (no context truncation before preinliner), and we can also remove ProbeStack and replace it with AddressStack everywhere. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121643