[PATCH] D92334: [CSSPGO][llvm-profgen] Pseudo probe decoding and disassembling

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Sorry for the delay in review. I will try to catch up.



================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.cpp:42
+  // postorder traversal to make the context stack in caller-callee order
+  if (std::get<0>(Cur->ISite) == 0)
+    return;
----------------
Nit: How about define a isDummyRoot() function in PseudoProbeInlineTree for it?


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.cpp:61
+                                   bool ShowName) const {
+  postOrderExtractContext(InlineTree, ContextStack, GUID2FuncMAP, ShowName);
+}
----------------
If it is traversing from leaf to root, why not just use a loop instead of a recursive function?


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.cpp:163
+
+  while (Data < End) {
+    uint64_t GUID = readUnencodedNumber<uint64_t>();
----------------
Do we expect Data to be equal to End at the end of the loop? If that is true, better add an assertion. 


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.cpp:217
+  // A DFS-based decoding
+  while (Data < End) {
+    // Read inline site for inlinees
----------------
Same here. If it is expected that Data to be equal to End at the end of the loop, add an assertion.


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.h:50-53
+  // Inlinee function GUID
+  uint64_t GUID = 0;
+  // Inline site in inliner
+  InlineSite ISite;
----------------
PseudoProbeInlineTree will not just represent node of inlined function but also node of outlined function, right? It is better to make the comment a little more precise. Same suggestion for the comment before class PseudoProbeInlineTree.


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.h:111
+      : Address(Ad), GUID(G), Index(I), Type(K), Attribute(At),
+        InlineTree(Tree){};
+
----------------
I assume PseudoProbeType of PseudoProbe will be direct call if Tree is not null. Indirect call has to be promoted or devirtualized to direct call before it can be inlined. Does it make sense to add an assertion here?


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.h:114
+  bool isEntry() const {
+    return Index == static_cast<uint32_t>(PseudoProbeReservedId::Last) + 1;
+  }
----------------
Nit: It is a little bit clearer to have a definition and use it also in other place.
const uint32_t PseudoProbeFirstId = static_cast<uint32_t>(PseudoProbeReservedId::Last) + 1;


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.h:162-163
+
+  // The root of the pseudo probe inline trie
+  PseudoProbeInlineTree InlineTreeRoot;
+
----------------
This is a dummy node used to collect all the outlined functions in its Children field. Will be helpful to put it in the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92334