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

[Lei Wang via Phabricator via llvm-commits]

wlei added a comment.

In D92334#2488123 <https://reviews.llvm.org/D92334#2488123>, @wmi wrote:

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

Thanks for your feedbacks, that's really helpful. Trying to address them.



================
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;
----------------
wmi wrote:
> Nit: How about define a isDummyRoot() function in PseudoProbeInlineTree for it?
Good suggestion!
As it's like  DummyRoot --(Dummy inline site)--> OutlinedFunc, the condition is to check whether the OutlinedFunc's inline site is a dummy one. So I would like to change to the name `hasInlineSite` to indicate it has a real inline site not a dummy one.


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.cpp:61
+                                   bool ShowName) const {
+  postOrderExtractContext(InlineTree, ContextStack, GUID2FuncMAP, ShowName);
+}
----------------
wmi wrote:
> If it is traversing from leaf to root, why not just use a loop instead of a recursive function?
I see, using recursion is for avoiding the reversing operation, but I'm also aware that the loop is good for the code locality, changed to loop.


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.cpp:163
+
+  while (Data < End) {
+    uint64_t GUID = readUnencodedNumber<uint64_t>();
----------------
wmi wrote:
> Do we expect Data to be equal to End at the end of the loop? If that is true, better add an assertion. 
Good suggestion. It's supposed to consume all the data here, assertion added


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


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.h:50-53
+  // Inlinee function GUID
+  uint64_t GUID = 0;
+  // Inline site in inliner
+  InlineSite ISite;
----------------
wmi wrote:
> 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.
Yeah, sorry for the confusion, this is unclear.
The real logic will be like:

DummyRoot -->OutlinedFunc --> InlinedFunc

Added the inline site , it will be like:

DummyRoot --(Dummy inline site)--> OutlinedFunc --(inline site)--> inlinedFunc

Refined the comments.






================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.h:111
+      : Address(Ad), GUID(G), Index(I), Type(K), Attribute(At),
+        InlineTree(Tree){};
+
----------------
wmi wrote:
> 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?
As mentioned above, for the tree like `DummyRoot -->OutlinedFunc --> InlinedFunc`, the `Tree` variable here can be either OutlinedFunc or InlinedFunc. It shouldn't be null.

I'm not quite sure about `Indirect call` type, I thought it has been already deprecated here. @hoy Could you help to confirm this? If so, I will add the assertion later.


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.h:114
+  bool isEntry() const {
+    return Index == static_cast<uint32_t>(PseudoProbeReservedId::Last) + 1;
+  }
----------------
wmi wrote:
> 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;
Good point! fixed


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.h:162-163
+
+  // The root of the pseudo probe inline trie
+  PseudoProbeInlineTree InlineTreeRoot;
+
----------------
wmi wrote:
> 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.
more comments added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92334