[PseudoProbe] Use callee name as callsite identfier for MCDecodedPseudoProbeInlineTree.

[hoy]

Summary

The callsite identifier used in pseudo probe encoding and decoding is consisted of a function name and the callsite probe id. For encoding, i.e., MCPseudoProbeInlineTree, the function name is callee function name. However for decoding, i.e., MCDecodedPseudoProbeInlineTree, the caller function name is used actually. This results in multiple callees that are inlined at the same callsite, likely via indirect call promotion, sharing the same decoded inline frame. While it is not a problem for profile generation, it confuses probe re-encoding in Bolt.

In Bolt, we decode pseudo probes first and build MCDecodedPseudoProbeInlineTree. The decoded tree is used for final re-encoding. Here comes the problem. Two inlinees from the same callsite share the same decoded inline frame. During re-encoding, the frame name (whatever inlinee comes first) will be used and encoded in the bolted binary. This will cause wrong inline contexts in the profile generated on the bolted binary.

The fix is a no-op to pre-bolt profile generation. Some of the bolt tests are not yet upstreamed, thus I'm not adding a bolt test here.

[wenlei]

llvm/lib/MC/MCPseudoProbe.cpp

576

is std::get<0>(xx->ISite) equivalent to xx->Parent->Guid?

[hoy]

llvm/lib/MC/MCPseudoProbe.cpp

576

They were equivalent, but they are not with this patch. Note that we are changing std::get<0>(xx->ISite) to the inlinee's Guid at line 419 above.

[wenlei]

lgtm, thanks

llvm/lib/MC/MCPseudoProbe.cpp

576

Got it. Can you update the comment below to make it explicit that the GUID is callee's.

// An inline frame has the form <Guid, ProbeID>
using InlineSite = std::tuple<uint64_t, uint32_t>;

[hoy]

Updating D126434: [PseudoProbe] Use callee name as callsite identfier for MCDecodedPseudoProbeInlineTree.

llvm/lib/MC/MCPseudoProbe.cpp

576

Good point, done.

llvm/include/llvm/MC/MCPseudoProbe.h

struct MCPseudoProbeFuncDesc {
   MCPseudoProbeFuncDesc(uint64_t GUID, uint64_t Hash, StringRef Name)
       : FuncGUID(GUID), FuncHash(Hash), FuncName(Name){};
 
   void print(raw_ostream &OS);
 };
 
 class MCDecodedPseudoProbe;
 
-// An inline frame has the form <Guid, ProbeID>
+// An inline frame has the form <CalleeGuid, ProbeID>
 using InlineSite = std::tuple<uint64_t, uint32_t>;
 using MCPseudoProbeInlineStack = SmallVector<InlineSite, 8>;
 // GUID to PseudoProbeFuncDesc map
 using GUIDProbeFunctionMap =
     std::unordered_map<uint64_t, MCPseudoProbeFuncDesc>;
 // Address to pseudo probes map.
 using AddressProbesMap =
     std::unordered_map<uint64_t, std::list<MCDecodedPseudoProbe>>;
public:
   InlineSite ISite;
   // Used for decoding
   uint32_t ChildrenToProcess = 0;
 
   MCDecodedPseudoProbeInlineTree() = default;
   MCDecodedPseudoProbeInlineTree(const InlineSite &Site) : ISite(Site){};
 
   // Return false if it's a dummy inline site
-  bool hasInlineSite() const { return std::get<0>(ISite) != 0; }
+  bool hasInlineSite() const { return !isRoot() && !Parent->isRoot(); }
 };
 
 /// Instances of this class represent the pseudo probes inserted into a compile
 /// unit.
 class MCPseudoProbeSection {
 public:
   void addPseudoProbe(MCSection *Sec, const MCPseudoProbe &Probe,
                       const MCPseudoProbeInlineStack &InlineStack) {

llvm/lib/MC/MCPseudoProbe.cpp

 void MCDecodedPseudoProbe::getInlineContext(
     SmallVectorImpl<MCPseduoProbeFrameLocation> &ContextStack,
     const GUIDProbeFunctionMap &GUID2FuncMAP) const {
   uint32_t Begin = ContextStack.size();
   MCDecodedPseudoProbeInlineTree *Cur = InlineTree;
   // It will add the string of each node's inline site during iteration.
   // Note that it won't include the probe's belonging function(leaf location)
   while (Cur->hasInlineSite()) {
-    StringRef FuncName =
-        getProbeFNameForGUID(GUID2FuncMAP, std::get<0>(Cur->ISite));
+    StringRef FuncName = getProbeFNameForGUID(GUID2FuncMAP, Cur->Parent->Guid);
     ContextStack.emplace_back(
         MCPseduoProbeFrameLocation(FuncName, std::get<1>(Cur->ISite)));
     Cur = static_cast<MCDecodedPseudoProbeInlineTree *>(Cur->Parent);
   }
   // Make the ContextStack in caller-callee order
   std::reverse(ContextStack.begin() + Begin, ContextStack.end());
 }
 
bool MCPseudoProbeDecoder::buildAddress2ProbeMap(
   // Decide if top-level node should be disgarded.
   if (Cur == &DummyInlineRoot && !GuildFilter.empty() &&
       !GuildFilter.count(Guid))
     Cur = nullptr;
 
   // If the incoming node is null, all its children nodes should be disgarded.
   if (Cur) {
     // Switch/add to a new tree node(inlinee)
-    Cur = Cur->getOrAddNode(std::make_tuple(Cur->Guid, Index));
+    Cur = Cur->getOrAddNode(std::make_tuple(Guid, Index));
     Cur->Guid = Guid;
   }
 
   // Read number of probes in the current node.
   auto ErrorOrNodeCount = readUnsignedNumber<uint32_t>();
   if (!ErrorOrNodeCount)
     return false;
   uint32_t NodeCount = std::move(*ErrorOrNodeCount);
InlineContextStack.emplace_back(
       MCPseduoProbeFrameLocation(FuncDesc->FuncName, Probe->getIndex()));
 }
 
 const MCPseudoProbeFuncDesc *MCPseudoProbeDecoder::getInlinerDescForProbe(
     const MCDecodedPseudoProbe *Probe) const {
   MCDecodedPseudoProbeInlineTree *InlinerNode = Probe->getInlineTreeNode();
   if (!InlinerNode->hasInlineSite())
     return nullptr;
-  return getFuncDescForGUID(std::get<0>(InlinerNode->ISite));
+  return getFuncDescForGUID(InlinerNode->Parent->Guid);
 }