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

[Hongtao Yu via Phabricator via llvm-commits]

hoy marked 5 inline comments as done.
hoy added inline comments.


================
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:
> 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.



================
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:
> 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. 


================
Comment at: llvm/include/llvm/MC/MCPseudoProbe.h:281
 /// unit.
-class MCPseudoProbeSection {
+class MCPseudoProbeSections {
 public:
----------------
wenlei wrote:
> do you want to rename this to MCPseudoProbeFunctions? 
This class kind of tells how probe encoding looks physically. Probes of a function will stay in a standalone section (comdat) and that's why section is used here. I'd like to keep it as is, but don't have a strong preference. WDYT?


================
Comment at: llvm/lib/MC/MCPseudoProbe.cpp:52
+  bool IsSentinel = isSentinelProbe(getAttributes());
+  assert(LastProbe ||
+         IsSentinel && "Last probe should not be null for non-sentinel probes");
----------------
wenlei wrote:
> Did you mean `assert((LastProbe || IsSentinel) && "...")`?
Good catch. Yeah, that's what I actually want.


================
Comment at: llvm/lib/MC/MCPseudoProbe.cpp:80
   } else {
-    // Emit label as a symbolic code address.
-    MCOS->emitSymbolValue(
-        Label, MCOS->getContext().getAsmInfo()->getCodePointerSize());
+    // Emit the GUID of the code range that the sentinel probe represents.
+    MCOS->emitInt64(Guid);
----------------
wenlei wrote:
> To be accurate, this is the GUID of the (split) binary function name / elf symbol name? 
Exactly.


================
Comment at: llvm/lib/MC/MCPseudoProbe.cpp:140
   });
+  assert(!isRoot() && "Root should not come here");
+
----------------
wenlei wrote:
> nit: this message doesn't provide additional info other than the assertion itself. the message could be something more meaningful - why we don't expect root node here
Fixed.


================
Comment at: llvm/lib/MC/MCPseudoProbe.cpp:155
+           "Starting probe of a top-level function should be a sentinel probe");
+    if (LastProbe->getGuid() != Guid)
+      NeedSentinel = true;
----------------
wenlei wrote:
> Is this checking the top level binary function name is different from the top level source function name?  And this is to avoid emitting sentinel for main function body?
Exactly. Added a comment about that.

The encoding for a function with outlined code will look like below. Note that the main entry doesn't need a sentinel probe.

```

GUID of Foo
  Probe 1

GUID of Foo 
  Sentinel probe of Foo.outlined 
  Probe 2
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135912