[PATCH] D135914: [PseudoProbe] Decode offset based pseudo probe.

Tue Oct 25 15:05:12 PDT 2022

hoy added a comment.

In D135914#3883493 <https://reviews.llvm.org/D135914#3883493>, @wenlei wrote:

> In D135914#3882912 <https://reviews.llvm.org/D135914#3882912>, @hoy wrote:
>
>> In D135914#3881607 <https://reviews.llvm.org/D135914#3881607>, @wenlei wrote:
>>
>>> Sentinel probe case isn't covered for both encoding and decoding in any test cases, would be good to have some coverage.
>>
>> Forgot to mention that the updated binary, i.e, inline-cs-pseudoprobe.perfbin, is built with the new encoding. The binary supports a dozen llvm-profgen tests and they all pass with the current decoding changes.
>
> Do we have a split function case in the existing tests? I thought we don't, but if we have, that'd be good enough.

Good point. The test binary func-split.perfbin is updated.

================
Comment at: llvm/lib/MC/MCPseudoProbe.cpp:476
+        // leading probe address or function start address.
+        EncodingIsAddrBased = true;
+      }
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > Maybe I'm asking the same question as the one on previous patch, but why do we still have cases with absolute address encoded? I thought for main function body we have GUID -> symbol (reverse lookup) -> address; and we have similar way to get address for sentinel for split binary function. The rest should all be offset/delta.
> > This is mainly for downwards-compatibility, i.e. to have new llvm-profgen handle old binaries. 
> Ok, makes sense. Like I mentioned in the other patch, add a comment explaining this is for backward compatibility and a todo for removal?
Done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135914