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

Tue Dec 8 17:00:43 PST 2020

wmi added a comment.

In D91878#2441363 <https://reviews.llvm.org/D91878#2441363>, @hoy wrote:

> In D91878#2440875 <https://reviews.llvm.org/D91878#2440875>, @wmi wrote:
>
>> 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.
>
> Thanks for reviewing this patch!
>
> Regarding the binary size, an object file will likely have the same size inflation with an executable since the size of the pseudo probe sections are propositional to the text section. If that is an issue to the linker, we can also perform fission to the pseudo probe data. Pseudo probe encoding is currently done on function (comdat) level (see `MCPseudoProbeSection::Emit` in MCPseudoProbe.cpp`), which means a function always gets a separate real absolute probe encoded in the first place followed by relative address delta encoded for subsequent probes. Therefore fission on function level should be fine. Fission on block level won't work, as you pointed out. Would function-level fission work for you?

Yes, function-level fission works for us. Adding that support later will be helpful.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91878