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

[Kyungwoo Lee via Phabricator via llvm-commits]

kyulee 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
----------------
Not sure where you got, but it seems slightly different than the one in the linked paper.


================
Comment at: llvm/include/llvm/Support/BalancedPartitioning.h:35
+//   * Optimizing Function Layout for Mobile Applications,
+//     https://arxiv.org/abs/2211.09285
+//
----------------
Not blocking, but I wonder if you want to add a link to LCTES 2023 when the proceeding is available soon?


================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:178
+    for (auto &UN : N.UtilityNodes)
+      UN = UtilityNodeIndex[UN];
+
----------------
I'm not sure what this code does. I might miss something.


================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:316
+  for (auto &UN : N.UtilityNodes)
+    Gain += (FromLeftToRight ? Signatures[UN].CachedCostLR
+                             : Signatures[UN].CachedCostRL);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147812