[BOLT][PseudoProbe] Support new pseudo probe encoding

[hoy]

Summary

This is the corresponding Bolt change to D135914: [PseudoProbe] Decode offset based pseudo probe.. To be
minimal intrusive, the pseudo probe re-encoding sticks with
the old encoding format. This is fine since unlike linker, Bolt
processes the pseudo probe section as a whole and it is free
from relocation overflow issues.

[maksfb]

Is this change going to be tested in the parent diff or otherwise?

A couple of nits: change [Bolt] -> [BOLT] in the Title and use 72-char line width for the Summary.

bolt/lib/Rewrite/RewriteInstance.cpp

611–612

I left the comment on the parent diff regarding the types.

3528

nit:

3529

nit: this line looks more than 80 chars wide.

[hoy]

In D136394#3872849, @maksfb wrote:

Is this change going to be tested in the parent diff or otherwise?

Good question. We have internal tests not upstreamed yet. Maybe it's time to upstream them?

bolt/lib/Rewrite/RewriteInstance.cpp

611–612

Thanks, fixed in the parent diff.

3529

oops, thought the linter would help.

[hoy]

Addressing comments.

[maksfb]

Looks good. There's a couple of styling nits. You might need to run arc with --verbatim to update Title and Summary fields.

bolt/lib/Rewrite/RewriteInstance.cpp

611–612

nit: following LLVM style for type names (https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)

613–614

nit: expand auto (https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable)

3527

[hoy]

Updating D136394: [Bolt][PseudoProbe] Support new pseudo probe encoding

[hoy]

In D136394#3876249, @maksfb wrote:

Looks good. There's a couple of styling nits. You might need to run arc with --verbatim to update Title and Summary fields.

I noticed that other Bolt commits has line wrapping in its summary. Does arc diff --verbatim do the trick?

[maksfb]

In D136394#3876345, @hoy wrote:

In D136394#3876249, @maksfb wrote:

Looks good. There's a couple of styling nits. You might need to run arc with --verbatim to update Title and Summary fields.

I noticed that other Bolt commits has line wrapping in its summary. Does arc diff --verbatim do the trick?

I mean if you fix Title+Summary locally, you'll need the flag to update the phabricator diff.

[wenlei]

bolt/lib/Rewrite/RewriteInstance.cpp

613

A quick question for bolt folks - does BinarayFunction represent individual segments of split functions; or does BinaryFunction simply maps to original function, with MCSymbol under a binary function representing each split funclets? Asking because we want to make sure naming convention is consistent for probe emission and bolt.

[maksfb]

bolt/lib/Rewrite/RewriteInstance.cpp

613

We have a limited support for inputs with split functions. At the moment, we have one BinaryFunction for each function fragment, but it doesn't work well for a number of reasons, and for a proper support we will eventually have to merge the fragments into a single instance of a BinaryFunction. When BOLT splits functions, each fragment will come from the same instance of BinaryFunction.

[maksfb]

@hoy, Amir will migrate the internal pseudo-probe tests. That should enable you to add/modify the test for this change.

[hoy]

In D136394#3884201, @maksfb wrote:

@hoy, Amir will migrate the internal pseudo-probe tests. That should enable you to add/modify the test for this change.

This is great! Actually I already have a fix to our internal pseudo probe tests. Will work with Amir to get that in.

[Amir]

In D136394#3884205, @hoy wrote:

In D136394#3884201, @maksfb wrote:

@hoy, Amir will migrate the internal pseudo-probe tests. That should enable you to add/modify the test for this change.

This is great! Actually I already have a fix to our internal pseudo probe tests. Will work with Amir to get that in.

Here's the test: D136729

[wenlei]

bolt/lib/Rewrite/RewriteInstance.cpp

613

Thanks for clarification. @hoy then I'd suggest let's not use the term binary function for split funclets during probe emission.

[hoy]

@maksim Does the current version look good to you? Any more comments? Thanks.

bolt/lib/Rewrite/RewriteInstance.cpp

613

Sounds good. Changed to using split function.

[hoy]

Rebasing

[maksfb]

LGTM. Thanks.

[hoy]

Closed by commit rGd5a963ab8b40

bolt/lib/Rewrite/RewriteInstance.cpp

