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

[Ellis Hoag via Phabricator via llvm-commits]

ellis added inline comments.


================
Comment at: llvm/include/llvm/Support/BalancedPartitioning.h:26
+// number of greedy iterations per split (IterationsPerSplit). The worst-case
+// time complexity of the implementation is bounded by O(M*log^2 N), where
+// N is the number of FunctionNodes and M is the number of
----------------
kyulee wrote:
> Not sure where you got, but it seems slightly different than the one in the linked paper.
Yes, the paper reports a runtime of O(m log n + n log2 n). However O(m log2 n) should be slightly worse and much easier to understand, so I think this is good enough 🙂


================
Comment at: llvm/include/llvm/Support/BalancedPartitioning.h:35
+//   * Optimizing Function Layout for Mobile Applications,
+//     https://arxiv.org/abs/2211.09285
+//
----------------
kyulee wrote:
> Not blocking, but I wonder if you want to add a link to LCTES 2023 when the proceeding is available soon?
Since the paper we submitted to LCTES 2023 is slightly shorter I think it makes sense to keep this arxiv link which contains our whole paper.


================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:178
+    for (auto &UN : N.UtilityNodes)
+      UN = UtilityNodeIndex[UN];
+
----------------
kyulee wrote:
> I'm not sure what this code does. I might miss something.
The contents of `BPFunctionNode::UtilityNodes` could originally contain arbitrary values. To make matters worse, each bisect step could shuffle these nodes into different sections.

Here we are computing `UtilityNodeIndex` to be a map from `UtilityNodeT` to an int in range [0,N) where N is the number of unique utility nodes. Then we can update each utility node so they are in range [0,N). Then we can make `SignaturesT` a normal vector indexed by utility nodes for performance.


================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:316
+  for (auto &UN : N.UtilityNodes)
+    Gain += (FromLeftToRight ? Signatures[UN].CachedCostLR
+                             : Signatures[UN].CachedCostRL);
----------------
kyulee wrote:
> Given `logCost` is a negative value which we want to minimize for an objective, my read on `CachedCostLR` is actually a cost saving (or gain) when `N` is moved from Left to Right. So, wouldn't it make sense naming it `CachedCostSavingLR` or `CachedGainLR` so that accumulating them to `Gain` sounds natural?
I've renamed it to `CachedGainLR` which makes sense because we want to maximize it. Thanks for the suggestion! 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147812