[PATCH] D91878: [CSSPGO] Pseudo probe encoding and emission.

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Sorry for the delay in review. Some general questions.

I remember Wenlei mentioned in the RFC discussion that the overall binary size increase from pseudo probe is ~12% if CFG encoding is added. Is the size increase for object file from pseudo probe about the same as binary? I am asking because the total size of linker input matters for us. Previously we may use fission to separate the debug information from object file so it won't be accounted as linker input.

According to the description, The address range will be split into multiple ranges by the address of pseudo probe. If some BB somehow doesn't have block type pseudo probe, the addresses in the BB will be mistakenly associated with the pseudo probe located before it in the binary. I don't know whether you saw cases like that. A random thought is we may generate some fake pseudo probes for the BBs which don't have them, as a mark of the BB boundary.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/PseudoProbePrinter.cpp:51
+  SmallVector<InlineSite, 8> InlineStack;
+  // When it's done InlineStack look like ([66, B], [88, A])
+  // which means, Function A inlines function B at line 88, and B inlines C at
----------------
Is the comment up-to-date? Looks like the InlineStack only contains guid and probeid?


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:2135-2136
+     << Attr;
+  // Emit inline stack like
+  //  @ GUIDDirectCaller:11 @ GUIDCaller:1 @ GUIDmain:3
+  if (!InlineStack.empty()) {
----------------
It is a reverse of sequence as the context string in the profile. Is it intentional? 


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:2137
+  //  @ GUIDDirectCaller:11 @ GUIDCaller:1 @ GUIDmain:3
+  if (!InlineStack.empty()) {
+    for (auto &Site : InlineStack) {
----------------
Is it unnecessary?


================
Comment at: llvm/lib/MC/MCPseudoProbe.cpp:92-95
+  // When it comes here, the input look like:
+  //    Probe: GUID of C, ...
+  //    InlineStack: [66, B], [88, A]
+  // which means, Function A inlines function B at line 88, and B inlines C at
----------------
I think inline stack only contains guids and pseudo probe ids but here the example mentions line numbers in the stack. I understand probe id is extracted from the discriminator in the debug Location but it will be confusing if people don't know about it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91878