void RewriteInstance::parsePseudoProbe() {
 
   StringRef Contents = PseudoProbeDescSection->getContents();
   if (!BC->ProbeDecoder.buildGUID2FuncDescMap(
           reinterpret_cast<const uint8_t *>(Contents.data()),
           Contents.size())) {
     errs() << "BOLT-WARNING: fail in building GUID2FuncDescMap\n";
     return;
   }
+
+  MCPseudoProbeDecoder::Uint64Set GuidFilter;
+  MCPseudoProbeDecoder::Uint64Map FuncStartAddrs;
+  for (const BinaryFunction *F : BC->getAllBinaryFunctions()) {
+    for (const MCSymbol *Sym : F->getSymbols()) {
+      FuncStartAddrs[Function::getGUID(NameResolver::restore(Sym->getName()))] =
+          F->getAddress();
+    }
+  }
   Contents = PseudoProbeSection->getContents();
   if (!BC->ProbeDecoder.buildAddress2ProbeMap(
-          reinterpret_cast<const uint8_t *>(Contents.data()),
-          Contents.size())) {
+          reinterpret_cast<const uint8_t *>(Contents.data()), Contents.size(),
+          GuidFilter, FuncStartAddrs)) {
     BC->ProbeDecoder.getAddress2ProbesMap().clear();
     errs() << "BOLT-WARNING: fail in building Address2ProbeMap\n";
     return;
   }
 
   if (opts::PrintPseudoProbes == opts::PrintPseudoProbesOptions::PPP_All ||
       opts::PrintPseudoProbes ==
           opts::PrintPseudoProbesOptions::PPP_Probes_Section_Decode) {
auto EmitSLEB128IntValue = [&](int64_t Value) {
     Contents.append(OSE.str().begin(), OSE.str().end());
   };
 
   // Emit indiviual pseudo probes in a inline tree node
   // Probe index, type, attribute, address type and address are encoded
   // Address of the first probe is absolute.
   // Other probes' address are represented by delta
   auto EmitDecodedPseudoProbe = [&](MCDecodedPseudoProbe *&CurProbe) {
+    assert(!isSentinelProbe(CurProbe->getAttributes()) &&
+           "Sentinel probes should not be emitted");
     EmitULEB128IntValue(CurProbe->getIndex());
     uint8_t PackedType = CurProbe->getType() | (CurProbe->getAttributes() << 4);
     uint8_t Flag =
         LastProbe ? ((int8_t)MCPseudoProbeFlag::AddressDelta << 7) : 0;
     EmitInt(Flag | PackedType, 1);
     if (LastProbe) {
       // Emit the delta between the address label and LastProbe.
       int64_t Delta = CurProbe->getAddress() - LastProbe->getAddress();
if (opts::PrintPseudoProbes == opts::PrintPseudoProbesOptions::PPP_All ||
           opts::PrintPseudoProbesOptions::PPP_Encoded_Probes) {
     // create a dummy decoder;
     MCPseudoProbeDecoder DummyDecoder;
     StringRef DescContents = PseudoProbeDescSection->getContents();
     DummyDecoder.buildGUID2FuncDescMap(
         reinterpret_cast<const uint8_t *>(DescContents.data()),
         DescContents.size());
     StringRef ProbeContents = PseudoProbeSection->getOutputContents();
+    MCPseudoProbeDecoder::Uint64Set GuidFilter;
+    MCPseudoProbeDecoder::Uint64Map FuncStartAddrs;
+    for (const BinaryFunction *F : BC->getAllBinaryFunctions()) {
+      const uint64_t Addr =
+          F->isEmitted() ? F->getOutputAddress() : F->getAddress();
+      FuncStartAddrs[Function::getGUID(
+          NameResolver::restore(F->getOneName()))] = Addr;
+    }
     DummyDecoder.buildAddress2ProbeMap(
         reinterpret_cast<const uint8_t *>(ProbeContents.data()),
-        ProbeContents.size());
+        ProbeContents.size(), GuidFilter, FuncStartAddrs);
     DummyDecoder.printProbesForAllAddresses(outs());
   }
 }
 
 void RewriteInstance::updateSDTMarkers() {
   NamedRegionTimer T("updateSDTMarkers", "update SDT markers", TimerGroupName,
                      TimerGroupDesc, opts::TimeRewrite);