[PATCH] D135912: [PseudoProbe] Replace relocation with offset for entry probe.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D135912#3883462 <https://reviews.llvm.org/D135912#3883462>, @wenlei wrote:

>> For those don't, a sentinel probe is emitted for each of the binary functions with a different name from the source.
>
> Nit but it indeed caused confusion for me: if the definition of binary function is each MCSymbol, the above isn't accurate because we never add sentinel for the original function.

I see. Will just remove "Most of the source functions end up with only one binary function. For those don't, ".

> Also the term binary function is a bit confusing too. Intuitively a binary function has 1:1 mapping to source function, which is how BOLT defines binary function I believe. I also didn't find precedence in LLVM/MC where these funclets are called binary function. Can we make it straightforward and just call it split functions in code and comments?
>
> Otherwise this looks good. Thanks for working on relocation removal.

How about using code ranges instead of binary functions?



================
Comment at: llvm/include/llvm/MC/MCPseudoProbe.h:17-18
 // FUNCTION BODY (one for each outlined function present in the text section)
 //    GUID (uint64)
 //        GUID of the function
 //    NPROBES (ULEB128)
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > Have you considered removing this GUID field, but always require a sentinel probe for any (split) binary function including the main body? 
> > The GUID field here is the GUID of the source/dwarf name, which may be different from the actually symbol linkage name, even for the main body of the function.  The source GUID will be used to decode and generate a profile against the source function name.
> > 
> Ok, makes sense. Maybe clarify this in this comment section as well? 
Done.


================
Comment at: llvm/include/llvm/MC/MCPseudoProbe.h:32
 //          ADDRESS_TYPE (uint1)
-//            0 - code address, 1 - address delta
+//            0 - code address for regular probes
+//              - GUID of linkage name for sentinel probes
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > I thought we now have GUID for symbol name for sentinel, and address offset for non-sentinel. What does regular probe that doesn't use offset refer to?
> > This is mainly for downwards compatibility, i.e, to have the new llvm-profgen handle old binaries. Also Bolt re-encoding uses the old encoding scheme for simplicity. 
> How about adding a TODO comment to remove this later? 
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135912