[XRay][tools] Use symbols instead of function id

[dberris]

Summary

In the graph-diff tool, we should use the symbol name for the labels in
the graph instead of the function ids.

[kristina]

Added some inline comments.

llvm/tools/llvm-xray/xray-graph-diff.cpp

349

Drop the formatv and the raw literal. It's in the old code sure, but formatv implementation in LLVM is rather expensive for two braces and the raw literal here makes zero sense. Use a Twine instead. Same goes for a a few below.

358

truncateString returns a Twine, you're not storing it anywhere so it's still on the stack at that point, what's the point of the explicit conversion here? .str(). A lot of things including formatv can accept a Twine as an argument.

[kristina]

Quite a big style nit. I'd say get rid of the switch completely.

llvm/tools/llvm-xray/xray-graph-diff.cpp

353

• Default is generally the first case in a switch.
• Switch case, followed by if, followed by return. Somewhat difficult to parse.
• No extra indentation for case statements.
• Is there a case for using a switch statement here at all? I think just an if statement will do, plus you can make use of early return here more easily. Switch is generally hard to parse (by a human) construct so I think it's generally a bad idea to use 2 case switches.

[kristina]

Gentle ping for author, has this landed or still in the backlog? (If it's landed can you attach the commit and close it, please? Just trying to to avoid diffs on Phab going stale.)

[dberris]

In D56383#1411871, @kristina wrote:

Gentle ping for author, has this landed or still in the backlog? (If it's landed can you attach the commit and close it, please? Just trying to to avoid diffs on Phab going stale.)

Sorry for the delay, this is still worth doing and this is still broken. I'll update this with an end to end test to ensure we have it fixed and won't regress. I've had some recent changes with my work situation that's making this less of a priority for me, hence the silence.

llvm/tools/llvm-xray/xray-graph-diff.cpp

default:
     const auto &LeftStat = EdgeAttr.CorrEdgePtr[0]->second.S;
     const auto &RightStat = EdgeAttr.CorrEdgePtr[1]->second.S;
 
     double RelDiff = statRelDiff(LeftStat, RightStat, EL);
     return formatv(R"({0:P})", RelDiff);
   }
 }
 
+template <typename T>
+static std::string anySymbol(const T &Collection, const StringRef &DefValue) {
+  for (const auto &E : Collection)
+    if (E != nullptr)
+      return E->second.SymbolName;
+  return DefValue;
+}
+
 static std::string getLabel(const GraphDiffRenderer::GraphT::VertexValueType &V,
                             GraphDiffRenderer::StatType VL, int TrunLen) {
   const auto &VertexId = V.first;
   const auto &VertexAttr = V.second;
   switch (VL) {
   case GraphDiffRenderer::StatType::NONE:
-    return formatv(R"({0})", truncateString(VertexId, TrunLen).str());
+    return formatv(
+        R"({0})",
+        truncateString(anySymbol(VertexAttr.CorrVertexPtr, VertexId), TrunLen)
+            .str());
   default:
     if (containsNullptr(VertexAttr.CorrVertexPtr))
-      return formatv(R"({0})", truncateString(VertexId, TrunLen).str());
+      return formatv(
+          R"({0})",
+          truncateString(anySymbol(VertexAttr.CorrVertexPtr, VertexId), TrunLen)
+              .str());
 
     const auto &LeftStat = VertexAttr.CorrVertexPtr[0]->second.S;
     const auto &RightStat = VertexAttr.CorrVertexPtr[1]->second.S;
 
     double RelDiff = statRelDiff(LeftStat, RightStat, VL);
-    return formatv(R"({{{0}|{1:P}})", truncateString(VertexId, TrunLen).str(),
-                   RelDiff);
+    return formatv(
+        R"({{{0}|{1:P}})",
+        truncateString(VertexAttr.CorrVertexPtr[0]->second.SymbolName, TrunLen)
+            .str(),
   }
 }
 
 static double getLineWidth(const GraphDiffRenderer::GraphT::EdgeValueType &E,
                            GraphDiffRenderer::StatType EL) {
   auto &EdgeAttr = E.second;
   switch (EL) {
   case GraphDiffRenderer::StatType::NONE:
OS << formatv(R"(F{0} [label="{1}" color="{2}"])"
                   VertexNo[VertexId], getLabel(V, VertexLabel, TruncLen),
                   getColor(V, G, H, VertexColor));
   }
 
   OS << "}\n";
 }
 
 template <typename T> static T &ifSpecified(T &A, cl::alias &AA, T &B) {
-  if (A.getPosition() == 0 && AA.getPosition() == 0)
+  if (A.getPosition() == 0)
     return B;
 
   return A;
 }
 
 static CommandRegistration Unused(&GraphDiff, []() -> Error {
   std::array<GraphRenderer::Factory, 2> Factories{
       {{ifSpecified(GraphDiffKeepGoing1, GraphDiffKeepGoing1A,