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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:937
 // be added compressed while looking up function profile
 static void extractPrefixContextStack(
     SampleContextFrameVector &ContextStack,
----------------
Now remove this function? 


================
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.
----------------
hoy wrote:
> wenlei wrote:
> > CallProbe can also be null if the caller isn't profiled? Does the comment need to be updated here? 
> Comment updated.
nit: We may not find a probe for functions that are not sampled.  -> We may not find a probe for functions that are not sampled when --decode-probe-for-profiled-functions-only is on. 

also clearer if you list the three scenario with bullets as it now grow larger. 


================
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));
----------------
wenlei wrote:
> can we assert `!ProfiledFunctions.empty()` to make sure profile functions are collected already? 
missed this one? 


================
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");
----------------
wenlei wrote:
> 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. 
> The only difference would be - for full decoding, we just need to provide a full Guid set; and for on-demand, we provide ProfiledGuids.

missed this one? By only difference, I meant the only place that need to check `DecodeProbeForProfiledFunctionsOnly`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121643