[PATCH] D147812: [InstrProf] Use BalancedPartitioning to order temporal profiling trace data

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev added inline comments.
Herald added a subscriber: hoy.


================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:147
+  unsigned NumNodes = std::distance(Nodes.begin(), Nodes.end());
+  if (NumNodes < 1 || RecDepth >= Config.SplitDepth) {
+    // We've reach the lowest level of the recursion tree. Fall back to the
----------------



================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:200
+  unsigned NumNodes = std::distance(Nodes.begin(), Nodes.end());
+  DenseMap<BPFunctionNode::UtilityNodeT, unsigned> UtilityNodeDegree;
+  for (auto &N : Nodes)
----------------
How much overhead does `DenseMap` have in comparison with `std::vector`? I assume one could first find the maximum utility index and then use a vector instead of a map. Of course, that should only be done if there is visible speedup.


================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:212
+  // Renumber utility nodes so they can be used to index into Signatures
+  DenseMap<BPFunctionNode::UtilityNodeT, unsigned> UtilityNodeIndex;
+  for (auto &N : Nodes)
----------------
maybe add `UtilityNodeIndex.reserve(NumUtilities)`
(and use the variable for signature initialization too)


================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:257-264
+    float cost = logCost(L, R);
+    float CostLR = 0, CostRL = 0;
+    if (L > 0)
+      CostLR = cost - logCost(L - 1, R + 1);
+    if (R > 0)
+      CostRL = cost - logCost(L + 1, R - 1);
+    Signature.CachedCostLR = CostLR;
----------------



================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:362
+        N.UtilityNodes.begin(), N.UtilityNodes.end(), 0.f,
+        [&](float Gain, auto &UN) { return Signatures[UN].CachedCostLR; });
+  } else {
----------------
should it be `Gain + Signatures[UN].CachedCostLR` instead?

Assuming I'm correct and this is a bug, can we add a test to catch the problem?


================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:366
+        N.UtilityNodes.begin(), N.UtilityNodes.end(), 0.f,
+        [&](float Gain, auto &UN) { return Signatures[UN].CachedCostRL; });
+  }
----------------
same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147812